Page MenuHomePhabricator

This phabricator revision is the merge of 4 patches that aim to provide resolving of AT_abstract_origin and AT_specification attributes.
AbandonedPublic

Authored by friss on Sep 5 2014, 4:01 AM.

Details

Summary

The 4 patches are:

  • Fix DWARFContext::getCompileUnitForOffset()

    The comparator used for the search compares the beginning of the CU Offset, but this will return CUs.end() when the searched offset is in the last CU. Use the Unit end offset instead in the comparator. This requires some cleverness so that searching for a start offset will return the good unit.

    lib/DebugInfo/DWARFContext.cpp | 37 +++++++++++++++++++++----------------
  • Make DWARFContext::getCompileUnitForOffset() public.

    They will be used by DIE dump() to resolve reference attributes.

    lib/DebugInfo/DWARFContext.h | 2 +-
  • Handle cross-unit references in DWARFDebugInfoEntryMinimal::getSubroutineName().

    lib/DebugInfo/DWARFDebugInfoEntry.cpp | 7 +++++++
  • [dwarfdump] Print the name for referenced specification of abstract_origin DIEs.

    This way, when you have a inlined/template instance in the debug information, it's easy to find out wihtout searching what the instance origin.

    lib/DebugInfo/DWARFDebugInfoEntry.cpp | 14 ++++++++++++++ test/Linker/type-unique-odr-a.ll | 2 +-

Diff Detail

Event Timeline

friss updated this revision to Diff 13313.Sep 5 2014, 4:01 AM
friss retitled this revision from to This phabricator revision is the merge of 4 patches that aim to provide resolving of AT_abstract_origin and AT_specification attributes..
friss added reviewers: dblaikie, echristo, aprantl.
friss updated this object.
friss added a subscriber: Unknown Object (MLST).
friss added inline comments.Sep 5 2014, 4:09 AM
lib/DebugInfo/DWARFContext.cpp
392–411

This patch was meant to only fix the logic of the comparator, but by rebasing/reworking my patch queue, it got templatized along the way. It's not apparent in this patch series, but I have followups that introduce a similar lookup function for TypeUnits which will need the template form.

dblaikie added inline comments.Sep 5 2014, 12:51 PM
lib/DebugInfo/DWARFContext.cpp
392–411

Might be nice to have that templating go where it's needed rather than here.

For what it's worth, when I do end up in the unfortunate situation of a lot of code that needs to be submitted in smaller patches (something worth avoiding when possible - especially when the incremental patches need precommit review (though it applies similarly to post-commit review) - as in that case flooding a reviewer is a bit tricky compared to having them time slice in a small review here or there as you're making progress) I tend to just copy/paste/rewrite portions of the large patch, into a separate branch, and send that for review - less chance of things getting too jumbled up. But once or twice I've managed to juggle my own incremental patch work into something reviewable.

401

I have a sneaking suspicion that <= ordering would violate the requirements of lower_bound...

I /suspect/ the right answer is:

Off < RHS->getOffset()

LHS->getNextUnitOffset() <= Off

LHS->getNextUnitOffset() <= RHS->getOffset()

That should be a proper "less than" for a half open range [offset, next unit offset), I think... maybe?

lib/DebugInfo/DWARFDebugInfoEntry.cpp
105

This needs some test coverage (the one test update seems like it wouldn't start failing if this code was all reverted/broken tomorrow & doesn't cover all the cases here - testing specification and abstract origin, testing cases where the name isn't available, etc)

I'm rather surprised more tests didn't need updating - we should have a bunch that test abstract_origin and specification values...

110

Roll the variable declaration into the 'if' here (& then you can drop the braces on the outer "if (DIE.extractFast" if you like)

319

Is "getCompileUnitForOffset" cheap enough that we should just always call it? (& maybe pass in the context to this function, rather than the DWARFUnit, at that point)

IIRC, originally getCompileUnitForOffset() was supposed to work only for the exact Offsets (which are returned by DWARFUnit::getOffset()). Now that you're changing it to work for arbitrary offsets, I think it makes more sense to rewrite it as follows:

if (CUs.empty()) return nullptr;
auto It = std::upper_bound(CUs.begin(), CUs.end(), Offset, OffsetComparator());
if (It == CUs.begin()) return nullptr;
--It;
return (It->getOffset <= Offset && Offset < It->getNextUnitOffset()) ? It->get() : nullptr;

and leave the comparator as is. I would rather not play with non-strict comparisons.

samsonov added inline comments.Sep 5 2014, 1:54 PM
lib/DebugInfo/DWARFDebugInfoEntry.cpp
319

This part makes no sense to me. We definitely should somehow ensure that "this" is in a fact a DIE in "U". When you call DWARFDebugInfoEntry::getSubroutineName(), you certainly should know which unit this DIE belongs to. How can it be not so?

dblaikie added inline comments.Sep 5 2014, 1:57 PM
lib/DebugInfo/DWARFDebugInfoEntry.cpp
319

LLVM under LTO will generate DIE references across the CUs (in an effort to reduce debug info size - eg: rather than putting a type (even a declaration) in multiple CUs, one CU just references directly into the other CU to reference the type defined there) - so you actually don't know.

samsonov added inline comments.Sep 5 2014, 2:17 PM
lib/DebugInfo/DWARFDebugInfoEntry.cpp
319

OK, makes sense. But you still are supposed to know which Unit a given DIE belongs to. You can't even extract it otherwise: DWARFDebugInfoEntry::extractFast() takes a pointer to Unit as an input parameter. So, in case of cross-CU references, and, for instance, getSubroutineName you should do the following:

  1. assume "this" is a DIE contained in "U".
  2. if DIE has name attribute, return it
  3. try to fetch name from specification: a) read DW_AT_specification reference SpecRef b) calculate SpecU this SpecRef corresponds to c) call SpecDIE.extractFast(SpecU, &SpecRef) d) call SpecDIE.getSubroutineName(SpecU)
dblaikie added inline comments.Sep 7 2014, 9:05 PM
lib/DebugInfo/DWARFDebugInfoEntry.cpp
319

@samsanov:

Yeah, what you're suggesting is something along the lines of what I was trying to describe when I said:

"Is "getCompileUnitForOffset" cheap enough that we should just always call it? (& maybe pass in the context to this function, rather than the DWARFUnit, at that point)" - essentially, as you described in (3(b)) - always compute the CU from the ref, rather than assuming/verifying if it's the same as the current one.

samsonov added inline comments.Sep 8 2014, 10:47 AM
lib/DebugInfo/DWARFDebugInfoEntry.cpp
319

I still see no benefit in passing DWARFContext here - you will always know the DWARFUnit for the DIE before you can issue the call to getSubroutineName, why not pass this DWARFUnit there, and keep it consistent with the rest various DWARFDebugInfoEntryMinimal accessors?

friss abandoned this revision.Sep 18 2014, 5:21 AM

The underlying code has changed too much and the description doesn't make sense anymore. Abandon this revision