Skip to content

Support Emscripten EH/SjLj in Wasm64 #14108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

Support Emscripten EH/SjLj in Wasm64 #14108

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0eca1c6
Support Emscripten EH/SjLj in Wasm64
aheejin May 6, 2021
abe0ce1
Address comments
aheejin May 12, 2021
de7ba72
Address comments
aheejin May 13, 2021
8b06b62
Fixes
aheejin May 13, 2021
06270c9
Merge branch 'main' into wasm64_emsjlj
aheejin May 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/library.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1527,6 +1527,7 @@ LibraryManager.library = {
longjmp__sig: 'vii',
#if SUPPORT_LONGJMP
longjmp: function(env, value) {
{{{ from64('env') }}};
Copy link
Collaborator

@sbc100 sbc100 May 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'; }

Copy link
Member Author

@aheejin aheejin May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

  1. Replace C longjmp call with emscripten_longjmp: Detail link
  2. 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?

Copy link
Collaborator

@sbc100 sbc100 May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@aheejin aheejin May 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_setThrew(env, value || 1);
throw 'longjmp';
},
Expand Down
4 changes: 2 additions & 2 deletions system/lib/compiler-rt/emscripten_exception_builtins.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@

#include <threads.h>

thread_local int __THREW__ = 0;
thread_local uintptr_t __THREW__ = 0;
thread_local int __threwValue = 0;

void setThrew(int threw, int value) {
void setThrew(uintptr_t threw, int value) {
if (__THREW__ == 0) {
__THREW__ = threw;
__threwValue = value;
Expand Down
7 changes: 4 additions & 3 deletions system/lib/compiler-rt/emscripten_setjmp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@
static uint32_t setjmpId = 0;

typedef struct TableEntry {
uint32_t id, label;
uintptr_t id;
Copy link
Collaborator

@sbc100 sbc100 May 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@aheejin aheejin May 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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.
  2. testSetjmp's first argument id is compared with the local variable curr. So it's uintptr_t now: Link
  3. That curr is set from table[i].id, which is TableEntry.id. So id in TableEntry here is now uintptr_t: Link
  4. Because table[i].id is set from setjmpId, setjmpId needs to be uintptr_t too: Link
  5. 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?

uint32_t label;
} TableEntry;

extern void setTempRet0(uint32_t value);
Expand Down Expand Up @@ -44,9 +45,9 @@ TableEntry* saveSetjmp(uint32_t* env, uint32_t label, TableEntry* table, uint32_
}

uint32_t testSetjmp(uint32_t id, TableEntry* table, uint32_t size) {
uint32_t i = 0, curr;
uint32_t i = 0;
while (i < size) {
uint32_t curr = table[i].id;
uintptr_t curr = table[i].id;
if (curr == 0) break;
if (curr == id) {
return table[i].label;
Expand Down