Stream: git-wasmtime

Topic: wasmtime / PR #3180 Reimplement how unwind information is...


view this post on Zulip Wasmtime GitHub notifications bot (Aug 11 2021 at 20:21):

alexcrichton opened PR #3180 from refactor-unwind to main:

This commit is a major refactoring of how unwind information is stored
after compilation of a function has finished. Previously we would store
the raw UnwindInfo as a result of compilation and this would get
serialized/deserialized alongside the rest of the ELF object that
compilation creates. Whenever functions were registered with
CodeMemory this would also result in registering unwinding information
dynamically at runtime, which in the case of Unix, for example, would
dynamically created FDE/CIE entries on-the-fly.

Eventually I'd like to support compiling Wasmtime without Cranelift, but
this means that UnwindInfo wouldn't be easily available to decode into
and create unwinding information from. To solve this I've changed the
ELF object created to have the unwinding information encoded into it
ahead-of-time so loading code into memory no longer needs to create
unwinding tables. This change has two different implementations for
Windows/Unix:

The overall result of this commit is that unwinding information is no
longer stored in its cranelift-data-structure form on disk. This means
that this unwinding information format is only present during
compilation, which will make it that much easier to compile out
cranelift in the future.

This commit also significantly refactors CodeMemory since the way
unwinding information is handled is not much different from before.
Previously CodeMemory was suitable for incrementally adding more and
more functions to it, but nowadays a CodeMemory either lives per
module (in which case all functions are known up front) or it's created
once-per-Func::new with two trampolines. In both cases we know all
functions up front so the functionality of incrementally adding more and
more segments is no longer needed. This commit removes the ability to
add a function-at-a-time in CodeMemory and instead it can now only
load objects in their entirety. A small helper function is added to
build a small object file for trampolines in Func::new to handle
allocation there.

Finally, this commit also refactors the wasmtime-obj crate and its
builder structure to be more amenable to this strategy of managing
unwinding tables.

It is not intentional to have any real functional change as a result of
this commit. This might accelerate loading a module from cache slightly
since less work is needed to manage the unwinding information, but
that's just a side benefit from the main goal of this commit which is to
remove the dependence on cranelift unwinding information being available
at runtime.

On a procedural note this draws from https://github.com/bytecodealliance/wasmtime/pull/3179 and https://github.com/bytecodealliance/wasmtime/pull/3176 so only the last commit here is what's to be reviewed.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 11 2021 at 20:22):

alexcrichton updated PR #3180 from refactor-unwind to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 11 2021 at 20:30):

alexcrichton updated PR #3180 from refactor-unwind to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 11 2021 at 21:25):

alexcrichton updated PR #3180 from refactor-unwind to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 11 2021 at 21:26):

alexcrichton updated PR #3180 from refactor-unwind to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 11 2021 at 21:33):

alexcrichton updated PR #3180 from refactor-unwind to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 12 2021 at 14:39):

alexcrichton updated PR #3180 from refactor-unwind to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 12 2021 at 20:16):

alexcrichton updated PR #3180 from refactor-unwind to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 12 2021 at 20:16):

alexcrichton updated PR #3180 from refactor-unwind to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 12 2021 at 20:39):

alexcrichton requested fitzgen for a review on PR #3180.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 12 2021 at 21:59):

alexcrichton updated PR #3180 from refactor-unwind to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 12 2021 at 21:59):

alexcrichton edited PR #3180 from refactor-unwind to main:

This commit is a major refactoring of how unwind information is stored
after compilation of a function has finished. Previously we would store
the raw UnwindInfo as a result of compilation and this would get
serialized/deserialized alongside the rest of the ELF object that
compilation creates. Whenever functions were registered with
CodeMemory this would also result in registering unwinding information
dynamically at runtime, which in the case of Unix, for example, would
dynamically created FDE/CIE entries on-the-fly.

