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 all commits
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
16 changes: 3 additions & 13 deletions src/library.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1524,13 +1524,9 @@ LibraryManager.library = {
// setjmp.h
// ==========================================================================

longjmp__sig: 'vii',
#if SUPPORT_LONGJMP
longjmp: function(env, value) {
_setThrew(env, value || 1);
throw 'longjmp';
},
#else
_emscripten_throw_longjmp__sig: 'v',
_emscripten_throw_longjmp: function() { throw 'longjmp'; },
#if !SUPPORT_LONGJMP
longjmp__deps: [function() {
error('longjmp support was disabled (SUPPORT_LONGJMP=0), but it is required by the code (either set SUPPORT_LONGJMP=1, or remove uses of it in the project)');
}],
Expand All @@ -1539,12 +1535,6 @@ LibraryManager.library = {
abort('longjmp not supported');
},
#endif
// TODO: remove these aliases if/when the LLVM backend can stop emitting them
// (it emits them atm as they are generated by an IR pass, at at that time
// they each have a different signature - it is only at the wasm level that
// they become identical).
emscripten_longjmp__sig: 'vii',
emscripten_longjmp: 'longjmp',

// ==========================================================================
// sys/wait.h
Expand Down
10 changes: 3 additions & 7 deletions src/library_signals.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,20 +138,16 @@ var funs = {
return -1;
},
#if SUPPORT_LONGJMP
#if ASSERTIONS
siglongjmp__deps: ['longjmp'],
Copy link
Member Author

@aheejin aheejin May 13, 2021

Choose a reason for hiding this comment

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

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.

siglongjmp: function(env, value) {
#if ASSERTIONS
// We cannot wrap the sigsetjmp, but I hope that
// in most cases siglongjmp will be called later.

// siglongjmp can be called very many times, so don't flood the stderr.
warnOnce("Calling longjmp() instead of siglongjmp()");
_longjmp(env, value);
},
#else
siglongjmp__sig: 'vii',
siglongjmp: 'longjmp',
Copy link
Member Author

@aheejin aheejin May 13, 2021

Choose a reason for hiding this comment

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

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 .

Copy link
Collaborator

@sbc100 sbc100 May 13, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@aheejin aheejin May 14, 2021

Choose a reason for hiding this comment

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

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

#endif
_emscripten_longjmp(env, value);
},
#endif

sigpending: function(set) {
Expand Down
5 changes: 3 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 @@ -9,12 +9,13 @@
* See: https://llvm.org/doxygen/WebAssemblyLowerEmscriptenEHSjLj_8cpp.html
*/

#include <stdint.h>
#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
23 changes: 17 additions & 6 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 @@ -9,15 +9,21 @@
#include <stdlib.h>
#include <setjmp.h>

static uint32_t setjmpId = 0;
// 0 - Nothing thrown
// 1 - Exception thrown
// Other values - jmpbuf pointer in the case that longjmp was thrown
static uintptr_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);
extern void setThrew(uintptr_t threw, int value);
extern void _emscripten_throw_longjmp(); // defined in src/library.js

TableEntry* saveSetjmp(uint32_t* env, uint32_t label, TableEntry* table, uint32_t size) {
TableEntry* saveSetjmp(uintptr_t* env, uint32_t label, TableEntry* table, uint32_t size) {
// Not particularly fast: slow table lookup of setjmpId to label. But setjmp
// prevents relooping anyhow, so slowness is to be expected. And typical case
// is 1 setjmp per invocation, or less.
Expand All @@ -43,10 +49,10 @@ TableEntry* saveSetjmp(uint32_t* env, uint32_t label, TableEntry* table, uint32_
return table;
}

uint32_t testSetjmp(uint32_t id, TableEntry* table, uint32_t size) {
uint32_t i = 0, curr;
uint32_t testSetjmp(uintptr_t id, TableEntry* table, uint32_t size) {
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 All @@ -55,3 +61,8 @@ uint32_t testSetjmp(uint32_t id, TableEntry* table, uint32_t size) {
}
return 0;
}

void emscripten_longjmp(uintptr_t env, int val) {
Copy link
Collaborator

@sbc100 sbc100 May 12, 2021

Choose a reason for hiding this comment

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

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

setThrew(env, val);
_emscripten_throw_longjmp();
}
3 changes: 1 addition & 2 deletions tools/deps_info.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,6 @@
'gmtime_r': ['malloc'],
'localtime': ['_get_tzname', '_get_daylight', '_get_timezone', 'malloc'],
'localtime_r': ['_get_tzname', '_get_daylight', '_get_timezone', 'malloc'],
'longjmp': ['setThrew'],
'mktime': ['_get_tzname', '_get_daylight', '_get_timezone', 'malloc'],
'mmap': ['memalign', 'memset', 'malloc'],
'munmap': ['malloc', 'free'],
Expand All @@ -181,7 +180,7 @@
'setjmp': ['saveSetjmp'],
'setprotoent': ['malloc'],
'setgroups': ['sysconf'],
'siglongjmp': ['setThrew'],
'siglongjmp': ['emscripten_longjmp', 'setThrew'],
'syslog': ['malloc', 'ntohs'],
'timegm': ['_get_tzname', '_get_daylight', '_get_timezone', 'malloc'],
'times': ['memset'],
Expand Down