@sbc100 Friendly ping :-).

Address review feedback.

I apologize for the extraordinary delays here; at long last, I've now addressed your feedback.

To support a transition to the new system, temporarily define a __main_void alias for no-argument main. This allows libc to detect whether it's calling old-style or new-style main and do the right thing.

This updates the main vs __main_argc_argv patch so that it doesn't apply to Emscripten targets, following the discussion in https://github.com/WebAssembly/tool-conventions/pull/134.

In D70500#1759831, @ilya-biryukov wrote:

I'm not an expert in driver code and how it should behave, but being consistent with how other tools are found definitely looks much better than special-casing a single tool.
Unless there are reasons why wasm-opt should be special, why not use the mechanism used by all other tools?

I've done some more experimenting with this, and it turns out not to work very well, because calls to these functions get generated after LTO runs, so I'll abandon this.

I've now posted https://reviews.llvm.org/D70677 which should fix the test failure when LLVM_APPEND_VC_REV=NO is set.

In D70500#1757735, @thakis wrote:

Please don't add code to the driver that runs programs off PATH. Nothing else does this.

In D70500#1756011, @dschuff wrote:

WRT the LTO directory name, there's theoretically the danger that someone (e.g. emscripten) could be doing a rolling release of the compiler and get invalidated within a major revision.

Do you think that setting this attribute should also prevent GC?

It appears this doesn't handle exporting an imported function yet, which is fine for now, but it would be good to issue a warning, because wasm itself is capable of representing this:

void aaa(void) __attribute__((import_module("imp"), import_name("foo"), export_name("bar")));

Don't run wasm-opt with -O0.

Use PATH instead of WASM_OPT to find wasm-opt.

In D70515#1754288, @sbc100 wrote:

Does this need to be wasm global? Or can it just be regular data symbol like globalBase? i.e. do we want fast access to it without a load?

In D70500#1754012, @sbc100 wrote:

On I just remember why this is probably a bad idea. llvm bitcode is not designed to be stable, unlike object files, so its probably not a good idea to encourage the distributing of bitcode files in SDKs and such.

This kind of thing ought to have helped code like

extern int A[];

Would it be better to do this check in LLVM, in the backend, with a report_fatal_error?

In D66983#1651981, @craig.topper wrote:

DAG combine is supposed to check with TargetLowering::isShuffleMaskLegal.

In D66978#1652070, @tlively wrote:

Generally LGTM, although the tests might be more readable if you pass -wasm-disable-explicit-locals and -wasm-keep-registers as well. It's also concerning that no one noticed this issue before. Do you know why other targets may not have run into it?

• Use -wasm-disable-explicit-locals -wasm-keep-registers to make the test more readable

In D66983#1651977, @dschuff wrote:

Oh, interesting I didn't notice that those are implementations of the target-specific intrinsics. I wonder if they do that so they can implement the intrinsics on hardware that doesn't support the corresponding instruction? In a situation other than that I think I'd be surprised if I used a builtin for a particular instruction and got something else.

Can you say what leads wasm users to maintain such an expectation when, for example, ARM users and x86 users don't?

In D66968#1651483, @sbc100 wrote:

I'm curious why a user would want to use WASM_SYMBOL_NO_STRIP if it doesn't export the symbol? Maybe for post-linker to do stuff perhaps?

Include the basic test case.

Abandoning in favor of https://reviews.llvm.org/D66968.

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

"memory" is now a de-facto standard. We may still be able to change it, but we'll now need a transition plan.

I have a slight preference for landing this change too.

x86 uses address spaces starting at 256 and counting up for its architecture-specific address spaces. The docs say "Address spaces 1-255 are currently reserved for user-defined code." so we should consider using 256 here.

This allows for us to fall back from arch-specific to generic headers as needed. The same can be true of libraries. Not all libraries contains compiled code. .so files can also be linker scripts that reference other libraries in which case they can be arch-neutral.

My opinion isn't too strong here either. wasm32 doesn't need 8 bytes for this, and there's an existing ABI knob to use less, so it seems to make sense to use it.

This looks nice!

The actual compiler crash here came from the fact that we don't have an entry for llvm.clear_cache in the builtin function signature table. Suppose we added a signature for it. Then, we'd get LLVM's and compiler-rt's default behavior, which would be to call __clear_cache, and ultimately just abort at runtime.

Any code that thinks it needs to "clear the cache" is very likely doing something which won't actually work on wasm. Would it be reasonable to issue a report_fatal_error here?

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.