Skip to content
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

Merged
merged 3 commits into from
Jul 16, 2021

Conversation

khuey
Copy link
Contributor

@khuey khuey commented Jul 14, 2021

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

…resented by DW_FORM_sec_offset but need to be adjusted by the rnglist_base value to find the proper values in the binary.
Copy link
Collaborator

@philipc philipc left a 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))
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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!(),
Copy link
Collaborator

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)

Copy link
Contributor Author

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

Copy link
Collaborator

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) }) {
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@khuey
Copy link
Contributor Author

khuey commented Jul 15, 2021

It seems like the dwarfdump dwo changes include things that we eventually should support in the library itself somehow.

fwiw, in Pernosco when processing .dwos we Dwarf::load the .dwo file and then replace debug_addr, debug_line, debug_line_str, and range_lists (if and only if the .dwo does not have a debug_rnglists) with those sections from the parent. I feel like it's a bit difficult to push this into gimli since object loading lives entirely outside it. I suppose you could have a Dwarf::load variant that takes two functions?

@philipc
Copy link
Collaborator

philipc commented Jul 15, 2021

I suppose you could have a Dwarf::load variant that takes two functions?

Or something you call after Dwarf::load that does the replacements, similar to load_sup. Not something that needs changing in this PR though.

@philipc philipc merged commit 7c13056 into gimli-rs:master Jul 16, 2021
@philipc philipc mentioned this pull request Jul 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants