LGTM!

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

In D62542#1529441, @sbc100 wrote:

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.

The wasi-libc PR is here.

• Rebase
• Rename IMPLICITLY_USED to NO_STRIP

• Rename IMPLICITLY_USED to NO_STRIP.
• Remove isExported and setExported accessors.

In D62443#1525418, @sbc100 wrote:

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.

Cool, this is nicer than my version :-).

In D62443#1520146, @sbc100 wrote:

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

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

In D62443#1517831, @carlokok wrote:

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).

If I follow the discussion correctly, there are subtle ABI issues either way, so making fence a RMW now doesn't give us fully a future-proof ABI anyway. Consequently, I would prefer to make this patch conditional on the Emscripten triple, as wasm32-wasi and wasm32-unknown-unknown don't require Emscripten compatibility.

LGTM

A function like this might be implemented in compiler_rt/libgcc or libc, which might in make future WASI system calls or future wasm language features underneath. An interface like __wasm_return_address doesn't commit to a particular implementation. I'm not suggesting enabling it for other targets now, just asking if it makes sense to pick a name which isn't tied to Emscripten.

Would it make sense to take this opportunity to rename the library function, from emscripten_return_address to something more general, like __wasm_return_address or __wasm_callsite_identifier or something, so that non-Emscripten environments could implement it too if they wished?

I don't know the lld internals well, but the basic approach here sounds right.

If "$sysroot/lib" ends up coming to mean "wasm32" because people come to depend on that, then wasm64 may end up needing to be different in a gratuitous way, which I'd like to avoid.

The value of supporting single-arch sysroots is unclear to me. It's always possible to have a sysroot with libraries for just one architecture installed, even with multi-arch paths. Is this just about compatibility with build scripts and tools which are hard-coded to "$sysroot/lib"?

If libraries don't correctly install into multi-arch directories, can we fix the libraries? We do this in the wasi-sdk repo to fix up the libc++ and libc++abi libraries, for example.

Cool :)

Drop the getentropy header file change from this patch.

Fixed in r359605.

• Convert the test input to yaml.
• Add ObjectYAML support for EXPLICIT_NAME symbols too.

Implemented proper diagnostics for import_name/import_module on functions with definitions, and updated the test.

I made the error message include the name of the function, and added a test.

The theory here is that br_table represents the performance characteristics of a jump table, while br_if represents the performance characteristics of a branch. In hardware, a small switch with 3 cases is often more efficient with a couple of conditional branches than a jump table.

In D59520#1434854, @aaron.ballman wrote:

Removes errnoneous use of diag::err_alias_is_definition, which turned out to be ineffective anyway since functions can be defined later in the translation unit and avoid detection.

If you need to keep the prohibition that these attributes not be applied to functions with definitions, there are ways to accomplish that (we merge declarations and can decide what to do at that point if we see a declaration that is a definition). Given that, do you still want to lift this restriction?

In D59281#1429332, @sbc100 wrote:

In D59281#1429109, @sunfish wrote:

In the future, atomics will likely be enabled by default, however it will continue to be desirable to be able to select non-shared memory, since non-shared memory doesn't require a maximum size, and since it can make interoperating with JS and other external code simpler. Is it possible to structure this patch in a way that supports this?

When building for non-shared memory, shouldn't input code check for "ifdef _REENTRANT" and not generate any atomic instructions?

In the future, atomics will likely be enabled by default, however it will continue to be desirable to be able to select non-shared memory, since non-shared memory doesn't require a maximum size, and since it can make interoperating with JS and other external code simpler. Is it possible to structure this patch in a way that supports this?

I've now made an updated post on https://github.com/WebAssembly/tool-conventions/issues/59.

Wasm gives users reasons to want -mthread-model single that other architectures don't, even when -matomics is enabled by default.

This is still a little confusing to me. -matomic is supposed to be a subtarget flag, stating that the wasm implementation we will run on supports atomic instructions. -mthread-model posix is about the C++ interpretation -- what style implementation of memory model do we want? In the future, -matomic may become enabled by default, when enough wasm engines generally support it. However, -mthread-model single/posix may still be useful to control independently, because even with wasm engines supporting atomic, there are reasons users might still want to compile their apps single-threaded: access to linear memory with no declared max, lower overall code size, or other things.

• -matomics means -mthread-model posix

That sounds reasonable to me too.

In D57632#1384200, @sbc100 wrote:

Looking good.

Can we can a test in the MC/WebAssembly and lld/test/wasm?

• Add MC/WebAssembly and lld tests
• Set the WASM_SYMBOL_NAME_EXPLICIT flag when a symbols name differs from its import name.

• Rename printf_no_Lf to __small_printf.
• Add more tests.
• Split out enabling the iprintf optimization for WASI, to be submitted in a separate patch later.

• Use a symbol flag to indicate when a custom symbol name is present.
• Revert the version number bump.
• Allow globals and events to have custom symbol names too, for consistency.

In D57602#1381220, @sbc100 wrote:

Do you could also achieve this I guess by renaming pwsix.read function itself to pwsix.__read? But I guess it makes sense to allow the use to control all 3 parts of the name separately.

I think WebAssembly is in the same situation as most other architectures, as discussed here:

In D57323#1374682, @sbc100 wrote:

• Remove the target constraint, allowing the main rewrite logic to work on all wasm32 targets.
• Remove the now-unneeded "wasm-temporary-workarounds" flag altogether.
• This uncovered some bugs with wrappers for main when main is only a declaration; fix those as well.

In D57323#1374572, @sbc100 wrote:

I mean with your fix we don't need this to be wasi specific do we?

In D57323#1374325, @sbc100 wrote:

I tried to enable this by default recently but had to revert: https://reviews.llvm.org/D54117.

I'm fine for this to land in the mean time but you might want to be aware of this issue.

LGTM.

Sorry for the delay here. Yes, let's land this.

In D56553#1356720, @sbc100 wrote:

Can you make change to llvm and clang at the same time? I thought that was not possible..