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

[friss]

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 +-

[friss]

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]

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)

[samsonov]

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]

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]

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]

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]

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]

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]

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

lib/DebugInfo/DWARFContext.h

public:
   virtual StringRef getStringDWOSection() = 0;
   virtual StringRef getStringOffsetDWOSection() = 0;
   virtual StringRef getRangeDWOSection() = 0;
   virtual StringRef getAddrSection() = 0;
 
   static bool isSupportedVersion(unsigned version) {
     return version == 2 || version == 3 || version == 4;
   }
-private:
+
   /// Return the compile unit that includes an offset (relative to .debug_info).
   DWARFCompileUnit *getCompileUnitForOffset(uint32_t Offset);
 
   /// Return the compile unit which contains instruction with provided
   /// address.
   DWARFCompileUnit *getCompileUnitForAddress(uint64_t Address);
 };
 

lib/DebugInfo/DWARFContext.cpp

while (DIData.isValidOffset(offset)) {
       if (!TU->extract(DIData, &offset))
         break;
       DWOTUs.push_back(std::move(TU));
       offset = DWOTUs.back()->getNextUnitOffset();
     }
   }
 }
 
 namespace {
-  struct OffsetComparator {
-
-    bool operator()(const std::unique_ptr<DWARFCompileUnit> &LHS,
-                    const std::unique_ptr<DWARFCompileUnit> &RHS) const {
-      return LHS->getOffset() < RHS->getOffset();
-    }
-    bool operator()(const std::unique_ptr<DWARFCompileUnit> &LHS,
-                    uint32_t RHS) const {
-      return LHS->getOffset() < RHS;
-    }
-    bool operator()(uint32_t LHS,
-                    const std::unique_ptr<DWARFCompileUnit> &RHS) const {
-      return LHS < RHS->getOffset();
-    }
-  };
+template<typename UnitType>
+  // The beginning offset of a CU is the NextUnitOffset of the
+  // preceding one. When searching for this particular offset, the
+  // comparator has to return true so that lower_bound skips the
+  // CU. Thus the <= comparison.
+  bool operator()(const std::unique_ptr<UnitType> &LHS,
+                  const std::unique_ptr<UnitType> &RHS) const {
+    return LHS->getNextUnitOffset() <= RHS->getNextUnitOffset();
+  bool operator()(const std::unique_ptr<UnitType> &LHS,
+    return LHS->getNextUnitOffset() <= RHS;
+                  const std::unique_ptr<UnitType> &RHS) const {
+    return LHS <= RHS->getNextUnitOffset();
 }
 
 DWARFCompileUnit *DWARFContext::getCompileUnitForOffset(uint32_t Offset) {
   parseCompileUnits();
 
   std::unique_ptr<DWARFCompileUnit> *CU =
-      std::lower_bound(CUs.begin(), CUs.end(), Offset, OffsetComparator());
+      std::lower_bound(CUs.begin(), CUs.end(), Offset,
+                       OffsetComparator<DWARFCompileUnit>());
   if (CU != CUs.end()) {
     return CU->get();
   }
   return nullptr;
 }
 
 DWARFCompileUnit *DWARFContext::getCompileUnitForAddress(uint64_t Address) {
   // First, get the offset of the compile unit.

lib/DebugInfo/DWARFDebugInfoEntry.cpp

void DWARFDebugInfoEntryMinimal::dumpAttribute(raw_ostream &OS,
   if (Name) {
     OS << Name;
   } else if (attr == DW_AT_decl_line || attr == DW_AT_call_line) {
     OS << *formValue.getAsUnsignedConstant();
   } else {
     formValue.dump(OS, u);
   }
 
+  // We dumped the attribute value potentially as raw data. Add some
+  // special handling for cases where we want to annotate the raw data
+  // with something more descriptive.
+  if (attr == DW_AT_specification ||
+      attr == DW_AT_abstract_origin) {
+    uint32_t Ref = formValue.getAsReference(u).getValue();
+    DWARFDebugInfoEntryMinimal DIE;
+    if (DIE.extractFast(u, &Ref)) {
+      const char *Name = DIE.getSubroutineName(u, FunctionNameKind::LinkageName);
+      if (Name)
+        OS << '\"' << Name << '\"';
+    }
+  }
+
   OS << ")\n";
 }
 
 bool DWARFDebugInfoEntryMinimal::extractFast(const DWARFUnit *U,
                                              uint32_t *OffsetPtr) {
   Offset = *OffsetPtr;
   DataExtractor DebugInfoData = U->getDebugInfoExtractor();
   uint32_t UEndOffset = U->getNextUnitOffset();
bool DWARFDebugInfoEntryMinimal::addressRangeContainsAddress(
   return false;
 }
 
 const char *
 DWARFDebugInfoEntryMinimal::getSubroutineName(const DWARFUnit *U,
                                               FunctionNameKind Kind) const {
   if (!isSubroutineDIE() || Kind == FunctionNameKind::None)
     return nullptr;
+
+  if (Offset < U->getOffset() ||
+      Offset >= U->getNextUnitOffset()) {
+    U = U->getContext().getCompileUnitForOffset(Offset);
+    assert(U && "Invalid DIE referenced.");
+  }
+
   // Try to get mangled name only if it was asked for.
   if (Kind == FunctionNameKind::LinkageName) {
     if (const char *name =
             getAttributeValueAsString(U, DW_AT_MIPS_linkage_name, nullptr))
       return name;
     if (const char *name =
             getAttributeValueAsString(U, DW_AT_linkage_name, nullptr))
       return name;

test/Linker/type-unique-odr-a.ll

 ; CHECK:      DW_TAG_subprogram
 ; CHECK-NOT: DW_TAG
 ; CHECK:   DW_AT_MIPS_linkage_name {{.*}} "_Z3bazv"
 ; CHECK:      DW_TAG_subprogram
 ; CHECK-NOT: DW_TAG
 ; CHECK:   DW_AT_MIPS_linkage_name {{.*}} "_ZL3barv"
 
 ; getFoo and A may only appear once.
-; CHECK-NOT:  {{(getFoo)|("A")}}
+; CHECK-NOT:  AT_name{{.*(getFoo)|("A")}}
 
 
 ; ModuleID = 'type-unique-odr-a.cpp'
 
 %class.A = type { i32 }
 
 ; Function Attrs: nounwind
 define void @_Z3bazv() #0 {