wasmtime: Refactor trap-handling #9087
Merged
Conversation
This file contains 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
This commit groups together the registers that have to be collected from a signal handler to correctly report a trap: namely, the program counter and frame pointer, as of the time that the trap occurred. I also moved the call to set_jit_trap inside test_if_trap for every platform that uses both methods. Only the implementation for Mach ports still needs to call set_jit_trap because it doesn't use test_if_trap. In addition I'm fixing an unrelated doc comment that I stumbled across while working on this.
github-actions
bot
added
the
wasmtime:api
elliottt
approved these changes
Aug 7, 2024
| @@ -496,17 +501,16 @@ impl CallThreadState { | |||
| #[cfg_attr(miri, allow(dead_code))] // miri doesn't handle traps yet | |||
| pub(crate) fn set_jit_trap( | |||
| &self, | |||
| pc: *const u8, | |||
| fp: usize, | |||
| TrapRegisters { pc, fp, .. }: TrapRegisters, | |||
There was a problem hiding this comment.
What do you think about removing the .. to require this match to be re-examined if the TrapRegisters type changes?
There was a problem hiding this comment.
I absolutely agree it's worth thinking about. My next PR specifically will add a field to this struct 😆 but this function won't need to look at it. I think it actually makes sense when pattern matching in this particular case to declare "I only care about these registers" even if more get collected later: these two are the ones we need for a backtrace or coredump.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 commit groups together the registers that have to be collected from a signal handler to correctly report a trap: namely, the program counter and frame pointer, as of the time that the trap occurred.
I also moved the call to set_jit_trap inside test_if_trap for every platform that uses both methods. Only the implementation for Mach ports still needs to call set_jit_trap because it doesn't use test_if_trap.
In addition I'm fixing an unrelated doc comment that I stumbled across while working on this.