[PATCH] D136039: [DebugInfo] Fix potential CU mismatch for attachRangesOrLowHighPC

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

If we wanted to fix this whole class of errors, maybe it's time we moved most of the DIE creation operations back to DwarfDebug - and any unit-specific operations would then have to explicitly look up the right unit from the DIE before they could be used. This would reduce the chance of accidentally using the wrong unit? (it'd cause more unit lookups - which should only be walking up the DIE chain. - if we make sure every DIE is added to its parent early enough, which seems plausible)

I tried, for instance, removing the DwarfUnit lookup in `getOrCreateSubprogramDIE` where it calls `applySubprogramAttributes` and sinking that down into the places where it was needed - which was only, so far as test coverage would indicate, at the `getOrCreateSourceID` calls (since in LTO we do produce separate line tables for each CU - so looking up/adding the file name in the right CU is necessary). That then uncovered a few cases where that code was being used with un-parented DIEs - specifically imported entities and variables. I was able to add those to the right parents earlier with a few changes. https://reviews.llvm.org/P8297



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:450
+  if (!*ContextCU) {
+    *ContextCU = DD->lookupCU(SPDie->getUnitDie());
+  }
----------------
Could you use `static_cast<DwarfCompileUnit*>(SPDie->getUnit())` here, instead of `DD->lookup`? (looks like that's what `getOrCreateSubprogramDIE` does?

& maybe that's cheap enough to use up in the caller, and avoid having to thread the ContextCU up and down through this stack?

Maybe even just build that CU change into `attachRangesOrLowHighPC` the same way it's built into `getOrCreateSubprogramDIE`?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h:231
+  DIE &constructSubprogramScopeDIE(const DISubprogram *Sub, LexicalScope *Scope,
+                                   DwarfCompileUnit **ContextCU);
 
----------------
ContextCU should probably be passed by reference-to-pointer - since you've updated the two callers so they don't pass null, there's no need for the implementation to consider whether the caller might've passed null, I think?

Alternatively, I guess, the caller could perform the lookup again, since they got the DIE back as a result, right? Maybe that's better than threading this through all the construct* abstractions to get it back out again?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2266
         TheCU.getCUNode()->getSplitDebugInlining())
-      SkelCU->constructSubprogramScopeDIE(SP, FnScope);
+      SkelCU->constructSubprogramScopeDIE(SP, FnScope, &ContextSkelCU);
+  }
----------------
Guessing this addition is basically untested - could you add a Split DWARF test to cover this? (& verify that the test fails without these changes applied)


================
Comment at: llvm/test/DebugInfo/Generic/cross-cu-inlining-ranges.ll:4
+
+; We want to check that when the CU attaches ranges use the correct ContextCU.
+
----------------
Maybe some more detailed description of the particular circumstances where it's difficult to attach to the right context/explains why this test has the features it has?

(does it need a global variable? Does that global variable need a non-trivial type (if the global variable's necessary, but maybe it could be "int" rather than a structure type?) there's quite a few calls (both all the calls to `f` in `main`, and multiple in `foo`)

& maybe a snippet of source code to help explain the situation?

& I'd still be curious if this can be reproduced with C or C++/clang produced IR - might be more generally explicable/understandable by LLVM developers who all work in C++ at least.


================
Comment at: llvm/test/DebugInfo/Generic/cross-cu-inlining-ranges.ll:9-13
+  call void @f()
+  call void @f()
+  call void @f()
+  call void @f()
+  call void @f()
----------------
Any particular reason you need 5 calls rather than, say, 1?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136039/new/

https://reviews.llvm.org/D136039