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

Valid imports and exports on the web #585

Closed
dschuff opened this issue Jun 13, 2016 · 9 comments
Closed

Valid imports and exports on the web #585

dschuff opened this issue Jun 13, 2016 · 9 comments

Comments

@dschuff
Copy link
Member

dschuff commented Jun 13, 2016

Currently s2wasm marks all functions marked .globl in the .s file as exported. (This basically corresponds to anything with external linkage in LLVM, and non-static functions in C). This results in a lot of exported functions from most wasm modules, including things that must be gobal in C but clearly need not be exported (e.g. libc and compiler-rt functions).
Having too many things exported is inefficient (because it causes the JS engine to generate a lot of unnecessary code at instantiation time), but also causes worse problems because on the web, no imported or exported function can have i64 return or parameter types, and of course many functions generated by the LLVM wasm backend do. So we need to restrict the exports for wasm backend output to be usable on the web at all.

We've previously discussed using hidden visibility to do this, and I think that's a fine place to start, given that we have to do something for anything to be usable. Clang already has hidden visibility by default (i.e. the equivalent of using the -fvisibility=hidden flag), so we'd just change s2wasm to be aware of visibility. Wasm exports don't quite match the semantics or use case of DSO/DLL exports on other platforms and we'll want to have our own dynamic linking implementation as well. Probably before MVP we'll want to think a bit more carefully about that choice, and what it might mean with respect to our expected dynamic linking implementation and whether we use ELF or not for a container format.

The immediate result will probably be that we need to manually mark some functions as exported in various headers, and code generated in various places, as well as have a specified way for users to do the same. For now we can use the __attribute__((visibility ("default")) syntax already supported by clang.

I guess the discussion here would be around 2 points:

  1. Objections to starting with visibility to determine exports?
  2. What should we use longer-term?
    And maybe also,
  3. What do we expect will break when we do this? I'm not super worried about fixing everything before activating this behavior, since nothing actually works right now. Presumably most of these changes would be in emscripten.
@dschuff
Copy link
Member Author

dschuff commented Jun 13, 2016

@jfbastien, @sunfishcode, @kripken

@kripken
Copy link
Member

kripken commented Jun 14, 2016

In Emscripten currently,

  1. Nothing is exported by default
  2. The user can add exported functions to the EXPORTED_FUNCTIONS compiler flag
  3. The user can also export functions in source code, using a macro which currently lowers to __attribute__((used))

The compiler flag option (2) can just be passed to s2wasm (we do it with other backends).

For source code exporting (3), perhaps using visibility makes more sense, as you suggest. Do I understand correctly that we can do that without any breaking changes?

@dschuff
Copy link
Member Author

dschuff commented Jun 14, 2016

Ah, I didn't realize that about emscripten currently. That should make things easier since there won't be any expectation of things working without some manual intervention by the user. I agree that for 2) we can just plumb something through to s2wasm. I'm not sure __attribute__((used)) can do what we want for wasm; IIRC it only prevents the function from being omitted/DCEd. If that's the only effect the macro has in asm.js, that would seem to imply that everything is implicitly exported just by virtue of its being in the module?

For visibility, I supposed it depends on what you mean by breaking changes. If there is a user who is already depending on the "everything is exported" property, then this will break them. But it seems like we still want to not have everything exported, and of course we haven't shipped yet so breaking changes like that should be expected :). Going forward, if someone uses default visibility to cause functions to be exported from their wasm modules, then we will have the question of what that means for dynamic libraries. We might conceivably want to separate the idea of wasm exports (e.g. to the environment) from the idea of DSO/DLL exports.

dschuff added a commit that referenced this issue Jun 14, 2016
As discussed in #585, this change makes s2wasm more aware of the
symbol visibility output from LLVM, and only exports global functions
with default visibility (instead of all globals as before).
@binji
Copy link
Member

binji commented Jun 14, 2016

The user can add exported functions to the EXPORTED_FUNCTIONS compiler flag

I actually found this to be pretty tedious; I ended up writing a script to automatically generate this in sexpr-wasm -- because the format is either quoted JSON on the commandline or a JSON file passed as response file. Wish I knew about __attribute__((used)), though I don't really like having to annotate the source either... :-|

@dschuff
Copy link
Member Author

dschuff commented Jun 14, 2016

Yeah, JSON seems not-great, for the command line at least. Assuming we do end up requiring or allowing exports to be specified externally to the program sources (e.g. a response file), it might make sense to have something unified with #558. Although for #558 we'd need a way to specify types as well as just names, so maybe something more structured than just a list of names would be needed.

dschuff added a commit that referenced this issue Jun 14, 2016
As discussed in #585, this change makes s2wasm more aware of the
symbol visibility output from LLVM, and only exports global functions
with default visibility (instead of all globals as before).
@kripken
Copy link
Member

kripken commented Jun 15, 2016

@dschuff: in the asm.js backend we hackishly interpret the "used" attribute as "keep this alive, and export it". We could do the same in the wasm backend and things would just work. Or, the optimal thing is probably a new attribute, __attribute__((export))?

guybedford pushed a commit to guybedford/binaryen that referenced this issue Mar 15, 2017
As discussed in WebAssembly#585, this change makes s2wasm more aware of the
symbol visibility output from LLVM, and only exports global functions
with default visibility (instead of all globals as before).
guybedford pushed a commit to guybedford/binaryen that referenced this issue Mar 15, 2017
As discussed in WebAssembly#585, this change makes s2wasm more aware of the
symbol visibility output from LLVM, and only exports global functions
with default visibility (instead of all globals as before).
dcodeIO pushed a commit to dcodeIO/binaryen that referenced this issue May 5, 2017
As discussed in WebAssembly#585, this change makes s2wasm more aware of the
symbol visibility output from LLVM, and only exports global functions
with default visibility (instead of all globals as before).
@alexcrichton
Copy link
Contributor

I've been playing around with binaryen, the LLVM WASM backend and Rust lately and I think this may help us quite a bit right now! We generate intrinsics like "memcpy" and such into LLVM modules and we're forced to export them all the way through to codegen to ensure it's not an unresolved symbol (for now). These are all currently hidden symbols but they currently get exported from the wasm module.

A good first step I think would be to avoid exporting these symbols, with an ideal end goal of GC'ing them out during the linking phase that s2wasm/wasm-as is doing, but I'd be fine with just not exporting to start out with!

Would y'all still be interested in a patch for this? It looked like it wouldn't be too hard to shim this in to the current s2wasm (just avoiding exporting hidden functions)

@dschuff
Copy link
Member Author

dschuff commented Oct 26, 2017

We hope to be able to replace s2wasm before too long. But for now I think it would be OK to make s2wasm's behavior match what emcc has for asm2wasm, which sounds like it could mean reducing the exports even more than that. This is something I'd been meaning to do "sometime" so if you wanted to pick that up that would be OK. At minimum we need to export functions named on the command line and things marked with EMSCRIPTEN_KEEPALIVE (I think that's what the macro is currently called? We discussed making something like EMSCRIPTEN_EXPORT based on the discussion above but not sure that we actually did). We probably do want to eventually have a visibility-like model but since that probably means exporting more things compared to the current asmjs behavior, switching to that probably wouldn't break things.

@tlively
Copy link
Member

tlively commented Jan 6, 2025

s2wasm no longer exists (and neither does asm2wasm).

@tlively tlively closed this as completed Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants