Reduce modifications done in wasm-emscripten-finalize #3043

kripken · 2020-08-14T22:42:15Z

Currently wasm-emscripten-finalize both writes out some metadata and modifies the wasm. If we can reduce the amount of modifications, we can eventually get to a place where there are no modifications, at least in some build types, which would be simpler and faster.

The larger goal here is to let emscripten do nothing after wasm-ld links the wasm, that is, not modify the wasm at all. Optimized builds will still want wasm-opt, of course, but it would be nice if unoptimized ones didn't need to.

wasm-emscripten-finalize renames EM_ASM calls to have the signature in the name. This isn't actually useful - emscripten doesn't benefit from that. I think it was optimized in fastcomp, and in upstream we copied the general form but not the optimizations, and then EM_JS came along which is easier to optimize anyhow. This PR makes those changes optional: when not doing them, it just leaves the calls as they are. Emscripten will need some changes to handle that, but those are simple. For convenience this adds a flag to "minimize wasm changes". The idea is that this flag avoids needing a double-roll or other inconvenience as the changes need to happen in tandem on the emscripten side. The same flag can be reused for later changes similar to this one. When they are all done we can remove the flag. (Note how the code ifdefed by the flag can be removed once we no longer need the old way of doing things - that is, the new approach is simpler on the binaryen side). See #3043

Emscripten counterpart to WebAssembly/binaryen#3044 , see background there, and WebAssembly/binaryen#3043 This basically gets rid of the special handling of EM_ASMs. Instead, they are just normal JS library methods like any other. (The one interesting thing in them, unchanged from before this PR, is that we need to read the varargs before doing any proxying, see the comment there for more details.)

The minimizeWasmChanges flag now does nothing (but new changes are coming, so keep it around) - this moves us to always doing the new way of things. With that we can update the tests. See #3043

…3043

LLVM adding __original_main can be confusing for users, as it shows up in stack traces, and they don't expect it. So we added a pass to inline that into main, and we ran it on all builds, debug and release. However, this has some downsides: * As a comment that this PR removes shows, we actually stopped doing this in debug builds because it can mess with DWARF. So the place we need it most is no longer relevant anyhow, and it wasn't actually helping us with debugging. * It is more work and complexity during link, which we are trying to reduce, see WebAssembly/binaryen#3043 * There are some planned changes to these functions and _start and all that anyhow (see discussion on the PR) so this will become irrelevant soon. This does not affect behavior, unless you look at the wasm internals. But a noticeable effect is that if you use ASYNCIFY_ADD or ASYNCIFY_ONLY then you may need to add __original_main to there (since you are doing manual fine-tuning of the list of functions, which depends on the wasm's internals) - see the test change here. Note that this should not matter in -O2+ anyhow as normal inlining generally removes __original_main.

Helps WebAssembly/binaryen#3043

That pass is a pure optimization: it removes calls to atexit when they would be ignored anyhow (EXIT_RUNTIME == 0). This removes a warning in atexit's implementation that was never actually called before: we used to always run that pass, so if EXIT_RUNTIME == 0 then we never had any calls to atexit anyhow. Now that we only run the pass when optimizing, leaving that warning would be a noticeable change (and it broke some tests actually!) so just remove it. With that, this is essentially NFC except that non-optimized builds may be a little larger (containing calls to atexit that end up doing nothing). Helps WebAssembly/binaryen#3043

