-
Notifications
You must be signed in to change notification settings - Fork 109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix ranges with the GNU DWO extension and other improvements #568
Conversation
…debug_addr and .debug_rnglists from.
…resented by DW_FORM_sec_offset but need to be adjusted by the rnglist_base value to find the proper values in the binary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find for the bug fix.
It seems like the dwarfdump dwo changes include things that we eventually should support in the library itself somehow.
@@ -297,6 +297,12 @@ impl<R: Reader> Dwarf<R> { | |||
unit: &Unit<R>, | |||
offset: RangeListsOffset<R::Offset>, | |||
) -> Result<RngListIter<R>> { | |||
let offset = if self.file_type == DwarfFileType::Dwo && unit.header.version() < 5 { | |||
RangeListsOffset(offset.0.wrapping_add(unit.rnglists_base.0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that RangeListsOffset
has different meanings at different places in the code. While your changes look correct, it is hard to be sure, and ditto for users of this library. e.g. it looks like the AttributeValue::RangeListsRef
support in write/unit.rs
needs fixing too. I prefer if we introduce something like RawRangeListsOffset
that is used in AttributeValue
, and then convert that into a RangeListsOffset
. Or do the conversion when parsing attributes but we may not have the required info there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the downside of a new type is that then everybody has to be aware of and handle this stuff that's only needed for a non-standard and obsolete extension.
I've never looked at the writing side of gimli but if it's at all possible I would just not support writing these non-standard GNU extensions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a valid concern, but if it's important enough for you to handle it, then maybe others want to as well? It is uglier to use, but see what you think of philipc@473e138
On the writing side, the only support required is for reading it when converting. It won't write it out in the same form.
let (_, this) = entries.next_dfs()?.unwrap(); | ||
match this.attr_value(gimli::constants::DW_AT_GNU_dwo_id)? { | ||
Some(gimli::AttributeValue::DwoId(dwo_id)) => dwo_id, | ||
Some(_) => unreachable!(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What makes this unreachable? (and same for the other use below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dwoid!
only ever returns AttributeValue::DwoId
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but then it falls through to self.value.clone()
for other forms.
let mmap_ptr = mmap.as_ptr(); | ||
let mmap_len = mmap.len(); | ||
mem::forget(mmap); | ||
match object::File::parse(unsafe { slice::from_raw_parts(mmap_ptr, mmap_len) }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but an arena would be nicer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idk what this means in practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See philipc@e6e7557
fwiw, in Pernosco when processing .dwos we |
Or something you call after |
This set of commits makes
dwarfdump
more useful for dumping a .dwo, by allowing me to provide the path of the "parent" binary, and fixes a nasty bug in the handling of ranges in the pre-DWARF5 GNU DWO extension where we weren't doing the behavior that's documented at https://github.com/bminor/binutils-gdb/blob/fac3b6a2e04f7feeefbf425c3798c34d134752d6/gdb/dwarf2/cu.h#L189