Page MenuHomePhabricator

[WebAssembly] Move Emscripten-specific behavior under a --emscripten flag
AbandonedPublic

Authored by sunfish on May 24 2019, 9:58 PM.

Details

Summary

This adds a --emscripten command-line option to lld, for use by Emscripten.

Issue an error if -shared or -export-dynamic are used without --emscripten, as the current infrastructure for dynamic linking is specific to Emscripten. We've had reports of users attempting to use these options outside of Emscripten, so it's desirable to diagnose this.

And, don't default to exporting heap_base or data_end without --emscripten. Exporting these exposes implementation details. Embeddings which need them exported can use -Wl,--export=__heap_base and -Wl,--export=__data_end as needed to request this explicitly. With this change, statically-linked WASI executables only export "memory" and "_start".

Diff Detail

Repository
rLLDB LLDB

Event Timeline

sunfish created this revision.May 24 2019, 9:58 PM

imo export-dynamic should still work? All it does it export all symbols that are linked in. My compiler uses and depends on that feature (though I can add emscripten as a flag to the linker, I don't think I should).

imo export-dynamic should still work? All it does it export all symbols that are linked in. My compiler uses and depends on that feature (though I can add emscripten as a flag to the linker, I don't think I should).

Can you use --export-all instead of --export-dynamic? On other platforms --export-dynamic exports symbols in the dynamic symbol table, which is not something that wasm has currently. When we do design dynamic linking for wasm, we may have a need for an option like --export-dynamic, and it would be unfortunate if it already means something different.

I suppose I could; do note they do quite different things:

if (ForceExport || Config->ExportAll)
  return true;

if (Config->ExportDynamic && !isHidden())
  return true;

the nice thing about the dynamic option was to not expose hidden symbols.

sunfish updated this revision to Diff 201735.May 28 2019, 11:52 AM

This patch now depends on a corresponding LLVM patch: https://reviews.llvm.org/D62542

This new patch reverts the change to --export-dynamic, to address review feedback. So now, this patch just:

  • Makes data_end and heap_base hidden, so that they're not automatically exported if they're just used.
  • Adds --emscripten to enable Emscripten compatibility mode, and restricts -shared and -pie to --emscripten mode.
  • Adds support for the IMPLICITLY_USED flag added in D62542.

Wouldn't it be better to try to avoid adding a --emscripten?

I think for heap_base/data_end we should change emscripten to export them explicitly. (I'll look into doing that).

Once we do that then this change only removes the -pie -shared behaviour, right?

docs/WebAssembly.rst
135 ↗(On Diff #201735)

The `__attribute__((used))` behaviour is depend on the compiler time triple isn't it ? Not the linker flag?

Wouldn't it be better to try to avoid adding a --emscripten?

I think for heap_base/data_end we should change emscripten to export them explicitly. (I'll look into doing that).

Sounds good.

Once we do that then this change only removes the -pie -shared behaviour, right?

Right, those are the original motivation for the --emscripten option. I'm open to other options here.

sbc100 added a comment.EditedMay 31 2019, 11:04 AM

I have proposed https://reviews.llvm.org/D62744 which covers most of the changes here.

For -pie/-shared I'd rather not add a --emscripten flag. In fact I'd rather not have such a flag at all. Some background that might help here: The PIC support in LLVM conforms to the (admittedly unstable/experimental) spec specified in https://github.com/WebAssembly/tool-conventions/blob/master/DynamicLinking.md. Note that this document includes both the "emscripten" way of doing things and the future "Planned Changes" way of doing things which is implemented in LLVM. emscripten-wasm-finalize, which is part of binaryen, then converts from the LLVM/official ABI to the emscripten ABI for dynamic linking. I will update the docs to be more clear but as I see it non of the PIC support in LLVM is emscripten-specific.

If you really want to add a new flag I'd be OK with "--experimental".. but I'd rather not have either. Have you been experiencing issues with people passing -pie/-shared this flag and getting unexpected results?

I have proposed https://reviews.llvm.org/D62744 which covers most of the changes here.

For -pie/-shared I'd rather not add a --emscripten flag. In fact I'd rather not have such a flag at all. Some background that might help here: The PIC support in LLVM conforms to the (admittedly unstable/experimental) spec specified in https://github.com/WebAssembly/tool-conventions/blob/master/DynamicLinking.md. Note that this document includes both the "emscripten" way of doing things and the future "Planned Changes" way of doing things which is implemented in LLVM. emscripten-wasm-finalize, which is part of binaryen, then converts from the LLVM/official ABI to the emscripten ABI for dynamic linking. I will update the docs to be more clear but as I see it non of the PIC support in LLVM is emscripten-specific.

I agree that the design isn't necessarily specific to Emscripten. My concern is that it seems to have some limitations, which I've been assuming mean that we'd want to design a new system when it comes time to add dynamic linking outside of Emscripten anyway. I've commented on other issues about the specific concerns, so we can follow up there.

If you really want to add a new flag I'd be OK with "--experimental".. but I'd rather not have either. Have you been experiencing issues with people passing -pie/-shared this flag and getting unexpected results?

Yes, we have reports both of people using -Wl,--shared and having things not work, and of people compiling with -fPIC and having things not work. These are what prompted me to write this patch and the LLVM one.

Can you rebase now that https://reviews.llvm.org/D62744 has landed?

Maybe worth splitting out the isImplicitlyUsed part of this change too? especially since the llvm side of that is still being discussed.

ruiu added a subscriber: ruiu.Jun 2 2019, 9:06 PM

I'm not in favor of this patch. Adding a blanket option for emscripten seems too specific to one use case. If you really need a special feature for emscripten, you probably should decompose it to fine-grained features and pass command line flags to enable the features from emscripten compiler driver.

sunfish updated this revision to Diff 202826.Jun 3 2019, 5:02 PM
  • Rebase
  • Rename IMPLICITLY_USED to NO_STRIP

Talking with @sbc100 offline, an alternative to the --emscripten command-line flag would be to write the target triple into .o files, so that the linker can determine if it should enable Emscripten-specific behavior based on that, rather than needing a command-line flag.

The patch here just rebases the earlier patch and updates the name of the new NO_STRIP flag, so that it's easier to see what the changes are.