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(debuginfo): Inlinees management in DWARF #237

Merged
merged 10 commits into from
May 25, 2020

Conversation

calixteman
Copy link
Contributor

@calixteman calixteman commented May 19, 2020

It aims to fix:

  • addresses which don't belong to the inlinee (it's due to the fact that in get_rows we get row just before inlinee start address),
  • the inlinee addresses in the parent.

So the idea is basically to get rows for non-inlined functions and then for an inlinee get line info from the parent according to the ranges we've for the inlinee.
I let a TODO, I'll fix in a follow-up patch: some non-inlined functions can have several range which messes up the computation of size/end.

Fixes #228
Fixes #194
Fixes #193

Ref #223

@calixteman calixteman requested a review from a team May 19, 2020 18:08
@jan-auer
Copy link
Member

@calixteman Thanks for getting this started. I'm not so sure about the changes in test snapshots, since they show line records that seem completely out of line.

I think that the change you were doing in #200 is the actual fix, essentially removing this check here:

if last == Some((row.file_index, line)) {
continue;
}

That way, we should no longer get overlapping ranges (DWARF usually splits ranges in the parent when there's an inlinee boundary), and all the complicated logic from #223 should no longer be necessary.

@calixteman
Copy link
Contributor Author

calixteman commented May 20, 2020

I didn't pay attention to the tests but there is something obviously wrong here.
Anyway the patch aims to fix too the spurious addresses which out-of-range: it's due the fact that get_rows gets a row were the range is inside and then the row.begin is used (probably it should be max(row.begin, range.begin)).

And there is the issue with non-inlined functions on several ranges to fix too.

@calixteman
Copy link
Contributor Author

There is still something in the tests... let me fix that

@calixteman
Copy link
Contributor Author

Normally this patch should fix what you tried to fix in #223.
And it fixes too the spurious out-of-range lines and I think we've at the end better informations.
So if you see a way to make this code simpler, then I'd be happy.
Anyway we need to find a way to handler non-inlined function on several ranges.

Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

Thank you so much for this PR. Checking ranges for patching the parent is indeed the correct approach, anything else would just change too many records or overwrite wrong ones, as soon as inlinees overlap.

debuginfo/src/dwarf.rs Outdated Show resolved Hide resolved
debuginfo/src/dwarf.rs Outdated Show resolved Hide resolved
debuginfo/src/dwarf.rs Outdated Show resolved Hide resolved
debuginfo/src/dwarf.rs Show resolved Hide resolved
@jan-auer jan-auer changed the title [debuginfo] Fix inlinees management in dwarf.rs fix(debuginfo): Inlinees management in DWARF May 25, 2020
Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

❤️ Thanks a bunch for this and the great collaboration!

@jan-auer
Copy link
Member

This last commit fixes the Python tests. When splitting a line info record in two, we must not skip the second half, since this one may need to be split further. The only two cases where skipping is valid is when adding records at the border of a range (since those come out of nowhere).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants