Wasmtime GitHub notifications bot (Apr 27 2023 at 17:45): Wasmtime GitHub notifications bot (Apr 27 2023 at 17:45):saulecabrera opened PR #6298 from saulecabrera:winch-handle-relocs-and-traps to bytecodealliance:main:
This change introduces handling of traps and relocations in Winch, which was left out in https://github.com/bytecodealliance/wasmtime/pull/6119.
In order to so, this change moves the
CompiledFunctionstruct to thewasmtime-cranelift-sharedcrate, allowing Cranelift and Winch to operate on a single, shared representation, following some of the ideas discussed in https://github.com/bytecodealliance/wasmtime/pull/5944.Even though Winch doesn't rely on all the fields of
CompiledFunction, it eventually will. With the addition of relocations and traps it started to be more evident that even if we wanted to have different representations of a compiled function, they would end up being very similar.This change also consolidates where the
trapsandrelocationsof theCompiledFunctionget created, by introducing a constructor that operates on aMachBufferFinalized<Final>, esentially encapsulating this process in a single place for both Winch and Cranelift.<!--
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 27 2023 at 19:15):saulecabrera updated PR #6298.
Wasmtime GitHub notifications bot (Apr 27 2023 at 19:40):alexcrichton submitted PR review:
I've got a possible high-level idea which might help simplify things by removing the number of types in flight, but irrespective of which way we go on that this looks reasonable to me!
Wasmtime GitHub notifications bot (Apr 27 2023 at 19:40):alexcrichton submitted PR review:
I've got a possible high-level idea which might help simplify things by removing the number of types in flight, but irrespective of which way we go on that this looks reasonable to me!
Wasmtime GitHub notifications bot (Apr 27 2023 at 19:40):alexcrichton created PR review comment:
One thing I attempted a long time ago but didn't get a chance to finish, which I think might simplify the winch/cranelift duo in the long run, is to replace this entire structure with
MachBufferFinalized. Most of these fields are already present within there and the copies here are simple translations from Cranelift datatypes towasmtime-cranelift-shareddatatypes. In such a situation I think it'd be better to leave everything at-rest as Cranelift datatypes and only much later, for example when inserting into an object, is anything translated from Cranelift.Would you be up for exploring such a change like this?
Wasmtime GitHub notifications bot (Apr 27 2023 at 19:40):alexcrichton created PR review comment:
This is an example where ideally we'd "simply" feed in a
&MachBufferFinalizedinto this function so you wouldn't have to pass all these extra fields manually. That's at least the rough idea, unsure how it'll play out in practice.
Wasmtime GitHub notifications bot (Apr 27 2023 at 19:51):saulecabrera created PR review comment:
Right, I see what you mean. Instead of eagerly converting from
MachBufferFinalized<T>into the currentCompiledFunctionincompile_function, the idea would be to defer any conversions when needed, e.g. inappend_codefor example.Now that I'm a bit more acquainted with this part of the codebase I think I can try this out.
Wasmtime GitHub notifications bot (May 01 2023 at 22:50):saulecabrera updated PR #6298.
Wasmtime GitHub notifications bot (May 01 2023 at 22:54):saulecabrera updated PR #6298.
Wasmtime GitHub notifications bot (May 01 2023 at 23:08):saulecabrera updated PR #6298.
Wasmtime GitHub notifications bot (May 02 2023 at 12:13):saulecabrera updated PR #6298.
Wasmtime GitHub notifications bot (May 02 2023 at 12:16):saulecabrera updated PR #6298.
Wasmtime GitHub notifications bot (May 02 2023 at 16:02):saulecabrera updated PR #6298.
Wasmtime GitHub notifications bot (May 02 2023 at 17:19):saulecabrera has marked PR #6298 as ready for review.
Wasmtime GitHub notifications bot (May 02 2023 at 17:19):saulecabrera requested itsrainy for a review on PR #6298.
Wasmtime GitHub notifications bot (May 02 2023 at 17:19):saulecabrera requested wasmtime-core-reviewers for a review on PR #6298.
Wasmtime GitHub notifications bot (May 02 2023 at 19:06):itsrainy requested alexcrichton for a review on PR #6298.
Wasmtime GitHub notifications bot (May 02 2023 at 19:45):alexcrichton submitted PR review:
Looks reasonable to me!
Wasmtime GitHub notifications bot (May 02 2023 at 20:22):saulecabrera merged PR #6298.
Last updated: Oct 23 2024 at 20:03 UTC