cranelift / Issue #1387 Improve support for bulk function... · git-cranelift

Stream: git-cranelift

Topic: cranelift / Issue #1387 Improve support for bulk function...

GitHub (Feb 12 2020 at 00:34):

I'd like FrameUnwindSink to allow collecting records for multiple functions into one sink. Currently, TargetIsa::emit_unwind_info is designed around emitting one full record (a whole .eh_frame or the UnwindInfo part of a RUNTIME_FUNCTION) per function, which is awkward to use in the face of multiple functions. In squaring away this interface with #902, I've realized that to generate good unwind information through cranelift-module, we would have to do a lot of legwork already done by gimli to join all distinct .eh_frame, so I'd rather just build the right structures in the first place. As @philipc commented the existing interface isn't entirely how gimli is intended to be used, either.

Functionally, I want isa::x86::fde::emit_fde to be designed around adding to a FrameTable provided by sink, rather than constructing and writing an .eh_frame section for the function being described. Windows unwind info generation wouldn't have to change much, with implementors of FrameUnwindSink collecting an array of UnwindInfo rather than just one.

I imagine this looking like FrameUnwindSink impls building a collection of records, either a FrameTable for Libunwind or a vec of UnwindInfo for Fastcall. To go along with this, I expect such impls would be added in cranelift-module. These may or may not be suitable for use from wasmtime, I'm optimistic that they could because in the worst case a FrameUnwindSink can be created, used for one function, and then discarded, preserving the current behavior.

My expectation is that a trait like
trait FrameUnwindSink {
    /// Create an instance of the implementor for the provided unwind style.
    fn for_style(kind: FrameUnwindKind) -> Self;

    /// Add a function to the sink. May panic if the function's calling convention
    /// is not compatible with the unwind style this sink was created with.
    fn add_function(&mut self, func: &Function, isa: &dyn TargetIsa);

    /// Serialize this sink's contents in a manner appropriate for this sink's
    /// unwind style.
    fn serialize(&self) -> Vec<u8>;
should satisfy everyone's needs?

The notable benefit of this is making it easy to generate a single .eh_frame section when using cranelift-faerie or cranelift-object, meaning lucet can unwind wasm, have backtraces, all that good stuff :) I imagine @bjorn3 might like this for having unwind information when using Cranelift as a rustc backend as well?

This also seems like a good excuse to move isa::x86::fde and isa::x86::unwind to isa::x86::unwind::windows and isa::x86::unwind::libunwind respectively?

cc @yurydelendik as I'd like your thoughts on this idea, and @peterhuene as I know you've also worked on unwind information here.

Opening an issue to make sure this gets a design friendly to both lucet and wasmtime, rather than just tweaking #902 until something looks right :)

GitHub (Feb 12 2020 at 04:59):

peterhuene commented on Issue #1387:

So the current design of the trait is that something external to Cranelift (e.g. Wasmtime) would be the implementer of FrameUnwindSink so (ideally) it wouldn't need to know anything about the unwind implementation details itself. Unfortunately, given the differences between Windows and the other platforms, the abstraction is quite a bit leaky (for example, the reloc and set_entry_offset methods are only used for FDEs).

With the above revised trait definition, it seems to me like it is now the sink that is responsible for recording each function and serializing the unwind information somehow. At first glance this seems to be the opposite of the current implementation and something we would like to avoid in Wasmtime so that it doesn't have to have knowledge of how to create the unwind info itself.

That said, I am convinced that FrameUnwindSink is no longer a working abstraction for what we want here.

Perhaps rather than having TargetIsa::emit_unwind_info being responsible for serializing opaque unwind data into a sink we could refactor this such that we can have a TargetIsa::calculate_unwind method that returns an Unwind enumeration with Unwind::None, Unwind::Windows(UnwindInfo), and Unwind::Dwarf(FrameDescriptionEntry) (for now). This would allow Cranelift users to collect these without having to only deal with the serialized data.

