Fix EM_ASM not working with setjmp/longjmp#2271
Conversation
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Thanks!
lgtm with one minor improvement suggestion, but @jgravelle-google knows this code the best so let's wait for review there.
aheejin
left a comment
aheejin
left a comment
There was a problem hiding this comment.
Didn't finish reading the whole thing yet, but just some passing nits
jgravelle-google
left a comment
jgravelle-google
left a comment
There was a problem hiding this comment.
Overall approach looks good
| // is the result of adding its offset to __memory_base. | ||
| // In this case are only looking for the offset with the data segment so | ||
| // the RHS of the addition is just what we want. | ||
| assert(value->op == AddInt32); |
There was a problem hiding this comment.
We're assuming things about the nature of the generated code. Is there more we can assert about the LHS here?
There was a problem hiding this comment.
This is the way the code was originally written. I think it's better to have more assertions, but I don't think it's in scope for this PR.
There was a problem hiding this comment.
Ah right, method extraction, carry on
|
It looks like this has broken on CI, on emscripten tests: https://chromium-review.googlesource.com/c/emscripten-releases/+/1729837 Note the poppler error, and also there may be another test that infinite loops since the infra hangs. |
|
Confirmed locally that I see the same behavior as the bots. |
4 participants
Refs emscripten-core/emscripten#8894. This fix does not handle dynamic linking, which requires additional work.