Support Emscripten EH/SjLj in Wasm64 #14108

aheejin · 2021-05-06T10:44:47Z

This reflects changes in the function signature in
https://reviews.llvm.org/D101985 and also uses from64 function defined
in #12869. This has not been tested against wasm64 yet and I'm uploading
this mainly for discussions at this point.

Also in the team chat of the tools team we discussed other alternatives
to from64 function, which can be done separately and not reflected
here.

This reflects changes in the function signature in https://reviews.llvm.org/D101985 and also uses `from64` function defined in emscripten-core#12869. This has not been tested against wasm64 yet and I'm uploading this mainly for discussions at this point. Also in the team chat of the tools team we discussed other alternatives to `from64` function, which can be done separately and not reflected here.

sbc100 · 2021-05-06T14:32:20Z

src/library.js

@@ -1527,6 +1527,7 @@ LibraryManager.library = {
  longjmp__sig: 'vii',
 #if SUPPORT_LONGJMP
  longjmp: function(env, value) {
+    {{{ from64('env') }}};


Looking at this I think we can maybe avoid this change by moving longjmp itself into have code and then renaming this functions to just be _emscripten_throw_longjmp:

_emscripten_throw_longjump: function() { throw 'longjmp'; }

That sounds really good if possible. But not sure how to resolve this case.

There are two cases we generate a call to emscripten_longjmp in WebAssemblyLowerEmscriptenEHSjLj pass in LLVM;

Replace C longjmp call with emscripten_longjmp: Detail link

Transform every call that might longjmp into some complex code sequence, when emscripten_longjmp is called somewhere in the middle of it: Detail link

The second case is fine, because we know the two parameters to emscripten_longjmp while generating the call. The first case is tricky, because existing longjmp calls can be indirect calls, in which case we don't know those two arguments to longjmp are. So in that case we just cast the value to the type of emscripten_longjmp: (i32, i32). Because we don't know the two parameters, we can't generate setThrew and emscripten_throw_longjmp sequence there. So with this I think it will be hard to remove emscripten_longjmp function. WDYT?

I wasn't suggesting changing anything about codegen. Only that we move this function from js code the c code. It shouldn't be a change the makes any difference to the calling code.

Oh I misunderstood what you said, sorry. Yeah, will do.

system/lib/compiler-rt/emscripten_exception_builtins.c

sbc100 · 2021-05-06T14:42:04Z

system/lib/compiler-rt/emscripten_setjmp.c

@@ -12,7 +12,8 @@
 static uint32_t setjmpId = 0;

 typedef struct TableEntry {
-  uint32_t id, label;
+  uintptr_t id;


It looks like this is an auto-incrementing counter.. set from the uint32_t setjmpId above. If so why does it need to change type? At least I would expect setjmpId to be of the same type.

It's tricky... It looks I need to turn even more variables into uintptr_t.

So I described in https://reviews.llvm.org/D101985 how changing the first argument of emscripten_longjmp in turn changed testSetjmp's first argument to uintptr_t.

testSetjmp's first argument id is compared with the local variable curr. So it's uintptr_t now: Link

That curr is set from table[i].id, which is TableEntry.id. So id in TableEntry here is now uintptr_t: Link

Because table[i].id is set from setjmpId, setjmpId needs to be uintptr_t too: Link

Because setjmpId is stored in env (first parameter) in saveSetjmp, saveSetjmp's first argument has to change to uintptr_t: Link

5 was missing in this PR so I added it too. Not sure if this propagation of uintptr_t is good... WDYT?

sbc100

You can also remove the 'emscripten_longjmp': ['setThrew'], line from deps_info.py now I think!

And maybe also 'longjmp': ['setThrew'] ... not sure..

system/lib/compiler-rt/emscripten_setjmp.c

src/library.js

sbc100 · 2021-05-12T16:07:34Z

system/lib/compiler-rt/emscripten_setjmp.c

@@ -56,3 +59,8 @@ uint32_t testSetjmp(uint32_t id, TableEntry* table, uint32_t size) {
  }
  return 0;
 }
+
+void emscripten_longjmp(uintptr_t env, int val) {


Cool... I tried this but I got some odd crashed in the compiler.. but if this works that is great!

aheejin

Thanks for all the comments!

The previous version didn't pass testing; now this passes all existing tests. I don't have a full grasp on all interactions between JS/C dependences, so this PR is somehow the result of trail and error until I find the combination that works..

Couldn't test for wasm64 yet because its support has not landed yet, but I think there will not likely to be any blockers for wasm64 after this.

You can also remove the 'emscripten_longjmp': ['setThrew'], line from deps_info.py now I think!

I couldn't. So the LLVM-generated code has emscripten_longjmp as a undefined symbol, but without this line, add_reverse_deps does not add setThrew to settings.EXPORTED_FUNCTIONS. I'm slightly confused because the comment in deps_info.py says that file is for JS->C dependences, but I still need this line even though now both emscripten_longjmp and setThrew are in C. Do you think this is how it's supposed to work?

And maybe also 'longjmp': ['setThrew'] ... not sure..

I was able to remove it because I removed longjmp anyway from src/library.js. I wonder why we were keeping two different versions (longjmp and emscripten_longjmp) in JS before..? Is it safe to remove now? It doesn't seem to affect tests though.

Edit: I thought all tests were passing but test_longjmp_standalone is failing. Investigating now. It fails with this message:

/usr/bin/ld: /tmp/test_longjmp-91065b.o: in function `w2c_f14':
test_longjmp.wasm.c:(.text+0x837e): undefined reference to `Z_envZ__emscripten_throw_longjmpZ_vv'

Do you have any idea why?

aheejin · 2021-05-13T07:13:27Z

src/library_signals.js

@@ -138,20 +138,16 @@ var funs = {
    return -1;
  },
 #if SUPPORT_LONGJMP
-#if ASSERTIONS
-  siglongjmp__deps: ['longjmp'],


What is this __deps and how is this different from entries in deps_info.py? I deleted this because tests run correctly without this, but not sure what this is for.

aheejin · 2021-05-13T07:16:54Z

src/library_signals.js

-  },
-#else
-  siglongjmp__sig: 'vii',
-  siglongjmp: 'longjmp',


I first tried to change this to

siglongjmp: '_emscripten_longjmp'

But then wasm0.test_longjmp succeeds but wasm1.test_longjmp fails saying "no alias for siglongjmp[here](https://github.com/emscripten-core/emscripten/blob/ad438dfba750622d124f2de5b1154a374aeeaec4/src/modules.js#L270). Not really sure what's going on and whywasm0.test_longjmpworks whilewasm1doesn't, but all tests work if I moveif ASSERTIONS` into the function .

You should probable move this alias into native code alongsize longjmp.

If its really just an alias you can use weak_alias in the C source code, but it looks like the signature is different so you might need to write a one-line wrapper here.

That sounds better. I'll split this PR into two as you suggested and do it there.

sbc100 · 2021-05-13T14:28:33Z

I wonder if its worth landing the "move longjmp to native code" change first.. before the wasm64 change.. since they seem logically seperate?

aheejin · 2021-05-14T09:02:43Z

Thanks for feedback! Yeah, I'll split this PR into two smaller ones. Closing this one.

aheejin · 2021-05-14T09:57:13Z

The "move longjmp to native code" part is in #14188.

In wasm64, the signatures of some library functions and global variables defined in Emscripten change: - `emscripten_longjmp`: `(i32, i32) -> ()` -> `(i64, i32) -> ()` This changes because the first argument is the address of a memory buffer. This in turn causes more changes below. - `setThrew`: `(i32, i32) -> ()` -> `(i64, i32) -> ()` `emscripten_longjmp` calls `setThrew` with the i64 buffer argument as the first parameter. - `__THREW__` (global var): `i32` to `i64` `setThrew`'s first argument is set to this `__THREW__` variable, so it should change to i64 as well. - `testSetjmp`: `(i32, i32*, i32) -> (i32)` -> `(i64, i32*, i32) -> (i32)` In the code transformation done in this pass, the value of `__THREW__` is passed as the first parameter of `testSetjmp`. This patch creates some helper functions to easily get types that become different depending on the wasm32/wasm64, and uses them to change various function signatures and code transformations. Also updates the tests with WASM32/WASM64 check lines. (Untested) Emscripten side patch: emscripten-core/emscripten#14108 Reviewed By: aardappel Differential Revision: https://reviews.llvm.org/D101985

aheejin requested review from sbc100 and aardappel May 6, 2021 10:44

sbc100 reviewed May 6, 2021

View reviewed changes

aardappel approved these changes May 6, 2021

View reviewed changes

Address comments

abe0ce1

sbc100 reviewed May 12, 2021

View reviewed changes

aheejin added 3 commits May 12, 2021 19:04

Address comments

de7ba72

Fixes

8b06b62

Merge branch 'main' into wasm64_emsjlj

06270c9

aheejin marked this pull request as ready for review May 13, 2021 07:11

aheejin commented May 13, 2021

View reviewed changes

sbc100 approved these changes May 13, 2021

View reviewed changes

aheejin closed this May 14, 2021

aheejin mentioned this pull request May 14, 2021

Move longjmp into native code #14188

Merged

aheejin deleted the wasm64_emsjlj branch May 14, 2021 09:56

aheejin mentioned this pull request Jun 3, 2021

Support Emscripten EH/SjLj in Wasm64 #14357

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Emscripten EH/SjLj in Wasm64 #14108

Support Emscripten EH/SjLj in Wasm64 #14108

aheejin commented May 6, 2021

sbc100 May 6, 2021

aheejin May 11, 2021

sbc100 May 11, 2021

aheejin May 12, 2021

sbc100 May 6, 2021

aheejin May 12, 2021

sbc100 left a comment

sbc100 May 12, 2021

aheejin left a comment •

edited

Loading

aheejin May 13, 2021

aheejin May 13, 2021

sbc100 May 13, 2021

aheejin May 14, 2021

sbc100 commented May 13, 2021

aheejin commented May 14, 2021

aheejin commented May 14, 2021

Support Emscripten EH/SjLj in Wasm64 #14108

Support Emscripten EH/SjLj in Wasm64 #14108

Conversation

aheejin commented May 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aheejin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 commented May 13, 2021

aheejin commented May 14, 2021

aheejin commented May 14, 2021

aheejin left a comment •

edited

Loading