Stream: git-wasmtime

Topic: wasmtime / PR #7047 riscv64: Cleanup trap handling

view this post on Zulip Wasmtime GitHub notifications bot (Sep 15 2023 at 13:01):

afonso360 opened PR #7047 from afonso360:riscv-dedup-trap to bytecodealliance:main:

:wave: Hey,

This PR cleans up our trap instructions in the backend. We have 2 "TrapIf" style instructions. One that compares against zero, and a second that does any integer comparison. This PR does a couple of changes.

Merge both TrapIf and TrapIfC. The new TrapIf instruction allows trapping with any integer condition. This is the first commit, and it does not change any of the emitted instruction sequences, just our printing.

Use the new MachBuffer::defer_trap (#6011) method to place trap opcodes out of line. This does actually change the golden tests, and is contained to the second commit.

Lastly, the defer_trap change exposed a bug in our lowering of br_table. Previously we were emitting islands right before the jump table targets, but crucially after calculating the jump offset. This meant that if the island were actually emitted, we could jump right into the middle of it!

This was exposed by the defer_trap change since the test case would emit a trap before a huge br_table, forcing the island, and then jump into offset 0, which actually ended up being the trap opcode instead of the branch target.

We now emit the island at the start of the br_table sequence instead of in the middle.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 15 2023 at 13:01):

afonso360 requested wasmtime-compiler-reviewers for a review on PR #7047.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 15 2023 at 13:01):

afonso360 requested fitzgen for a review on PR #7047.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 15 2023 at 13:09):

afonso360 edited PR #7047:

:wave: Hey,

This PR cleans up our trap instructions in the backend. We have 2 "TrapIf" style instructions. One that compares against zero, and a second that does any integer comparison. This PR does a couple of changes.

Merge both TrapIf and TrapIfC. The new TrapIf instruction allows trapping with any integer condition. This is the first commit, and it does not change any of the emitted instruction sequences, just our printing.

Use the new MachBuffer::defer_trap (#6011) method to place trap opcodes out of line. This does actually change the golden tests, and is contained to the second commit.

Lastly, the defer_trap change exposed a bug in our lowering of br_table. Previously we were emitting islands right before the jump table targets, but crucially after calculating the jump offset. This meant that if the island were actually emitted, we could jump right into the middle of it!

This was exposed by the defer_trap change since the test case would emit a trap before a huge br_table, forcing the island, and then jump into the first br_table target, which actually ended up being the trap opcode instead of the branch target.

We now emit the island at the start of the br_table sequence instead of in the middle.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 15 2023 at 13:25):

afonso360 edited PR #7047:

:wave: Hey,

This PR cleans up our trap instructions in the backend. We have 2 "TrapIf" style instructions. One that compares against zero, and a second that does any integer comparison. This PR does a couple of changes.

Merge both TrapIf and TrapIfC. The new TrapIf instruction allows trapping with any integer condition. This is the first commit, and it does not change any of the emitted instruction sequences, just our printing.

Use the new MachBuffer::defer_trap (#6011) method to place trap opcodes out of line. This does actually change the golden tests, and is contained to the second commit.

Lastly, the defer_trap change exposed a bug in our lowering of br_table. Previously we were emitting islands right before the jump table targets, but crucially after calculating the jump offset. This meant that if the island were actually emitted, we could jump right into the middle of it!

This was exposed by the defer_trap change since the test case would emit a trap before a huge br_table, forcing the island, and then jump into the first br_table target, which actually ended up being the trap opcode instead of the intended branch target.

We now emit the island at the start of the br_table sequence instead of in the middle.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 15 2023 at 15:28):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 15 2023 at 15:28):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 15 2023 at 15:28):

alexcrichton created PR review comment:

This is confusing me a bit as trapif seems to trap if the condition succeeds, so the condition here is that if the input is not equal to zero this is trapping, asserting the register is zero. The function, however, is intended to trap if the register is zero.

Should the helper perhaps be trapnz?

view this post on Zulip Wasmtime GitHub notifications bot (Sep 15 2023 at 15:28):

alexcrichton created PR review comment:

Along the lines of the above, should this perhaps become a trapz helper?

view this post on Zulip Wasmtime GitHub notifications bot (Sep 16 2023 at 09:23):

afonso360 updated PR #7047.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 16 2023 at 09:24):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 16 2023 at 09:24):

afonso360 created PR review comment:

Yeah, looks like I got a bit confused here. Thanks for catching this!

view this post on Zulip Wasmtime GitHub notifications bot (Sep 16 2023 at 10:42):

afonso360 updated PR #7047.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2023 at 19:09):

afonso360 updated PR #7047.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 18 2023 at 20:32):

alexcrichton merged PR #7047.

Last updated: Nov 22 2024 at 16:03 UTC