UnwindInfo::emit would change to take simpler "writer" trait. This would potentially allow it to write directly to where the unwind information needs to be in memory rather than some intermediate Vec like it does now.

For Dwarf, users would then be free to construct a single FrameTable and serialize all of the FDEs at once.

I also think putting the unwind types into corresponding submodules of isa::x86::unwind makes sense.

@iximeow @yurydelendik what are your thoughts on this approach?

GitHub (Feb 12 2020 at 18:59):

iximeow commented on Issue #1387:

With the above revised trait definition, it seems to me like it is now the sink that is responsible for recording each function and serializing the unwind information somehow.

This is true - my hope was that in fact implementors of FrameUnwindSink could live in cranelift and be reused. I realize now that relocations make that questionable, since there's no simple Vec<u8> representation for unwind info that includes them.

I like the idea of a calculate_unwind with an UnwindWriter trait. I had a momentary concern about constructing FrameDescriptionEntry without their corresponding CIE, but that's all taken care of when the FDE are serialized!

GitHub (Feb 12 2020 at 19:16):

peterhuene commented on Issue #1387:

With regards to an UnwindWriter trait, I don't think we would need to abstract the writing across the different types of unwind information given a model above where we return UnwindInfo and FrameDescriptionEntry directly to Cranelift users; that enables the caller to collect these and decide how best to write them in bulk.

Cranelift users will need to know how to build a FrameTable for handling DWARF, but gimli makes that pretty easy to do. Wasmtime has a bunch of DWARF-specific handling for the unwind information already, so moving the construction of a FrameTable to there isn't a big deal imo.

For UnwindInfo::emit, I think perhaps a writer trait wouldn't be necessary and it should just accept a &mut [u8]. UnwindInfo::emit can simply wrap the slice internally with a "writer" and perhaps panic if the slice isn't big enough or returns a Result.

GitHub (Feb 12 2020 at 19:46):

iximeow commented on Issue #1387:

I see, because Windows UnwindInfo doesn't involve relocations it is serializable to a simple byte Vec. And FrameDescriptionEntry tracks addresses that might involve relocations via gimli::Address, so users can handle that as they see it. Cool!

GitHub (Feb 18 2020 at 21:37):

peterhuene assigned Issue #1387:

I'd like FrameUnwindSink to allow collecting records for multiple functions into one sink. Currently, TargetIsa::emit_unwind_info is designed around emitting one full record (a whole .eh_frame or the UnwindInfo part of a RUNTIME_FUNCTION) per function, which is awkward to use in the face of multiple functions. In squaring away this interface with #902, I've realized that to generate good unwind information through cranelift-module, we would have to do a lot of legwork already done by gimli to join all distinct .eh_frame, so I'd rather just build the right structures in the first place. As @philipc commented the existing interface isn't entirely how gimli is intended to be used, either.

Functionally, I want isa::x86::fde::emit_fde to be designed around adding to a FrameTable provided by sink, rather than constructing and writing an .eh_frame section for the function being described. Windows unwind info generation wouldn't have to change much, with implementors of FrameUnwindSink collecting an array of UnwindInfo rather than just one.

I imagine this looking like FrameUnwindSink impls building a collection of records, either a FrameTable for Libunwind or a vec of UnwindInfo for Fastcall. To go along with this, I expect such impls would be added in cranelift-module. These may or may not be suitable for use from wasmtime, I'm optimistic that they could because in the worst case a FrameUnwindSink can be created, used for one function, and then discarded, preserving the current behavior.

My expectation is that a trait like
trait FrameUnwindSink {
    /// Create an instance of the implementor for the provided unwind style.
    fn for_style(kind: FrameUnwindKind) -> Self;

    /// Add a function to the sink. May panic if the function's calling convention
    /// is not compatible with the unwind style this sink was created with.
    fn add_function(&mut self, func: &Function, isa: &dyn TargetIsa);