Eventually I'd like to support compiling Wasmtime without Cranelift, but
this means that UnwindInfo wouldn't be easily available to decode into
and create unwinding information from. To solve this I've changed the
ELF object created to have the unwinding information encoded into it
ahead-of-time so loading code into memory no longer needs to create
unwinding tables. This change has two different implementations for
Windows/Unix:

The overall result of this commit is that unwinding information is no
longer stored in its cranelift-data-structure form on disk. This means
that this unwinding information format is only present during
compilation, which will make it that much easier to compile out
cranelift in the future.

This commit also significantly refactors CodeMemory since the way
unwinding information is handled is not much different from before.
Previously CodeMemory was suitable for incrementally adding more and
more functions to it, but nowadays a CodeMemory either lives per
module (in which case all functions are known up front) or it's created
once-per-Func::new with two trampolines. In both cases we know all
functions up front so the functionality of incrementally adding more and
more segments is no longer needed. This commit removes the ability to
add a function-at-a-time in CodeMemory and instead it can now only
load objects in their entirety. A small helper function is added to
build a small object file for trampolines in Func::new to handle
allocation there.

Finally, this commit also refactors the wasmtime-obj crate and its
builder structure to be more amenable to this strategy of managing
unwinding tables.

It is not intentional to have any real functional change as a result of
this commit. This might accelerate loading a module from cache slightly
since less work is needed to manage the unwinding information, but
that's just a side benefit from the main goal of this commit which is to
remove the dependence on cranelift unwinding information being available
at runtime.

On a procedural note this draws from https://github.com/bytecodealliance/wasmtime/pull/3179 and https://github.com/bytecodealliance/wasmtime/pull/3176 so only the last commit here is what's to be reviewed.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2021 at 17:08):

alexcrichton updated PR #3180 from refactor-unwind to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2021 at 17:08):

alexcrichton updated PR #3180 from refactor-unwind to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2021 at 17:28):

alexcrichton updated PR #3180 from refactor-unwind to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2021 at 20:33):

alexcrichton updated PR #3180 from refactor-unwind to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2021 at 20:33):

alexcrichton updated PR #3180 from refactor-unwind to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2021 at 20:37):

alexcrichton updated PR #3180 from refactor-unwind to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 16 2021 at 22:48):

alexcrichton updated PR #3180 from refactor-unwind to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 20:45):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 20:45):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 20:45):

fitzgen created PR review comment:

I think we could do a slightly more helpful chained error message here:

failed to parse internal ELF compilation artifact

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 20:45):

fitzgen created PR review comment:

Nitpick: this can be removed, I guess?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 20:45):

fitzgen created PR review comment:

ugh I forgot about this bug

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 20:45):

fitzgen created PR review comment:

To clarify: the alternative to this whole DW_EH_PE_pcrel approach would be to emit relocs for the FDEs, right?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 20:45):

fitzgen created PR review comment:

Nitpick, but I find it more confusing to turbo fish unzip than to give an explicit type to the LHS of the assignment, since unzip has those two extra type parameters that I don't remember what is going on with them and have to look up in the docs again. On the other hand, when the explicit types are in the LHS of the assignment, I don't get side tracked by these meaningless type parameters.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 20:45):

fitzgen created PR review comment:

//! symbols that refer to libcalls.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 20:45):

fitzgen created PR review comment:

//! relocation records for the linking stage. If DWARF sections exist, their

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 21:20):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 21:20):

alexcrichton created PR review comment:

Indeed yeah. I'll admit that I know so little about relocations I didn't really try to do this at all and I didn't feel I needed to pursue this much because I got the pcrel bits working. Not having relocations is somewhat beneficial in that loading these tables is that much faster (it's just what's in-memory) and in theory amenable to the hypothetical mmap-and-go world.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 21:21):

alexcrichton updated PR #3180 from refactor-unwind to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 21:22):

alexcrichton updated PR #3180 from refactor-unwind to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 17 2021 at 22:14):

alexcrichton merged PR #3180.


Last updated: Dec 23 2024 at 13:07 UTC