Support Emscripten EH/SjLj in Wasm64#14357
Merged
Merged
Conversation
This mostly changes some `uint32_t`s to `uintptr_t` in libraries for EH and SjLj to support wasm64. More details in case you are interested: 1. 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](https://github.com/emscripten-core/emscripten/blob/aa2bad5d6a0de5b175688fc8ead46f70a1b43a0d/system/lib/compiler-rt/emscripten_setjmp.c#L53) 3. That `curr` is set from `table[i].id`, which is `TableEntry.id`. So `id` in `TableEntry` here is now `uintptr_t`: [Link](https://github.com/emscripten-core/emscripten/blob/aa2bad5d6a0de5b175688fc8ead46f70a1b43a0d/system/lib/compiler-rt/emscripten_setjmp.c#L51) 4. Because `table[i].id` is set from `setjmpId`, `setjmpId` needs to be `uintptr_t` too: [Link](https://github.com/emscripten-core/emscripten/blob/aa2bad5d6a0de5b175688fc8ead46f70a1b43a0d/system/lib/compiler-rt/emscripten_setjmp.c#L31) 5. Because `setjmpId` is stored in `env` (first parameter) in `saveSetjmp`, `saveSetjmp`'s first argument has to change to `uintptr_t`: [Link](https://github.com/emscripten-core/emscripten/blob/aa2bad5d6a0de5b175688fc8ead46f70a1b43a0d/system/lib/compiler-rt/emscripten_setjmp.c#L28) Not sure how to add tests for this given that wasm64 support is not ready yet. Existing tests run fine.
Member
Author
|
This is the second half of #14108. |
sbc100
approved these changes
Jun 3, 2021
Collaborator
|
nice, thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This mostly changes some
uint32_ts touintptr_tin libraries for EHand SjLj to support wasm64.
More details in case you are interested:
first argument of
emscripten_longjmpin turn changedtestSetjmp'sfirst argument to
uintptr_t.testSetjmp's first argumentidis compared with the localvariable
curr. So it'suintptr_tnow:Link
curris set fromtable[i].id, which isTableEntry.id. SoidinTableEntryhere is nowuintptr_t:Link
table[i].idis set fromsetjmpId,setjmpIdneeds to beuintptr_ttoo:Link
setjmpIdis stored inenv(first parameter) insaveSetjmp,saveSetjmp's first argument has to change touintptr_t:Link
Not sure how to add tests for this given that wasm64 support is not
ready yet. Existing tests run fine.