Stream: git-wasmtime

Topic: wasmtime / PR #7798 winch: Emit unwind info in the x64 ba...


view this post on Zulip Wasmtime GitHub notifications bot (Jan 24 2024 at 20:19):

elliottt edited PR #7798.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2024 at 00:54):

elliottt has marked PR #7798 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2024 at 00:54):

elliottt requested wasmtime-compiler-reviewers for a review on PR #7798.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2024 at 00:54):

elliottt requested fitzgen for a review on PR #7798.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2024 at 00:54):

elliottt requested wasmtime-core-reviewers for a review on PR #7798.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2024 at 00:54):

elliottt requested wasmtime-default-reviewers for a review on PR #7798.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2024 at 00:54):

elliottt requested saulecabrera for a review on PR #7798.

view this post on Zulip 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:

With answers to those questions, I think this PR will be in a good state.

Follow-on work that needs to be done is:

view this post on Zulip 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-codegen details were necessary to expose to allow for this, most notably the RealReg type 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:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2024 at 01:00):

elliottt updated PR #7798.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2024 at 01:23):

elliottt updated PR #7798.

view this post on Zulip 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 call instruction's return address)

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2024 at 18:20):

elliottt updated PR #7798.

view this post on Zulip 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.

view this post on Zulip 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.

view this post on Zulip 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?

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2024 at 18:30):

saulecabrera created PR review comment:

Similar to my comment above.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2024 at 18:33):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2024 at 18:33):

elliottt created PR review comment:

Absolutely!

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2024 at 18:33):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2024 at 18:33):

elliottt created PR review comment:

Oh fantastic, I didn't realize this existed!

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2024 at 18:50):

elliottt updated PR #7798.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2024 at 19:03):

elliottt updated PR #7798.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2024 at 19:25):

elliottt updated PR #7798.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 25 2024 at 23:39):

elliottt merged PR #7798.


Last updated: Dec 23 2024 at 12:05 UTC