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

[David Blaikie]

================
Comment at: lib/DebugInfo/DWARFContext.cpp:392-411
@@ -391,18 +391,22 @@
 
 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>
+struct OffsetComparator {
+  // 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,
+                  uint32_t RHS) const {
+    return LHS->getNextUnitOffset() <= RHS;
+  }
+  bool operator()(uint32_t LHS,
+                  const std::unique_ptr<UnitType> &RHS) const {
+    return LHS <= RHS->getNextUnitOffset();
+  }
+};
 }
----------------
friss wrote:
> 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.
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.

================
Comment at: lib/DebugInfo/DWARFContext.cpp:401
@@ +400,3 @@
+                  const std::unique_ptr<UnitType> &RHS) const {
+    return LHS->getNextUnitOffset() <= RHS->getNextUnitOffset();
+  }
----------------
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?

================
Comment at: lib/DebugInfo/DWARFDebugInfoEntry.cpp:105
@@ +104,3 @@
+  // with something more descriptive.
+  if (attr == DW_AT_specification ||
+      attr == DW_AT_abstract_origin) {
----------------
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... 

================
Comment at: lib/DebugInfo/DWARFDebugInfoEntry.cpp:110
@@ +109,3 @@
+    if (DIE.extractFast(u, &Ref)) {
+      const char *Name = DIE.getSubroutineName(u, FunctionNameKind::LinkageName);
+      if (Name)
----------------
Roll the variable declaration into the 'if' here (& then you can drop the braces on the outer "if (DIE.extractFast" if you like)

================
Comment at: lib/DebugInfo/DWARFDebugInfoEntry.cpp:319
@@ +318,3 @@
+      Offset >= U->getNextUnitOffset()) {
+    U = U->getContext().getCompileUnitForOffset(Offset);
+    assert(U && "Invalid DIE referenced.");
----------------
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)

http://reviews.llvm.org/D5208