Change the Module creation interface. #11
Conversation
There is now a more general `ModuleSectionInfo` trait which is used to query sections/segments as needed. This moves a lot of cumbersome conditional code into the library to make clients' lives easier. This also adds `object` as an optional dependency, where (if enabled) the trait is implemented for references to `object::read::Object` types.
mstange
left a comment
mstange
left a comment
There was a problem hiding this comment.
The trait looks good but I think having the old type available for simplicity would make the documentation look less overwhelming.
|
|
||
| [dependencies] | ||
| gimli = "0.27.0" | ||
| object = { version = ">=0.30", optional = true } |
There was a problem hiding this comment.
Hmm, what are the best practices around features? I find this behavior of "optional dependencies automatically turn into off-by-default feature names" a bit weird, it's hard to see at a glance what the list of available features is. Is this a common way of doing things?
There was a problem hiding this comment.
From a consumer perspective (I've not authored many crates with this requirement), I see this pattern occasionally. The convenient part is that it Just Works:tm: as a sibling to the optional crate. E.g., if I have a project where I'm using object already and then I add framehop to do some stuff (such a situation is likely to be quite common), the framehop support for the object crate will be automatically enabled without me doing anything else.
If you feel that it should be behind a separate, explicit flag, I can get behind that. But FWIW I think that cargo/docs.rs could use improvement when it comes to feature documentation as it is. I would have preferred a world where features in Cargo.toml had to have descriptions of their purpose. But IMO an optional dependency of this nature is a little different from explicit features, as it's meant to add some convenience features when the other dependency is present.
| svma_info: ModuleSvmaInfo, | ||
| unwind_data: ModuleUnwindData<D>, | ||
| text_data: Option<TextByteData<D>>, | ||
| mut section_info: impl ModuleSectionInfo<D>, |
There was a problem hiding this comment.
Hmm, this changes the API quite significantly, doesn't it? Now the consumers of this crate no longer decide themselves which type of unwind data gets used. In the past they could pick between .debug_frame and .eh_frame, for example. Now they just provide section contents and framehop will hopefully pick the right thing for them.
There was a problem hiding this comment.
In a way, yes, but if someone has a use case where they care about picking between those sections they can provide an appropriate impl to expose the things they are interested in. I think that use case is not very likely, but this interface still allows it nonetheless.
mach-O. Always check for `__eh_frame`/`__eh_frame_hdr` in case there's a wonky compiler out there.
The text bytes are only used by macOS "Compact Unwind Info"-guided unwinding. Before #11, we were computing `offset_from_base` in that code based on the text segment/section start SVMA and the base SVMA. In #11 this code was changed to use the file range, but this is actually incorrect; file offsets are not always equivalent to relative-to-base addresses, for example in the dyld shared cache. Furthermore, in samply's case, we're getting these bytes straight from the other process's memory, so we're not interested in any information about the file anyway. This PR changes things back to deal in terms of SVMAs.
The text bytes are only used by macOS "Compact Unwind Info"-guided unwinding. Before #11, we were computing `offset_from_base` in that code based on the text segment/section start SVMA and the base SVMA. In #11 this code was changed to use the file range, but this is actually incorrect; file offsets are not always equivalent to relative-to-base addresses, for example in the dyld shared cache. Furthermore, in samply's case, we're getting these bytes straight from the other process's memory, so we're not interested in any information about the file anyway. This PR changes things back to deal in terms of SVMAs.
The text bytes are only used by macOS "Compact Unwind Info"-guided unwinding. Before #11, we were computing `offset_from_base` in that code based on the text segment/section start SVMA and the base SVMA. In #11 this code was changed to use the file range, but this is actually incorrect; file offsets are not always equivalent to relative-to-base addresses, for example in the dyld shared cache. Furthermore, in samply's case, we're getting these bytes straight from the other process's memory, so we're not interested in any information about the file anyway. This PR changes things back to deal in terms of SVMAs.
2 participants
There is now a more general
ModuleSectionInfotrait which is used to query sections/segments as needed. This moves a lot of cumbersome conditional code into the library to make clients' lives easier.This also adds
objectas an optional dependency, where (if enabled) the trait is implemented for references toobject::read::Objecttypes.