Page MenuHomePhabricator

[WebAssembly] Make Emscripten-specific behavior specific to the Emscripten target
ClosedPublic

Authored by sunfish on May 28 2019, 11:48 AM.

Details

Summary

Make some behaviors added for Emscripten compatibility conditional on the target being Emscripten.

  • Add an WASM_SYMBOL_IMPLICITLY_USED flag, so that attribute((used)) doesn't need to imply exporting. When targeting Emscripten, have WASM_SYMBOL_IMPLICITLY_USED imply exporting.
  • Limit PIC support to the Emscripten target, since the current PIC support is Emscripten-specific.

Diff Detail

Repository
rL LLVM

Event Timeline

sunfish created this revision.May 28 2019, 11:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2019, 11:48 AM
sbc100 accepted this revision.May 28 2019, 12:02 PM
sbc100 added inline comments.
include/llvm/BinaryFormat/Wasm.h
316 ↗(On Diff #201734)

Why "implicitly"? It seems like it a very explicit attribute :) How about just "WASM_SYMBOL_NO_STRIP" to match MCSA_NoDeadStrip and the mach-o implementation which is .no_dead_strip at the asm level.

This revision is now accepted and ready to land.May 28 2019, 12:02 PM
sbc100 added inline comments.May 28 2019, 3:49 PM
lib/MC/MCWasmStreamer.cpp
125 ↗(On Diff #201734)

It looks like this is the only caller of setExported so you can remove setExported and isExported and probably also remove WASM_SYMBOL_EXPORTED?

In which case why not just rename WASM_SYMBOL_EXPORTED rather than adding another flag? Or can you imagine the EXPORTED flag getting set in some other way?

LGTM in general. Maybe IMPLICITLY_EXPORTED would be an even better name?
Anyway, I'm hoping we can make that export behavior nicer soon; I find the attribute(used) -> export behavior a bit odd too. Once we drop fastcomp it will be easier to redefine EMSCRIPTEN_KEEPALIVE and other things.

dschuff accepted this revision.May 29 2019, 8:47 AM
sunfish marked an inline comment as done.Jun 3 2019, 4:55 PM
sunfish added inline comments.
lib/MC/MCWasmStreamer.cpp
125 ↗(On Diff #201734)

I can remove setExported and isExported. But we appear to still need WASM_SYMBOL_EXPORTED -- there's also the code in lib/MC/WasmObjectWriter.cpp which adds wasm::WASM_SYMBOL_EXPORTED to symbols with NoStrip in Emscripten mode, for compatibility with Emscripten, going by this comment.

sunfish updated this revision to Diff 202825.Jun 3 2019, 4:57 PM
  • Rename IMPLICITLY_USED to NO_STRIP.
  • Remove isExported and setExported accessors.
sbc100 added a comment.Jun 4 2019, 9:25 AM

I was hoping you could have the linker default to exporting NO_STRIP symbols? If you don't like that idea then perhaps would have the "export NO_STRIP symbols" behaviour be emscripten-only in the linker and have it driven by the same signal we end up using for -shared/-pie.

So then we can just rename the existing EXPORTED to NO_STRIP to avoid adding another flag here?

Do do you think we will end up wanted a separate EXPORTED flag for symbol in any case? For example with WASI, will we will presumably want some way to export symbols based on source attributes? Should we add wasm-export-name attribute like we have for wasm-import-name?

I was hoping you could have the linker default to exporting NO_STRIP symbols? If you don't like that idea then perhaps would have the "export NO_STRIP symbols" behaviour be emscripten-only in the linker and have it driven by the same signal we end up using for -shared/-pie.

It doesn't have that meaning in ELF targets, for example.

Do do you think we will end up wanted a separate EXPORTED flag for symbol in any case? For example with WASI, will we will presumably want some way to export symbols based on source attributes?

I don't know yet. There still are some questions about whether we want to have a separate layer of visibility between ESM's and C++ libraries.

Should we add wasm-export-name attribute like we have for wasm-import-name?

Yes, I expect we'll want this, though I myself don't have anything that urgently needs it.

sunfish updated this revision to Diff 203234.Jun 5 2019, 1:06 PM

The PIC portion of this patch is now split off as r362638. This patch now just contains the __attribute__((used))/no_dead_strip changes.

https://reviews.llvm.org/D66968 is now a patch implementing the lld side of this.

This revision was automatically updated to reflect the committed changes.