Rename Emscripten EHSjLj functions in wasm backend #3191

aheejin · 2020-10-01T20:37:28Z

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, 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

aheejin · 2020-10-01T20:39:25Z

test/lld/duplicate_imports.wat.out

+   (local.get $1)
+   (local.get $fptr)
+  )
+ )


These dynCalls are generated because the input file contains invoke_ functions. Before they were not targets for dynCall generations because EmscriptenGlueGenerator looked for the format generated by the LLVM backend, like __invoke_void_i32, and not invoke_vi.

sbc100 · 2020-10-01T22:46:25Z

src/passes/GenerateDynCalls.cpp

+  wasm->addFunction(f);
+  exportFunction(*wasm, f->name, true);
+}
+


How about making this its own PR that would be NFC? Seems like it might be nice intermediate step?

Done in #3192. Will rebase this PR on top of that. It's inconvenient that Github doesn't support dependent PRs... (We can emulate it if I used branches in this repo and not my own fork though..)

kripken · 2020-10-01T23:19:30Z

src/passes/GenerateDynCalls.cpp

      }
    }
  }

+  void visitFunction(Function* func) {
+    // Generate dynCalls for invokes
+    if (func->imported() && func->base.startsWith("invoke_")) {


please check also that func->module == ENV

aheejin · 2020-10-06T02:57:28Z

I incorporated invoke wrapper renaming we discussed in the meeting this morning here; I thought about doing it as a separate patch but it will take three more patches, each from LLVM, Binaryen, and Emscripten, and rolling them will be a pain, so... Also edited the PR description reflecting this.

This reverts commit c7b2e41.

aheejin · 2020-10-06T10:40:11Z

Sorry for changing things again; I reverted the invoke format changing part (two leading underscore thing). Renaming this turns out to change a lot of things in Emscripten side, which I think is really better be a separate patch.

sbc100

I'm curious, can the llvm change land before this on its own?

It seems like it should be possible .. it would just mean the fixInvokeFunctionNames would becoming inactive.. and then change to roll after?

sbc100 · 2020-10-06T19:21:02Z

src/passes/GenerateDynCalls.cpp

@@ -55,7 +55,8 @@ struct GenerateDynCalls : public WalkerPass<PostWalker<GenerateDynCalls>> {

  void visitFunction(Function* func) {
    // Generate dynCalls for invokes
-    if (func->imported() && func->base.startsWith("invoke_")) {
+    if (func->imported() && func->module == ENV &&
+        func->base.startsWith("invoke_")) {


This seems like maybe unneeded/unrelated?

If you do want to make this change you you should change the logic in src/wasm/wasm-emscripten.cpp which seems do duplicate this when generating invokeFuncs mabye?

It was suggested in #3191 (comment), and yeah, I think the condition in wasm/wasm-emscripten.cpp should be the same. Should I duplicate the condition there? @kripken

Added the same condition in wasm-emscripten.cpp.

aheejin · 2020-10-06T19:58:50Z

I'm curious, can the llvm change land before this on its own?

It seems like it should be possible .. it would just mean the fixInvokeFunctionNames would becoming inactive.. and then change to roll after?

Yeah I think llvm change can land on its own; maybe I was not clear when I was asking for the double roll thing.. What I meant was, Emscripten CI will pass only after both the LLVM and Binaryen patches land. But landing those two will break the roller because Emscripten patch hasn't landed.

This was going to be a part of emscripten-core#12399, but commiting this single line first will enable rolling of the corresponding LLVM (https://reviews.llvm.org/D88697) and Binaryen (WebAssembly/binaryen#3191) patches successfully without us needing to disable any Emscripten tests or do a double rolling.

This was going to be a part of #12399, but commiting this single line first will enable rolling of the corresponding LLVM (https://reviews.llvm.org/D88697) and Binaryen (WebAssembly/binaryen#3191) patches successfully without us needing to disable any Emscripten tests or do a special rolling.

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, 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

aheejin requested review from kripken and sbc100 October 1, 2020 20:37

aheejin commented Oct 1, 2020

View reviewed changes

aheejin mentioned this pull request Oct 1, 2020

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

Merged

Improve comment

1b856a4

sbc100 reviewed Oct 1, 2020

View reviewed changes

kripken approved these changes Oct 1, 2020

View reviewed changes

aheejin added 3 commits October 1, 2020 16:39

Add func->module == ENV

e5d781d

Merge branch 'master' into emeh

614bb2a

Revise invoke name format

c7b2e41

aheejin added 2 commits October 5, 2020 20:08

Merge branch 'master' into emeh

6d7ed8a

Revert "Revise invoke name format"

b5381c5

This reverts commit c7b2e41.

sbc100 approved these changes Oct 6, 2020

View reviewed changes

aheejin mentioned this pull request Oct 7, 2020

Add deps info for emscripten_longjmp emscripten-core/emscripten#12456

Merged

aheejin added 2 commits October 10, 2020 19:11

Merge branch 'master' into emeh

a3819d5

Add 'import->module == ENV' in wasm-emscripten.cpp

9dd5db3

aheejin merged commit f4d074f into WebAssembly:master Oct 11, 2020

aheejin deleted the emeh branch October 11, 2020 03:01

aheejin mentioned this pull request Oct 11, 2020

Rename __invoke_* functions in wasm backend? #3081

Closed

sbc100 mentioned this pull request Oct 20, 2021

parse_llvm_nm_symbols: when symbol name contains ":" character error occurs emscripten-core/emscripten#15325

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Rename Emscripten EHSjLj functions in wasm backend #3191

Rename Emscripten EHSjLj functions in wasm backend #3191

Uh oh!

aheejin commented Oct 1, 2020 •

edited

Loading

Uh oh!

aheejin Oct 1, 2020

Uh oh!

sbc100 Oct 1, 2020

Uh oh!

aheejin Oct 1, 2020

Uh oh!

kripken Oct 1, 2020

Uh oh!

aheejin Oct 1, 2020

Uh oh!

aheejin commented Oct 6, 2020 •

edited

Loading

Uh oh!

aheejin commented Oct 6, 2020 •

edited

Loading

Uh oh!

sbc100 left a comment

Uh oh!

sbc100 Oct 6, 2020

Uh oh!

aheejin Oct 6, 2020

Uh oh!

aheejin Oct 11, 2020

Uh oh!

aheejin commented Oct 6, 2020

Uh oh!

Uh oh!

Rename Emscripten EHSjLj functions in wasm backend #3191

Rename Emscripten EHSjLj functions in wasm backend #3191

Uh oh!

Conversation

aheejin commented Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aheejin commented Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aheejin commented Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aheejin commented Oct 6, 2020

Uh oh!

Uh oh!

aheejin commented Oct 1, 2020 •

edited

Loading

aheejin commented Oct 6, 2020 •

edited

Loading

aheejin commented Oct 6, 2020 •

edited

Loading