-
Notifications
You must be signed in to change notification settings - Fork 13k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Export WASM table by default #53237
Export WASM table by default #53237
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
Thanks for the PR @overdrivenpotato! For these sorts of flags to LLD we tend to just want to make sure that it can be turned off as well. Is there something that can be passed via |
You can use |
Ah ok interesting! In that case we may not want to have this turned on by default (as it can't be turned off). While true that it doesn't affect the behavior of the binary it does make it a bit larger because the table section has to get exported. Note that it's also pretty easy to export the table section in an existing wasm binary, this is what |
If the only concern is binary size, it's worth noting that the It's also worth noting that the Test case: #![no_std]
#![feature(lang_items, panic_implementation, core_intrinsics)]
#[panic_implementation]
pub fn panic_fmt(_fmt: &core::panic::PanicInfo) -> ! {
unsafe { core::intrinsics::abort() }
}
#[lang = "eh_personality"]
pub fn eh_personality() {}
extern {
fn test(f: extern fn() -> i32);
}
#[lang = "start"]
pub fn main() {
extern fn cb() -> i32 {
123
}
unsafe {
test(cb);
}
} With
Without
|
Ah good points! While it seems small people are definitely counting bytes when looking at Rust "hello world" use cases on wasm, but this seems like a handful of bytes that may not matter too much. @fitzgen can you think of any reason why this might cause problems? I'd be tempted to merge this myself now! |
Is most of the bytes the name of the exported table? I'm fine with this so long as the bloat is not proportional to the size of the wasm binary, which I don't expect it to be, or else things have gone really wrong :-p |
@bors: r+ Ok! Sounds good! |
📌 Commit c7a39b1 has been approved by |
LLVM exports the table as |
…hton Export WASM table by default This allows loading a rust-generated `.wasm` binary in a host and using the exported table much like the `memory` export.
☀️ Test successful - status-appveyor, status-travis |
Because it was turned on by default in the recent nightlies. See rust-lang/rust#53237
Because it was turned on by default in the recent nightlies. See rust-lang/rust#53237
* Don't use --export-table anymore Because it was turned on by default in the recent nightlies. See rust-lang/rust#53237 * use_extern_macros stabilization With recent nightlies rustc produces a warning ``` the feature `use_extern_macros` has been stable since 1.30.0 and no longer requires an attribute to enable ```
…alexcrichton Don't export table by default in wasm Revert of rust-lang#53237 As per discussion here rustwasm/team#251
…alexcrichton Don't export table by default in wasm Revert of rust-lang#53237 As per discussion here rustwasm/team#251
This allows loading a rust-generated
.wasm
binary in a host and using the exported table much like thememory
export.