Stream: git-wasmtime

Topic: wasmtime / issue #6011 Add a `MachBuffer::defer_trap` method

Wasmtime GitHub notifications bot (Mar 14 2023 at 03:58):

alexcrichton commented on issue #6011:

@uweigand If you're able I'd like to get your review on the last commit of this PR, https://github.com/bytecodealliance/wasmtime/pull/6011/commits/0fd035c33f80e856671e203baf1cb564fcd7803c, where I updated how signal handling works on s390x. Trap emission is now buried within MachBuffer so while I could plumb through a flag for "register the trap code on the last byte of the opcode vs the first" I thought it might be easier to update the s390x-specific logic in the signal handler instead to avoid the s390x-specific-ness from spilling over into the mostly-independent MachBuffer. You probably know much more about the intricacies here, though, and I'd like to confirm that things should work as I expect/hope.

Wasmtime GitHub notifications bot (Mar 14 2023 at 18:18):

uweigand commented on issue #6011:

@uweigand If you're able I'd like to get your review on the last commit of this PR, 0fd035c, where I updated how signal handling works on s390x. Trap emission is now buried within MachBuffer so while I could plumb through a flag for "register the trap code on the last byte of the opcode vs the first" I thought it might be easier to update the s390x-specific logic in the signal handler instead to avoid the s390x-specific-ness from spilling over into the mostly-independent MachBuffer. You probably know much more about the intricacies here, though, and I'd like to confirm that things should work as I expect/hope.

Hmm. As far as I can tell, this will work - at least for now. The assumption that only 2-byte instructions can result in a SIGILL is of course not correct in general, that's just a consequence of the particular subset of instructions the back-end is currently using, so I'd be a bit hesitant to hard-code this ...

The whole approach of moving trap instructions to the end of the function may not be the best approach on s390x anyway. Other compilers tend to use a trick (that I haven't implemented yet in cranelift) where the "trap" instruction overlaps the conditional branch instruction. Specifically, to implement a conditional trap, you can use something like

   jgCOND .+2

which is encoded as a 6-byte instruction like so (with M encoding the condition):

  C0 M4 00 00 00 01

If the condition is true, this will branch two bytes forward, landing on the 00 00 bytes embedded in the branch itself - which now double as the trap instruction.

This is the same size as a branch to the end of the function, but doesn't actually require anything to be there.

Wasmtime GitHub notifications bot (Mar 14 2023 at 18:21):

alexcrichton commented on issue #6011:

Ah ok makes sense, would it be best to not apply this change to s390x in that case?

Wasmtime GitHub notifications bot (Mar 14 2023 at 18:37):

uweigand commented on issue #6011:

Ah ok makes sense, would it be best to not apply this change to s390x in that case?

Agreed. Just leave s390x out for now, and I'll implement that other change separately. Thanks!

Wasmtime GitHub notifications bot (Mar 21 2023 at 16:07):

uweigand commented on issue #6011:

@alexcrichton I've now implemented the s390x solution discussed above as #6079 .

Last updated: Nov 22 2024 at 16:03 UTC