Wasmtime GitHub notifications bot (Jan 24 2024 at 20:19): Wasmtime GitHub notifications bot (Jan 24 2024 at 20:19):elliottt edited PR #7798.
Wasmtime GitHub notifications bot (Jan 25 2024 at 00:54):elliottt has marked PR #7798 as ready for review.
Wasmtime GitHub notifications bot (Jan 25 2024 at 00:54):elliottt requested wasmtime-compiler-reviewers for a review on PR #7798.
Wasmtime GitHub notifications bot (Jan 25 2024 at 00:54):elliottt requested fitzgen for a review on PR #7798.
Wasmtime GitHub notifications bot (Jan 25 2024 at 00:54):elliottt requested wasmtime-core-reviewers for a review on PR #7798.
Wasmtime GitHub notifications bot (Jan 25 2024 at 00:54):elliottt requested wasmtime-default-reviewers for a review on PR #7798.
Wasmtime GitHub notifications bot (Jan 25 2024 at 00:54):elliottt requested saulecabrera for a review on PR #7798.
Wasmtime GitHub notifications bot (Jan 25 2024 at 00:58):elliottt commented on PR #7798:
There are a few open questions for me at this point:
- Do we have a guiding principle for what goes in
cranelift-sharedvs making things public incranelift-codegen?- Should we reuse the
FrameLayouttype in cranelift instead of using constants when constructingUnwindInfovalues?With answers to those questions, I think this PR will be in a good state.
Follow-on work that needs to be done is:
- Add the probe stack step to stack overflow checking
- Check if we need the same 32k stack overflow check that cranelift does
- Move the stack overflow check into the prologue, as the stack overflow check happening outside of the prologue has consequences on windows
Wasmtime GitHub notifications bot (Jan 25 2024 at 00:59):elliottt edited PR #7798:
Reuse cranelift's implementation of unwinding info for windows, allowing for traps to work properly in winch. Some
cranelift-codegendetails were necessary to expose to allow for this, most notably theRealRegtype needed to be exported. Additionally, I opted to reuse functionality from cranelift by adding public functions for ingesting the unwind info present, which greatly simplified the implementation in winch.<!--
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 (Jan 25 2024 at 01:00):elliottt updated PR #7798.
Wasmtime GitHub notifications bot (Jan 25 2024 at 01:23):elliottt updated PR #7798.
Wasmtime GitHub notifications bot (Jan 25 2024 at 01:29):alexcrichton commented on PR #7798:
Do we have a guiding principle for what goes in cranelift-shared vs making things public in cranelift-codegen?
AFAIK it's "whatever goes". There's probably better places to draw these abstractions and/or perhaps use other abstractions already in Cranelift instead of these abstractions. In general though I think it's ok to not think too too hard about this as it's all internal and we can refactor it in the future pretty easily.
Should we reuse the FrameLayout type in cranelift instead of using constants when constructing UnwindInfo values?
I haven't used this type much but reading over this to prologue is pretty neatly defined within a single function so I think it's reasonable to leave the constants as-is, although perhaps with a comment explaining a bit (e.g. 16 instead of 8 to account for the original
callinstruction's return address)
Wasmtime GitHub notifications bot (Jan 25 2024 at 18:20):elliottt updated PR #7798.
Wasmtime GitHub notifications bot (Jan 25 2024 at 18:30):saulecabrera submitted PR review:
Looks reasonable to me! Thanks for tackling this. I left a couple of comments/suggestions.
Wasmtime GitHub notifications bot (Jan 25 2024 at 18:30):saulecabrera submitted PR review:
Looks reasonable to me! Thanks for tackling this. I left a couple of comments/suggestions.
Wasmtime GitHub notifications bot (Jan 25 2024 at 18:30):saulecabrera created PR review comment:
Could this code be abstracted into a single method in
Compiler?
Wasmtime GitHub notifications bot (Jan 25 2024 at 18:30):saulecabrera created PR review comment:
Would it be possible to use https://github.com/bytecodealliance/wasmtime/blob/main/winch/codegen/src/isa/x64/abi.rs#L71 here?
Wasmtime GitHub notifications bot (Jan 25 2024 at 18:30):saulecabrera created PR review comment:
Similar to my comment above.
Wasmtime GitHub notifications bot (Jan 25 2024 at 18:33):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Jan 25 2024 at 18:33):elliottt created PR review comment:
Absolutely!
Wasmtime GitHub notifications bot (Jan 25 2024 at 18:33):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Jan 25 2024 at 18:33):elliottt created PR review comment:
Oh fantastic, I didn't realize this existed!
Wasmtime GitHub notifications bot (Jan 25 2024 at 18:50):elliottt updated PR #7798.
Wasmtime GitHub notifications bot (Jan 25 2024 at 19:03):elliottt updated PR #7798.
Wasmtime GitHub notifications bot (Jan 25 2024 at 19:25):elliottt updated PR #7798.
Wasmtime GitHub notifications bot (Jan 25 2024 at 23:39):elliottt merged PR #7798.
Last updated: Nov 22 2024 at 16:03 UTC