[PATCH] D32177: Using address range map to speedup finding inline stack for address.

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:361
+  }
+  // Parent DIEs are added to the AddrDieMap prior to the Children DIEs.
+  for (DWARFDie Child = Die.getFirstChild(); Child; Child = Child.getSibling())
----------------
Probably more important to explain why this is done, rather than the fact it is done (the fact is mostly evident from the code, the motivation less-so). (I take it it's done this way to simplify the implementation - so that child ranges overwrite parent ranges - rather than having to implement a non-overriding splitting form for parent ranges to not stomp on child ranges)


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:367-375
 DWARFUnit::getSubprogramForAddress(uint64_t Address) {
   extractDIEsIfNeeded(false);
-  for (const DWARFDebugInfoEntry &D : DieArray) {
-    DWARFDie DIE(this, &D);
-    if (DIE.isSubprogramDIE() &&
-        DIE.addressRangeContainsAddress(Address)) {
-      return DIE;
-    }
-  }
-  return DWARFDie();
+  if (AddrDieMap.empty())
+    for (const DWARFDebugInfoEntry &D : DieArray)
+      // Traverse top-level DIEs to avoid duplicated nodes.
+      if (D.getDepth() == 0)
+        updateAddressDieMap(DWARFDie(this, &D));
----------------
It looks like this changes the implementation - getSubprogramForAddress only ever returns subprogram DIEs (so no need for recursing down through children, nor for splitting ranges), but the address map contains both subprogram and inlined subroutine DIEs. Seems like a change in behavior - if intentional, that would warrant some tests to demonstrate the new behavior.


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:370-373
+    for (const DWARFDebugInfoEntry &D : DieArray)
+      // Traverse top-level DIEs to avoid duplicated nodes.
+      if (D.getDepth() == 0)
+        updateAddressDieMap(DWARFDie(this, &D));
----------------
I think these 3 lines can be replaced with:

  updateAddressDieMap(getUnitDIE());

Probably?


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:393-397
+    while (SubprogramDIE) {
+      if (SubprogramDIE.isSubroutineDIE())
+        InlinedChain.push_back(SubprogramDIE);
+      SubprogramDIE = SubprogramDIE.getParent();
+    }
----------------
What's this change for?

Ah, I guess this is the other half of the changed contract of getSubprogramForAddress?

Perhaps it'd be good to rename getSubprogramForAddress to getSubroutineForAddress to reflect the change in semantics (& check all the existing callers are OK with that)?


https://reviews.llvm.org/D32177