Ping

Yay!

Please ignore this change, I had thought we wanted size_t to match either int32_t or int64_t, but it turns out that isn't a requirement and that code that expects this to be true needs fixing.

• add check for -r

Perhaps I should make this -z compress although I'm not sure what the policy is in using -z keywords vs regular flags.

• fix type mismatch
• add test

remove debugging

Hmm. I don't know the convention. Not a huge and of "The" prefix. LGTM otherwise.

I think you are right about the optimized builds. One thing I thought we might want to preserve is that data objects from the same section should be grouped together contiguously.. but I think we can probably do that while at the same time putting it all in a single segment.

In D40526#1192688, @sunfish wrote:

Is this related to the issue reported in the thread here?

Should we change int32_t as well? Or does the above code just need fixing? I'm a little concerned because this code is very portable which implies that WebAssembly might be doing something unusual here.

I'm running into an issue with an internal codebase that assumes that size_t is equivalent to either int32_t or int64_t which this change invalidates.

I agree this feels a bit awkward, but I prefer to custom bounds checking I think.

• update tests

• update test

I see. This seems a little strange to me. I'm not sure it makes sense for CLANG_DEFAULT_LINKER to effect targets like WebAssembly that only have single supported linker. I'm OK with landing this now but I think we might need to look into a better solution.

But in the CL description you say "..when configuring clang to use a different linker by default". How is this possible? i.e. do you have a config where these tests are currently failing?

• update test

Second attempt at this change since I think it makes sense. Otherwise emscripten would need to pass --export=foo and --undefined=foo for each symbol which seems redundant.

• --undefined should work with -r

• revert part

• Handle --undefined like the ELF linker

• Update comment

Ok, lets try that approach: https://reviews.llvm.org/D50279

Oh, I didn't realize that. In that case perhaps we need to bring --undefined in line with the ELF linker?

A bit more detail: Emscripten has flag called "-s EXPORTED_FUNCTIONS=<list>". By default this list includes just main. But those symbols can either come from native code or from JS libraries, so they might not exist and link time. Ideally we would know which ones we need at link time and with --export=foo and --undefined=foo which would force all these symbols to exist. But right now we don't have that knowledge so we use --allow-undefined with --export=foo, which roughly equates to "export these symbols if they are found at link time but don't error out if they are not".

Emscripten uses --export in order to choose which symbols to export. It does this for main too with --export=main. We can't use --undefined in general since (for complicated reasons) some of these symbols might actually be missing at link time.

ping..

rL338703 made this redundant

• update comment

PTAL

• remove logging

• revert

gererate runtime calls to abort rather than failing to compile
add more tests

How do you configure clang to use a different linker? It looks like getDefaultLinker() is hardcoded, but maybe I'm missing something.

https://reviews.llvm.org/D49825 seems to fix it.

FYI, this broke the WebAssembly waterfall, where the library is no longer being installed: https://wasm-stat.us/builders/linux/builds/34191

Interesting, so you are proposing delaying the error until link time but still producing a valid wasm binary?

• remove overload

I did consider that, but the COFF version is slightly different. Perhaps as a followup to keep this change strictly wasm-only?

• revert

• revert change to wasm addLazy

ping..

Do did enforcement of this rule increase recently? I wonder if they knew they would break existing code.

When you say not all types are valid to cast to all others.. how would I know which are valid? And how would this code fail if they are invalid? Would llvm bail out during the call to getCastOpcode()?

My goal here is not so much to make it "work" as to not produce invalid webassembly. i.e. it should compile, even it if fails at runtime to do anything useful/expected.

I'm still not sure I understand why writing zero here is not a good idea.

• rebase

• rebase

Adding @jgravelle-google. Maybe @ruiu is OOO. This change I pretty small an non-controversial I think, not that I've explained it. These two lines should have been in the original LTO change.

I think we can still do that library option, with this as an intermediate step. This change has the nice property of not requiring an emscripten-side change.

• feedback

• feedback

Anybody feeling LG'ing this?

• feedback

• cleanup

I agree it that the other approach is more elegant. However, I decided to go with this one for now since:
(1) Its a smaller incremental change
(2) Hopefully all this code will be removed pretty soon anyway

• cleanup

Abandoning in favor of weak linking approach: https://reviews.llvm.org/D49263