In -O0 builds we want to leave the wasm from LLVM unmodified as much as possible (see WebAssembly/binaryen#3043), so this PR makes us stop modifying it to remove the producers section. There is no change to optimized builds (-O1+), so release builds are not changed in any way. There is also no change if the user told LLVM to not emit a producer's section in the first place. We basically just do less things on -O0 builds after this PR, and leave it to LLVM.

We used to always run wasm-opt's --strip-debug when -g was not specified, which would strip out DWARF as well as the Name section. This changes us to leave it alone. This has no effect on release builds (-O1+) and no effect on proper debug builds (-O0 -g), but does have an effect on unoptimized builds (-O0) without -g, which may now contain DWARF or the Name section now, depending on how clang and wasm-ld were invoked. Part of reducing unnecessary work after link, and aligning us more with what LLVM tools normally do, see WebAssembly/binaryen#3043

In fastcomp we implemented emscripten_get_sbrk_ptr in wasm, and exported _emscripten_get_sbrk_ptr. We don't need that anymore and can remove it. However I want to switch us to implementing emscripten_get_sbrk_ptr in wasm in upstream too, as part of removing DYNAMICTOP_PTR and other silliness that we have around link (#3043). This makes us support an export of emscripten_get_sbrk_ptr (no prefix), and also it makes sure not to instrument that function, which may contain some memory operations itself, but if we SAFE_HEAP-ify them we'd get infinite recursion, as the SAFE_HEAP methods need to call that.

Helps WebAssembly/binaryen#3043

Two flags here, one to completely remove dynCalls, and another to limit them to only signatures that contains i64. See #3043

kripken · 2020-09-03T22:54:49Z

Not running wasm-emscripten-finalize uncovered what looks like an LLVM bug with wasm exceptions,

https://bugs.llvm.org/show_bug.cgi?id=47413

But that doesn't need to block this, as an experimental feature I think we could stop running -fwasm-exceptions on test_exceptions_3 where it happens.

This reaches the first milestone of WebAssembly/binaryen#3043, to stop running wasm-opt in some -O0 builds (the next milestone is wasm-emscripten-finalize). To do so, this stops running --post-emscripten unless we optimize. This was the last pass we ran in -O0 builds, so it finally gets us into the code path of not running wasm-opt. That turned up a bug in the logic there - view that area without whitespace, it's a trivial change (we checked is passes is empty an extra time in an outer unnecessary scope). By running wasm-opt less this appears to have uncovered an LLVM bug, https://bugs.llvm.org/show_bug.cgi?id=47413 so part of one test is disabled.

The longer name appears from the wasm backend, and wasm-emscripten-finalize renames it. Instead, support both, and then we can remove the long name either in LLVM or in binaryen. See WebAssembly/binaryen#3043

Instead of finalize renaming emscripten_longjmp_jmpbuf to emscripten_longjmp, do nothing in finalize. But in the optional --post-emscripten pass, rename it there if both exist, so that we don't end up using two imports (other optimization passes can then remove an unneeded import). Depends on emscripten-core/emscripten#12157 to land first so that emscripten can handle both names, and it is just an optimization to have one or the other. See #3043

We no longer need or allow static allocations from JS. See WebAssembly/binaryen#3043

…CHANGES_AFTER_LINK option (#12173) emscripten.py now tracks whether we actually need to run wasm-emscripten-finalize. Currently we don't need to in a specific subset of -O0 builds, in which we don't need legalization (by using BigInt support) and we don't need invokes (which means we don't use longjmp or C++ exceptions; this restriction will be lifted once WebAssembly/binaryen#3081 is fixed). This new option will show an error if any binaryen work is done after wasm-ld. That is, it ensures that we do not run wasm-emscripten-finalize or wasm-opt. That is useful to verify that the link is as fast as possible and also that it does not rewrite DWARF, which is necessary to support split DWARF (binaryen can rewrite normal DWARF, but it's simpler and better to support split DWARF by just not doing any binaryen work after wasm-ld). See WebAssembly/binaryen#3043

Companions:

Now that we rename invoke wrappers and `emscripten_longjmp_jmpbuf` in the wasm backend, this deletes all related renaming routines and relevant tests. But we still need to generate dynCalls for invokes; for that this adds dynCall generations for invokes in `GenerateDynCalls` pass, and moves related functions from wasm-emscripten.cpp to GenerateDynCalls.cpp, given that now they are only used there. Addresses: WebAssembly#3043 and WebAssembly#3081 Companions:

Now that we rename invoke wrappers and `emscripten_longjmp_jmpbuf` in the wasm backend, this deletes all related renaming routines and relevant tests. But we still need to generate dynCalls for invokes; for that this adds dynCall generations for invokes in `GenerateDynCalls` pass, and moves related functions from wasm-emscripten.cpp to GenerateDynCalls.cpp, given that now they are only used there. Some lld tests now have new dynCalls in their results, which is expected, because they have Addresses: WebAssembly#3043 and WebAssembly#3081 Companions:

Now that we rename invoke wrappers and `emscripten_longjmp_jmpbuf` in the wasm backend, this deletes all related renaming routines and relevant tests. But we still need to generate dynCalls for invokes; for that this adds dynCall generations for invokes in `GenerateDynCalls` pass, and moves related functions from wasm-emscripten.cpp to GenerateDynCalls.cpp, given that now they are only used there. Addresses: WebAssembly#3043 and WebAssembly#3081

Now that we are renaming invoke wrappers and `emscripten_longjmp_jmpbuf` in the wasm backend, this deletes all related renaming routines and relevant tests. But we still need to generate dynCalls for invokes; for that this adds dynCall generations for invokes in `GenerateDynCalls` pass, and moves related functions from wasm-emscripten.cpp to GenerateDynCalls.cpp, given that now they are only used there. Addresses: WebAssembly#3043 and WebAssembly#3081

Now that we are renaming invoke wrappers and `emscripten_longjmp_jmpbuf` in the wasm backend, using Emscripten EH or SjLj does not need Binaryen's postprocessing. This makes Emscripten not enable Binaryen postprocessing using wasm-emscripten-finalize when we are using them, and deletes all special handling for `emscripten_longjmp_jmpbuf`, given that Emscripten will not see it. Addresses: WebAssembly/binaryen#3043 WebAssembly/binaryen#3081

Renaming for some Emscripten EH functions has so far been done in wasm-emscripten-finalize tool in Binaryen. But recently we decided to make a compilation/linking path that does not rely on wasm-emscripten-finalize for modifications, so here we move that functionality to LLVM. Invoke wrappers are generated in LowerEmscriptenEHSjLj pass, but final wasm types are not available in the IR pass, we need to rename them at the end of the pipeline. This patch also removes uses of `emscripten_longjmp_jmpbuf` in LowerEmscriptenEHSjLj pass, replacing that with `emscripten_longjmp`. `emscripten_longjmp_jmpbuf` is lowered to `emscripten_longjmp`, but previously we generated calls to `emscripten_longjmp_jmpbuf` in LowerEmscriptenEHSjLj pass because it takes `jmp_buf*` instead of `i32`. But we were able use `ptrtoint` to make it use `emscripten_longjmp` directly here. Addresses: WebAssembly/binaryen#3043 WebAssembly/binaryen#3081 Companions: WebAssembly/binaryen#3191 emscripten-core/emscripten#12399 Reviewed By: dschuff, tlively, sbc100 Differential Revision: https://reviews.llvm.org/D88697

Now that we are renaming invoke wrappers and `emscripten_longjmp_jmpbuf` in the wasm backend, this deletes all related renaming routines and relevant tests. Depends on #3192. Addresses: #3043 and #3081 Companions: https://reviews.llvm.org/D88697 emscripten-core/emscripten#12399

Now that we are renaming invoke wrappers and `emscripten_longjmp_jmpbuf` in the wasm backend, using Emscripten EH or SjLj does not need Binaryen's postprocessing. This makes Emscripten not enable Binaryen postprocessing using wasm-emscripten-finalize when we are using them, and deletes all special handling for `emscripten_longjmp_jmpbuf`, given that Emscripten will not see it. Addresses: WebAssembly/binaryen#3043 WebAssembly/binaryen#3081 Companions: https://reviews.llvm.org/D88697 WebAssembly/binaryen#3191

Renaming for some Emscripten EH functions has so far been done in wasm-emscripten-finalize tool in Binaryen. But recently we decided to make a compilation/linking path that does not rely on wasm-emscripten-finalize for modifications, so here we move that functionality to LLVM. Invoke wrappers are generated in LowerEmscriptenEHSjLj pass, but final wasm types are not available in the IR pass, we need to rename them at the end of the pipeline. This patch also removes uses of `emscripten_longjmp_jmpbuf` in LowerEmscriptenEHSjLj pass, replacing that with `emscripten_longjmp`. `emscripten_longjmp_jmpbuf` is lowered to `emscripten_longjmp`, but previously we generated calls to `emscripten_longjmp_jmpbuf` in LowerEmscriptenEHSjLj pass because it takes `jmp_buf*` instead of `i32`. But we were able use `ptrtoint` to make it use `emscripten_longjmp` directly here. Addresses: WebAssembly/binaryen#3043 WebAssembly/binaryen#3081 Companions: WebAssembly/binaryen#3191 emscripten-core/emscripten#12399 Reviewed By: dschuff, tlively, sbc100 Differential Revision: https://reviews.llvm.org/D88697

kripken mentioned this issue Aug 14, 2020

wasm-emscripten-finalize: Make EM_ASM modifications optional #3044

Merged

kripken mentioned this issue Aug 20, 2020

Simplify EM_ASM handling emscripten-core/emscripten#11972

Merged

kripken mentioned this issue Aug 21, 2020

Remove old EM_ASM handling method #3069

Merged

dschuff mentioned this issue Aug 21, 2020

Stop running --inline-main emscripten-core/emscripten#11995

Merged

kripken added a commit to emscripten-core/emscripten that referenced this issue Aug 22, 2020

Run --no-exit-runtime only when optimizing. See WebAssembly/binaryen#…

8b3616e

…3043

This was referenced Aug 22, 2020

Only strip the producers' section in optimized builds emscripten-core/emscripten#11996

Merged

Only run --no-exit-runtime when optimizing emscripten-core/emscripten#11998

Merged

Only run --low-memory-unused when optimizing emscripten-core/emscripten#11999

Merged

kripken added a commit to emscripten-core/emscripten that referenced this issue Aug 23, 2020

Only run --low-memory-unused when optimizing (#11999)

cdf99a4

Helps WebAssembly/binaryen#3043

kripken mentioned this issue Aug 23, 2020

Don't strip debug info in unoptimized builds emscripten-core/emscripten#12016

Merged

kripken added a commit to emscripten-core/emscripten that referenced this issue Aug 25, 2020

Remove a static allocation from Fetch JS code (#12040)

9712c09

Helps WebAssembly/binaryen#3043

sbc100 mentioned this issue Aug 26, 2020

wasm-emscripten-finalize: Add flags to limit dynCall creation #3070

Merged

sbc100 added a commit that referenced this issue Aug 26, 2020

wasm-emscripten-finalize: Add flags to limit dynCall creations

2fde47b

Two flags here, one to completely remove dynCalls, and another to limit them to only signatures that contains i64. See #3043

sbc100 added a commit that referenced this issue Aug 26, 2020

wasm-emscripten-finalize: Add flags to limit dynCall creations

5d3a966

Two flags here, one to completely remove dynCalls, and another to limit them to only signatures that contains i64. See #3043

sbc100 mentioned this issue Aug 26, 2020

Remove the need for auto-generated dynCall_xx wasm functions emscripten-core/emscripten#12002

Closed

kripken mentioned this issue Sep 4, 2020

Stop running wasm-opt at all in some -O0 builds emscripten-core/emscripten#12108

Merged

This was referenced Sep 9, 2020

[discussion] JIT dynCall functions emscripten-core/emscripten#12141

Draft

Handle both emscripten_longjmp and emscripten_longjmp_jmpbuf emscripten-core/emscripten#12157

Merged

This was referenced Sep 11, 2020

Run wasm-emscripten-finalize only when needed, and add ERROR_ON_WASM_CHANGES_AFTER_LINK option emscripten-core/emscripten#12173

Merged

Remove makeStaticAlloc/String emscripten-core/emscripten#12174

Merged

kripken added a commit to emscripten-core/emscripten that referenced this issue Sep 11, 2020

Remove makeStaticAlloc/String (#12174)

79da9f9

We no longer need or allow static allocations from JS. See WebAssembly/binaryen#3043

aheejin added a commit to aheejin/binaryen that referenced this issue Oct 1, 2020

Addresses: WebAssembly#3043 and WebAssembly#3081

220b2bc

Companions:

This was referenced Oct 1, 2020

Rename Emscripten EHSjLj functions in wasm backend #3191

Merged

Don't run binaryen postprocessing for Emscripten EH/SjLj emscripten-core/emscripten#12399

Merged

allsey87 mentioned this issue Apr 11, 2022

Disable import/export name mangling in -O3 builds emscripten-core/emscripten#16695

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce modifications done in wasm-emscripten-finalize #3043

Reduce modifications done in wasm-emscripten-finalize #3043

kripken commented Aug 14, 2020 •

edited

Loading

kripken commented Sep 3, 2020

Reduce modifications done in wasm-emscripten-finalize #3043

Reduce modifications done in wasm-emscripten-finalize #3043

Comments

kripken commented Aug 14, 2020 • edited Loading

kripken commented Sep 3, 2020

kripken commented Aug 14, 2020 •

edited

Loading