-
Notifications
You must be signed in to change notification settings - Fork 165
Conversation
the remaining unhandled piece of work here is turning on macos CI, which is, I think, straightforward? I just haven't looked at it yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't test this myself, but the changes here look good to me!
da48da4
to
be53c48
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice work! Just a few cleanup requests
8963987
to
781fd9d
Compare
This branch has picked up a few non-signal-handling-related changes now, so given a +1 I'll fix up the commit history here into the ~three commits I'd like to land ("change signal handling around", "fix C API signal stack sizes to the largest minimum we know we have to work with -- 32kb on macos", and "enable mac CI") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting Mac CI working again!
d2088fb
to
acfe0a2
Compare
// pointing the instruction pointer at `lucet_context_set`, then by preparing the argument | ||
// that `lucet_context_set` should read from `rdi` - the context to switch to. | ||
// | ||
// NOTE: it is absolutely critical that `lucet_context_set` does not use the guest stack! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
full disclosure: I left most of the branch unchanged while adjusting the commits, but this NOTE:
is new. I'd realized that there wasn't anything discussing the specific constraints on the function we continue into after handling a signal, and this addresses that.
This is in contrast to calling `Context::set_from_signal` directly, which would skip over typical signal handling teardown some platforms might require. pthread_sigmask are removed here as they only served to reset signal masks that Linux would apply when entering the signal handler. Now that we return normally from the signal handler, on Linux signals are unmasked normally.
* conditionally define signals for invalid memory accesses per platform * add old macos module for version testing binary blob for the module is probably similar to the linux `old_module.so`, for which the source is not known. the macos `old_module.dylib` is built from `fibonacci.wat` as existed in Lucet commit `8639967` with the following command: ``` $ cargo run --bin lucetc ./lucetc/tests/wasm/fibonacci.wat ``` * make signal stacks sufficiently large in C API tests that mac CI passes
also involves fixing some wasi-sdk installation bitrot
acfe0a2
to
78aa0ed
Compare
Promise I'm done fiddling with this branch now, it's ready for a final +1! 🤞 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ship it!
changes have been made, and @pchickey has since approved
This mostly gets things in place for the Lucet signal handler to work properly on non-linux posix-y platforms, most interestingly: macos and flavors of bsd.
The gist of this change is to not context swap in the signal handler itself, but replace the address the guest continues with by the context swap function. So the signal handler can return on all platforms, passing through
sigreturn
-like mechanisms if present, and we have fewer cross-platform compatibility issues.