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

[Dehao Chen via Phabricator via llvm-commits]

danielcdh added inline comments.


================
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));
----------------
dblaikie wrote:
> 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.
It does not change the behavior.

function name updated to make it clearer.

Also updated the implementation to make sure high addresses are filtered so that the behavior does not change.


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:393-397
+    while (SubprogramDIE) {
+      if (SubprogramDIE.isSubroutineDIE())
+        InlinedChain.push_back(SubprogramDIE);
+      SubprogramDIE = SubprogramDIE.getParent();
+    }
----------------
dblaikie wrote:
> 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)?
That's correct. Function name updated.


https://reviews.llvm.org/D32177