[PATCH] D40987: Rewrite the cached map used for locating the most precise DIE among inlined subroutines for a given address.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Code mostly looks plausible. Haven't quite understood the "ParentIntervals*" variables/processing.



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:358-359
+    if (!Die.isSubprogramDIE()) {
+      for (DWARFDie Child = Die.getFirstChild(); Child;
+           Child = Child.getSibling())
+        Worklist.push_back(Child);
----------------
  for(DWARFDie Child : Die.children())


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:371-382
+    SubprogramDIEAddrInfos.push_back({Die, (uint64_t)-1, {}});
     for (const auto &R : Die.getAddressRanges()) {
       // Ignore 0-sized ranges.
       if (R.LowPC == R.HighPC)
         continue;
-      auto B = AddrDieMap.upper_bound(R.LowPC);
-      if (B != AddrDieMap.begin() && R.LowPC < (--B)->second.first) {
-        // The range is a sub-range of existing ranges, we need to split the
-        // existing range.
-        if (R.HighPC < B->second.first)
-          AddrDieMap[R.HighPC] = B->second;
-        if (R.LowPC > B->first)
-          AddrDieMap[B->first].first = R.LowPC;
+
+      SubprogramDIEAddrMap.push_back({R.LowPC, DIEIndex});
----------------
This does slightly change the claim in the patch description about only 2GB functions being a problem. Since this at least in theory supports fragmented functions, even a small function with, say split into a hot and cold section, could end up with the hot part 2GB of address away from the cold part perhaps more readily/likely than a 2GB function existing.

But still probably nothing to worry about.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:384-388
+    // Also recurse into any children DIEs that are also subprogram DIEs.
+    for (DWARFDie Child = Die.getFirstChild(); Child;
+         Child = Child.getSibling())
+      if (Child.isSubprogramDIE())
+        Worklist.push_back(Child);
----------------
subprograms that are children of other subprograms might be arbitrarily nested - not only direct children (looks like this code only handles direct children?)

(though there's no /good/ reason they would be nested inside an inlined subroutine)

eg: you could have code like:

  void f1() {
    if (int x = ...) {
      void f2() { ... }
      ... 
    }
  }

in which case, you could have DWARF like:

  DW_TAG_subprogram
    DW_AT_name "f1"
    DW_TAG_lexical_block
      DW_TAG_subprogram
        DW_AT_name "f2"

which, yes, does mean you have to walk all the DIEs to find all the subprograms... not sure how much that defeats the goals here (I guess not having to build up so much in the way of data structures while doing so is still advantageous).


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:457
+    // If the start addresses are exactly the same, we can
+    // remove all but the first one as it is the only one that
+    // will be found and used.
----------------
What's the "first" in this case - is it deterministically the most nested? Least nested? Unspecified?

We'd need to make sure it's the most nested.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:471
+  SmallVector<DieAndParentIntervalRange, 16> Worklist;
+  auto EnqueueChildDIEs = [&Worklist](const DWARFDie &Die,
+                                      int ParentIntervalsBeginIdx,
----------------
Nit: Usually I'm of the opinion if a lambda doesn't leak out of scope (via a std::function or similar), just use [&] rather than an explicit list. Same as a loop/conditional/etc doesn't need documentation about which variables are used in the scope. One more thing to have to touch if the code changes, especially since there's a -Wunused-lambda-capture that comes up a bit on the bots regularly.

But up to you.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:474-475
+                                      int ParentIntervalsEndIdx) {
+    for (DWARFDie Child = Die.getFirstChild(); Child;
+         Child = Child.getSibling())
+      Worklist.push_back(
----------------
  for (DWARFDie Child : Die)


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:482
+    DWARFDie Die = Worklist.back().Die;
+    ;
+    int ParentIntervalsBeginIdx = Worklist.back().ParentIntervalsBeginIdx;
----------------
Spurious semi


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:507-509
+      // Clamp the PCs to be above the base.
+      R.LowPC = std::max(R.LowPC, BasePC);
+      R.HighPC = std::max(R.HighPC, BasePC);
----------------
This is just for bad input, I take it? (where the address range of an inlined subroutine is outside that of the enclosing subprogram)


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:535-536
+
+    // The PCs from this DIE should never overlap, so we can easily sort them
+    // here.
+    std::sort(AddrMap.begin() + DieIntervalsBeginIdx, AddrMap.end(),
----------------
What's the behavior if they do overlap? While the DWARF spec & a reasonable person would say they can't, physically the data could contain such ranges. Will this crash? Have unreliable behavior? What's reasonable on invalid input here?

(it's probably fine, just checking it's articulated)


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:561
+      auto ParentIntervalIdx = ParentIntervalsBeginIdx;
+      for (int i = DieIntervalsBeginIdx, e = DieIntervalsEndIdx - 1; i < e;
+           ++i) {
----------------
Nit: Usually I see "i != e" in C++ code. Is < preferred?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:618-620
+        if (IntervalEnd < ParentIntervalEnd) {
+          AddrMap.push_back({IntervalEnd, ParentIntervalDieIdx});
+        }
----------------
Nit: {} on a single line scope.


https://reviews.llvm.org/D40987