    /// Serialize this sink's contents in a manner appropriate for this sink's
    /// unwind style.
    fn serialize(&self) -> Vec<u8>;
should satisfy everyone's needs?

The notable benefit of this is making it easy to generate a single .eh_frame section when using cranelift-faerie or cranelift-object, meaning lucet can unwind wasm, have backtraces, all that good stuff :) I imagine @bjorn3 might like this for having unwind information when using Cranelift as a rustc backend as well?

This also seems like a good excuse to move isa::x86::fde and isa::x86::unwind to isa::x86::unwind::windows and isa::x86::unwind::libunwind respectively?

cc @yurydelendik as I'd like your thoughts on this idea, and @peterhuene as I know you've also worked on unwind information here.

Opening an issue to make sure this gets a design friendly to both lucet and wasmtime, rather than just tweaking #902 until something looks right :)

GitHub (Feb 20 2020 at 18:55):

iximeow commented on Issue #1387:

I was going to put together a PR based on this conversation but I see you've assigned this to yourself so I'll take that as you working on this - thank you @peterhuene!

GitHub (Feb 20 2020 at 19:21):

peterhuene commented on Issue #1387:

@iximeow I've indeed started on this and will get it done as soon as I get some other unrelated Wasmtime work up.

GitHub (Feb 28 2020 at 23:28):

alexcrichton transferred Issue #1387 (assigned to peterhuene):

I'd like FrameUnwindSink to allow collecting records for multiple functions into one sink. Currently, TargetIsa::emit_unwind_info is designed around emitting one full record (a whole .eh_frame or the UnwindInfo part of a RUNTIME_FUNCTION) per function, which is awkward to use in the face of multiple functions. In squaring away this interface with #902, I've realized that to generate good unwind information through cranelift-module, we would have to do a lot of legwork already done by gimli to join all distinct .eh_frame, so I'd rather just build the right structures in the first place. As @philipc commented the existing interface isn't entirely how gimli is intended to be used, either.

Functionally, I want isa::x86::fde::emit_fde to be designed around adding to a FrameTable provided by sink, rather than constructing and writing an .eh_frame section for the function being described. Windows unwind info generation wouldn't have to change much, with implementors of FrameUnwindSink collecting an array of UnwindInfo rather than just one.

I imagine this looking like FrameUnwindSink impls building a collection of records, either a FrameTable for Libunwind or a vec of UnwindInfo for Fastcall. To go along with this, I expect such impls would be added in cranelift-module. These may or may not be suitable for use from wasmtime, I'm optimistic that they could because in the worst case a FrameUnwindSink can be created, used for one function, and then discarded, preserving the current behavior.

My expectation is that a trait like
trait FrameUnwindSink {
    /// Create an instance of the implementor for the provided unwind style.
    fn for_style(kind: FrameUnwindKind) -> Self;

    /// Add a function to the sink. May panic if the function's calling convention
    /// is not compatible with the unwind style this sink was created with.
    fn add_function(&mut self, func: &Function, isa: &dyn TargetIsa);

    /// Serialize this sink's contents in a manner appropriate for this sink's
    /// unwind style.
    fn serialize(&self) -> Vec<u8>;
should satisfy everyone's needs?

The notable benefit of this is making it easy to generate a single .eh_frame section when using cranelift-faerie or cranelift-object, meaning lucet can unwind wasm, have backtraces, all that good stuff :) I imagine @bjorn3 might like this for having unwind information when using Cranelift as a rustc backend as well?

This also seems like a good excuse to move isa::x86::fde and isa::x86::unwind to isa::x86::unwind::windows and isa::x86::unwind::libunwind respectively?

cc @yurydelendik as I'd like your thoughts on this idea, and @peterhuene as I know you've also worked on unwind information here.

Opening an issue to make sure this gets a design friendly to both lucet and wasmtime, rather than just tweaking #902 until something looks right :)

Last updated: Nov 22 2024 at 16:03 UTC