Wasmtime GitHub notifications bot (Apr 18 2024 at 18:04): Wasmtime GitHub notifications bot (Apr 18 2024 at 18:04):elliottt requested fitzgen for a review on PR #8402.
Wasmtime GitHub notifications bot (Apr 18 2024 at 18:04):elliottt opened PR #8402 from elliottt:trevor/x64-savereg-offsets to bytecodealliance:main:
The stack offsets provided to
UnwindInst::SaveReginstructions are relative to the beginning of the clobber area of the stack, and on main the x64 backend computes those offsets by assuming a SP-relative offset and then adjusting it. This is problematic, as any changes to the frame layout might require updates in two locations: where we compute the initial SP offset, and where we compute the adjustment to make the offset relative to the clobber area again.This PR fixes this issue by making the offset we track in the loop always relative to the beginning of the clobber area, and adding the offset from SP only when emitting the store. This relies on the assumption that the clobber area is sized according to the constraints of the registers stored within it (16-byte alignment is required for floating point registers) but this assumption is enforced by existing behavior in
compute_clobber_size.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Apr 18 2024 at 18:04):elliottt requested wasmtime-compiler-reviewers for a review on PR #8402.
Wasmtime GitHub notifications bot (Apr 18 2024 at 18:05):elliottt requested jameysharp for a review on PR #8402.
Wasmtime GitHub notifications bot (Apr 18 2024 at 18:09):fitzgen submitted PR review:
Didn't audit this super carefully, since I trust you and Jamey on this stuff more than myself right now since you've been digging into it very recently, but I wonder if we should (in a follow up at some point in the future) only adjust SP via a helper function that not only emits the SP adjustment, but also the unwind info metadata instruction at the same time.
Wasmtime GitHub notifications bot (Apr 18 2024 at 18:12):elliottt updated PR #8402.
Wasmtime GitHub notifications bot (Apr 18 2024 at 18:14):elliottt commented on PR #8402:
Didn't audit this super carefully, since I trust you and Jamey on this stuff more than myself right now since you've been digging into it very recently, but I wonder if we should (in a follow up at some point in the future) only adjust SP via a helper function that not only emits the SP adjustment, but also the unwind info metadata instruction at the same time.
Possibly, though in this case the offset is expected to be relative to part of the stack. Were you thinking that we'd have a helper that assumed all unwind offsets to be SP relative, and made the adjustment when emitting the underlying
UnwindInst?
Wasmtime GitHub notifications bot (Apr 18 2024 at 18:14):jameysharp submitted PR review:
I love it, this is so much better. Thank you!
Wasmtime GitHub notifications bot (Apr 18 2024 at 18:15):elliottt edited PR #8402:
The stack offsets provided to
UnwindInst::SaveReginstructions are relative to the beginning of the clobber area of the stack, and on main the x64 backend computes those offsets by assuming a SP-relative offset and then adjusting it. This is problematic, as any changes to the frame layout might require updates in two locations: where we compute the initial SP offset, and where we compute the adjustment to make the offset relative to the clobber area again.This PR fixes this issue by making the offset we track in the loop always relative to the beginning of the clobber area, and adding the offset from SP only when emitting the store. This relies on the assumption that the clobber area is sized according to the constraints of the registers stored within it (16-byte alignment is required for floating point registers) but this assumption is enforced by existing behavior in
compute_clobber_size.This also fixes a long-standing bug with floating point register spill offsets, as the offset was saved before the alignment to a 16-byte offset was made, leaving the resulting unwind info potentially pointing at the wrong offset.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Apr 18 2024 at 18:15):elliottt updated PR #8402.
Wasmtime GitHub notifications bot (Apr 18 2024 at 18:33):elliottt updated PR #8402.
Wasmtime GitHub notifications bot (Apr 18 2024 at 18:34):elliottt requested jameysharp for a review on PR #8402.
Wasmtime GitHub notifications bot (Apr 18 2024 at 18:37):jameysharp submitted PR review:
Ah, making the same cleanup in clobber-restore was a good idea.
Wasmtime GitHub notifications bot (Apr 18 2024 at 20:37):fitzgen commented on PR #8402:
Possibly, though in this case the offset is expected to be relative to part of the stack. Were you thinking that we'd have a helper that assumed all unwind offsets to be SP relative, and made the adjustment when emitting the underlying
UnwindInst?It wasn't a fully formed idea. I was thinking that the canonical frame address (CFA) is always at a constant offset from the SP for any given PC (I think this holds true for us, since we don't have any
allocas) and that whenever we increment the SP, we always want to increment the offset to the CFA (i.e. emit an unwind inst) by the same amount. Not sure that this actually aligns 100% with what we are trying to do in general, though.
Wasmtime GitHub notifications bot (Apr 18 2024 at 20:57):elliottt merged PR #8402.
Last updated: Oct 23 2024 at 20:03 UTC