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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:657
+                                                DIE &ParentScopeDIE,
+                                                DwarfCompileUnit *ContextCU) {
   assert(Scope->getScopeNode());
----------------
Looks like some of these sites didn't get updated to 


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1053
+                                                 DIE &ScopeDIE,
+                                                 DwarfCompileUnit *ContextCU) {
   DIE *ObjectPointer = nullptr;
----------------
This whole chain that takes the ContextCU by pointer (rather than ref to pointer) - as an input only parameter, probably doesn't neeed to be changed. It should probably call with `ContextCU` as `this` instead, eg: somewhere up the stack, it should be `ContextCU->...` insntead of `otherCU->...(..., ContextCU);`?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:450
+  if (!*ContextCU) {
+    *ContextCU = DD->lookupCU(SPDie->getUnitDie());
+  }
----------------
DianQK wrote:
> dblaikie wrote:
> > 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`?
> Probably not, after trying your patch, I can still find some cases in `attachRangesOrLowHighPC` where the unit does not exist. Maybe we need more changes? 
> 
> And `getUnit` always return `DwarfCompileUnit` in `attachRangesOrLowHighPC`? I'm not quite sure about the logic here yet.
> Probably not, after trying your patch, I can still find some cases in `attachRangesOrLowHighPC` where the unit does not exist. Maybe we need more changes?

But each of the places where it doesn't have a unit is a risk that it gets attached to the wrong thing, right? (but not a guaranteed bug, so yeah, maybe not the thing we should hold this all up on)

> And `getUnit` always return `DwarfCompileUnit` in `attachRangesOrLowHighPC`? I'm not quite sure about the logic here yet.

Yeah, that should be fine - DwarfTypeUnits can't have address ranges, so anything we're attaching ranges to should be a CU.

In any case, could we still avoid all the `DwarfCompileUnit *&ContextCU` additions and have the caller in `DwarfDebug::endFunctionImpl` lookup the `DwarfCompileUnit` from the returned DIE the same way it's being done here ^? It'd avoid a lot of threading things back and forth through all these construct APIs?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2266
         TheCU.getCUNode()->getSplitDebugInlining())
-      SkelCU->constructSubprogramScopeDIE(SP, FnScope);
+      SkelCU->constructSubprogramScopeDIE(SP, FnScope, ContextSkelCU);
+  }
----------------
Also, I think the right thing to do here is to call `constructSubprogramScopeDIE` on the `ContextSkelCU` maybe, rather than on `SkelCU`?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2266
         TheCU.getCUNode()->getSplitDebugInlining())
-      SkelCU->constructSubprogramScopeDIE(SP, FnScope);
+      SkelCU->constructSubprogramScopeDIE(SP, FnScope, &ContextSkelCU);
+  }
----------------
dblaikie wrote:
> 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)
Ping here ^


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136039