iximeow opened 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 theUnwindInfo
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 throughcranelift-module
, we would have to do a lot of legwork already done bygimli
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 howgimli
is intended to be used, either.Functionally, I want
isa::x86::fde::emit_fde
to be designed around adding to aFrameTable
provided bysink
, 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 ofFrameUnwindSink
collecting an array ofUnwindInfo
rather than just one.I imagine this looking like
FrameUnwindSink
impls building a collection of records, either aFrameTable
forLibunwind
or a vec ofUnwindInfo
forFastcall
. To go along with this, I expect such impls would be added incranelift-module
. These may or may not be suitable for use from wasmtime, I'm optimistic that they could because in the worst case aFrameUnwindSink
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 usingcranelift-faerie
orcranelift-object
, meaninglucet
can unwind wasm, have backtraces, all that good stuff :) I imagine @bjorn3 might like this for having unwind information when using Cranelift as arustc
backend as well?This also seems like a good excuse to move
isa::x86::fde
andisa::x86::unwind
toisa::x86::unwind::windows
andisa::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 :)
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, thereloc
andset_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 aTargetIsa::calculate_unwind
method that returns anUnwind
enumeration withUnwind::None
,Unwind::Windows(UnwindInfo)
, andUnwind::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 intermediateVec
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?
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 incranelift
and be reused. I realize now that relocations make that questionable, since there's no simpleVec<u8>
representation for unwind info that includes them.I like the idea of a
calculate_unwind
with anUnwindWriter
trait. I had a momentary concern about constructingFrameDescriptionEntry
without their correspondingCIE
, but that's all taken care of when theFDE
are serialized!
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 returnUnwindInfo
andFrameDescriptionEntry
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 aFrameTable
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 aResult
.
iximeow commented on Issue #1387:
I see, because Windows
UnwindInfo
doesn't involve relocations it is serializable to a simple byte Vec. AndFrameDescriptionEntry
tracks addresses that might involve relocations viagimli::Address
, so users can handle that as they see it. Cool!
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 theUnwindInfo
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 throughcranelift-module
, we would have to do a lot of legwork already done bygimli
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 howgimli
is intended to be used, either.Functionally, I want
isa::x86::fde::emit_fde
to be designed around adding to aFrameTable
provided bysink
, 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 ofFrameUnwindSink
collecting an array ofUnwindInfo
rather than just one.I imagine this looking like
FrameUnwindSink
impls building a collection of records, either aFrameTable
forLibunwind
or a vec ofUnwindInfo
forFastcall
. To go along with this, I expect such impls would be added incranelift-module
. These may or may not be suitable for use from wasmtime, I'm optimistic that they could because in the worst case aFrameUnwindSink
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 usingcranelift-faerie
orcranelift-object
, meaninglucet
can unwind wasm, have backtraces, all that good stuff :) I imagine @bjorn3 might like this for having unwind information when using Cranelift as arustc
backend as well?This also seems like a good excuse to move
isa::x86::fde
andisa::x86::unwind
toisa::x86::unwind::windows
andisa::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 :)
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!
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.
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 theUnwindInfo
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 throughcranelift-module
, we would have to do a lot of legwork already done bygimli
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 howgimli
is intended to be used, either.Functionally, I want
isa::x86::fde::emit_fde
to be designed around adding to aFrameTable
provided bysink
, 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 ofFrameUnwindSink
collecting an array ofUnwindInfo
rather than just one.I imagine this looking like
FrameUnwindSink
impls building a collection of records, either aFrameTable
forLibunwind
or a vec ofUnwindInfo
forFastcall
. To go along with this, I expect such impls would be added incranelift-module
. These may or may not be suitable for use from wasmtime, I'm optimistic that they could because in the worst case aFrameUnwindSink
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 usingcranelift-faerie
orcranelift-object
, meaninglucet
can unwind wasm, have backtraces, all that good stuff :) I imagine @bjorn3 might like this for having unwind information when using Cranelift as arustc
backend as well?This also seems like a good excuse to move
isa::x86::fde
andisa::x86::unwind
toisa::x86::unwind::windows
andisa::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: Dec 23 2024 at 13:07 UTC