Stream: git-wasmtime

Topic: wasmtime / PR #6298 winch: Handle relocations and traps


view this post on Zulip 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 CompiledFunction struct to the wasmtime-cranelift-shared crate, 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 traps and relocations of the CompiledFunction get created, by introducing a constructor that operates on a MachBufferFinalized<Final>, esentially encapsulating this process in a single place for both Winch and Cranelift.

<!--
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 (Apr 27 2023 at 19:15):

saulecabrera updated PR #6298.

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

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

view this post on Zulip 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 to wasmtime-cranelift-shared datatypes. 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?

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

view this post on Zulip 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 current CompiledFunction in compile_function, the idea would be to defer any conversions when needed, e.g. in append_code for example.

Now that I'm a bit more acquainted with this part of the codebase I think I can try this out.

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2023 at 22:50):

saulecabrera updated PR #6298.

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2023 at 22:54):

saulecabrera updated PR #6298.

view this post on Zulip Wasmtime GitHub notifications bot (May 01 2023 at 23:08):

saulecabrera updated PR #6298.

view this post on Zulip Wasmtime GitHub notifications bot (May 02 2023 at 12:13):

saulecabrera updated PR #6298.

view this post on Zulip Wasmtime GitHub notifications bot (May 02 2023 at 12:16):

saulecabrera updated PR #6298.

view this post on Zulip Wasmtime GitHub notifications bot (May 02 2023 at 16:02):

saulecabrera updated PR #6298.

view this post on Zulip Wasmtime GitHub notifications bot (May 02 2023 at 17:19):

saulecabrera has marked PR #6298 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (May 02 2023 at 17:19):

saulecabrera requested itsrainy for a review on PR #6298.

view this post on Zulip Wasmtime GitHub notifications bot (May 02 2023 at 17:19):

saulecabrera requested wasmtime-core-reviewers for a review on PR #6298.

view this post on Zulip Wasmtime GitHub notifications bot (May 02 2023 at 19:06):

itsrainy requested alexcrichton for a review on PR #6298.

view this post on Zulip Wasmtime GitHub notifications bot (May 02 2023 at 19:45):

alexcrichton submitted PR review:

Looks reasonable to me!

view this post on Zulip Wasmtime GitHub notifications bot (May 02 2023 at 20:22):

saulecabrera merged PR #6298.


Last updated: Dec 23 2024 at 13:07 UTC