Page MenuHomePhabricator

[DWARF5] Fix offset check when using .debug_names

Authored by kimanh on Jul 19 2021, 6:04 AM.



When going through the CU entries in the name index,
make sure to compare the name entry's CU
offset against the skeleton CU's offset.

Previously there would be a mismatch, since the
wrong offset was compared, and thus no suitable
entry was found.

Diff Detail

Event Timeline

kimanh created this revision.Jul 19 2021, 6:04 AM
kimanh added a subscriber: pfaffe.Jul 19 2021, 6:06 AM
kimanh published this revision for review.Jul 19 2021, 6:23 AM
kimanh edited the summary of this revision. (Show Details)
kimanh added a reviewer: labath.
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2021, 6:24 AM
JDevlieghere added inline comments.Jul 19 2021, 9:24 AM

I assume the const of the DWARFUnit is being dropped because GetNonSkeletonUnit? Rather than doing that, can't that function be const instead?

kimanh edited the summary of this revision. (Show Details)Jul 19 2021, 10:50 PM
kimanh added inline comments.Jul 19 2021, 11:06 PM

Yes correct. I agree with your point that dropping const here is not very nice, but I'm not sure whether we can make that function const easily.
As is GetNonSkeletonUnit is updating itself in ExtractUnitDIEIfNeeded(), which kicks off many non-const operations for updating itself. As far as I understand, if we would want to make it const, we'd need to change the architectural logic in how the skeleton and non-skeleton units are connected. I'm not very familiar with the code base, so any advice here is welcome!

Ping on this thread, any comment/guidance on this would help us a lot!

shafik added a subscriber: shafik.Jul 26 2021, 4:20 PM
shafik added inline comments.

So is the problem that calling GetNonSkeletonUnit() does not work here b/c in DebugNamesDWARFIndex case we don't want this but in the other cases we do?

kimanh added inline comments.Jul 26 2021, 11:55 PM

Yes, that's correct!

jankratochvil accepted this revision.Aug 6 2021, 10:43 AM



I see ld.lld is being run directly even in other testcases so OK.

This revision is now accepted and ready to land.Aug 6 2021, 10:45 AM
This revision was landed with ongoing or failed builds.Aug 9 2021, 4:39 AM
This revision was automatically updated to reflect the committed changes.
kimanh added a comment.Aug 9 2021, 4:39 AM

Thanks for the review! Landing now.