[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 accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

I'm pretty OK with this - haven't thought /deeeply/ about the algorithm from the code (more from the discussions we've had), mostly glossed over, looked at the code style, etc, seems plausible.

Wonder if it's worth doing some kind of stress test for this? (take an optimized clang, symbolize all the addresses, compare before/after this change - that would've likely caught the "there can be non-subroutine scopes that need to be recursed through" case I brought up in the first pass of review & maybe some others I've not spotted?)



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:496
+
+    // For non-subroutines, just recurse to find any nested subroutines.
+    if (Die.getTag() != DW_TAG_inlined_subroutine) {
----------------
I know the naming here is subtle/annoying, but maybe "inlined subroutines" rather than "nested subroutines" would make it more clear what we're searching for?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:521-527
+          (R.LowPC - BasePC) > (uint64_t)std::numeric_limits<uint32_t>::max()
+              ? std::numeric_limits<uint32_t>::max()
+              : (uint32_t)(R.LowPC - BasePC);
+      uint32_t RelativeHighPC =
+          (R.HighPC - BasePC) > (uint64_t)std::numeric_limits<uint32_t>::max()
+              ? std::numeric_limits<uint32_t>::max()
+              : (uint32_t)(R.HighPC - BasePC);
----------------
JDevlieghere wrote:
> Maybe extract `(uint64_t)std::numeric_limits<uint32_t>::max()` to make this a little more readable?
> 
> Not relevant here (as it actually improves readability) but I wonder what the consensus is on C-style casts in LLVM? I don't think the style guide actually mentions it.
Worth pulling out a named constant for std::numeric_limits<uint32_t>::max() ? it's a long expression used 4 times in these long/wrapped lines.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:571-573
+        const uint32_t IntervalStart = AddrMap[i].first;
+        const uint32_t IntervalEnd = AddrMap[i + 1].first;
+        const int IntervalDieIdx = AddrMap[i].second;
----------------
top level const on locals is a bit uncommon (don't have to change it, just mentioning it in case there's some special motivation, or in case the choice is worth revisiting)


https://reviews.llvm.org/D40987