Skip to content
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

Merged
merged 1 commit into from
Aug 15, 2018
Merged

Export WASM table by default #53237

merged 1 commit into from
Aug 15, 2018

Conversation

overdrivenpotato
Copy link
Contributor

This allows loading a rust-generated .wasm binary in a host and using the exported table much like the memory export.

@rust-highfive
Copy link
Collaborator

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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 9, 2018
@nikomatsakis
Copy link
Contributor

r? @alexcrichton

@alexcrichton
Copy link
Member

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 -C link-args to disable this behavior?

@overdrivenpotato
Copy link
Contributor Author

You can use -C link-arg=--import-table to use an imported table instead of generating an exported table. However, there is no way to revert to the old behavior (a private table). With this in mind, marking the table exported should not be an issue in any WebAssembly binary as the behavior of the table stays the same. It is simply accessible from the host now. In practice, this avoids the need for a custom trampolining function to handle callbacks between Rust and the host environment, as we can instead use the native WebAssembly table feature.

@alexcrichton
Copy link
Member

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 wasm-bindgen does today to implement callbacks for rust modules. (despite the table not by default being exported it ensures that it's flagged to get exported)

@overdrivenpotato
Copy link
Contributor Author

overdrivenpotato commented Aug 14, 2018

If the only concern is binary size, it's worth noting that the --export-table flag will only add 28 bytes to the final binary. Couldn't we then consider removing this table export in a build step to be an edge case optimization, rather than having to patch it back in for almost all use cases?

It's also worth noting that the memory export is exported by default as well and I don't believe there is a linker flag to disable it (correct me if I'm wrong). Enabling this flag by default should reduce the friction required for users, while allowing those who really need the few extra bytes to manually remove this export (and potentially the memory export as well) in a custom build step.

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 --export-table - 270 bytes:

$ RUSTFLAGS="-C link-arg=--export-table" cargo build --target wasm32-unknown-unknown --release
   Compiling test_lld v0.1.0 (file:///private/tmp/test_lld)
    Finished release [optimized] target(s) in 0.18s
$ ls -alh target/wasm32-unknown-unknown/release/test_lld.wasm
-rwxr-xr-x  2 marko  wheel   270B 13 Aug 21:30 target/wasm32-unknown-unknown/release/test_lld.wasm

Without --export-table - 242 bytes:

$ cargo build --target wasm32-unknown-unknown --release
   Compiling test_lld v0.1.0 (file:///private/tmp/test_lld)
    Finished release [optimized] target(s) in 0.27s
$ ls -alh target/wasm32-unknown-unknown/release/test_lld.wasm
-rwxr-xr-x  2 marko  wheel   242B 13 Aug 21:29 target/wasm32-unknown-unknown/release/test_lld.wasm

@alexcrichton
Copy link
Member

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!

@fitzgen
Copy link
Member

fitzgen commented Aug 14, 2018

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

@alexcrichton
Copy link
Member

@bors: r+

Ok! Sounds good!

@bors
Copy link
Contributor

bors commented Aug 14, 2018

📌 Commit c7a39b1 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 14, 2018
@overdrivenpotato
Copy link
Contributor Author

LLVM exports the table as indirect_function_table, that’s most of the additional bloat. There’s also a few extra bytes to add an export entry. It will always be 28-29 bytes (possibly one extra with LEB128), no matter the size of the actual Rust project.

@bors
Copy link
Contributor

bors commented Aug 15, 2018

⌛ Testing commit c7a39b1 with merge 0f4b498...

bors added a commit that referenced this pull request Aug 15, 2018
…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.
@bors
Copy link
Contributor

bors commented Aug 15, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 0f4b498 to master...

@bors bors merged commit c7a39b1 into rust-lang:master Aug 15, 2018
pepyakin added a commit to paritytech/substrate that referenced this pull request Aug 24, 2018
Because it was turned on by default in the recent nightlies.

See rust-lang/rust#53237
pepyakin added a commit to paritytech/substrate that referenced this pull request Aug 24, 2018
Because it was turned on by default in the recent nightlies.

See rust-lang/rust#53237
rphmeier pushed a commit to paritytech/substrate that referenced this pull request Aug 25, 2018
* 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
```
Centril added a commit to Centril/rust that referenced this pull request Jan 24, 2019
…alexcrichton

Don't export table by default in wasm

Revert of rust-lang#53237
As per discussion here rustwasm/team#251
Centril added a commit to Centril/rust that referenced this pull request Jan 24, 2019
…alexcrichton

Don't export table by default in wasm

Revert of rust-lang#53237
As per discussion here rustwasm/team#251
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants