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

[DianQK via Phabricator via llvm-commits]

DianQK marked an inline comment as done.
DianQK added inline comments.


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


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:450
+  if (!*ContextCU) {
+    *ContextCU = DD->lookupCU(SPDie->getUnitDie());
+  }
----------------
dblaikie wrote:
> 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?
> 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)
Yeah, so even if it's not a bug, bug if any DIE can have a unit (add by parent chain?) after `DIE::get(DIEValueAllocator, foo)`, any subsequent operations on the `unit` will be friendly. So for this change, maybe we can create another Diff?

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

> 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?
I will try later.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2266
         TheCU.getCUNode()->getSplitDebugInlining())
-      SkelCU->constructSubprogramScopeDIE(SP, FnScope);
+      SkelCU->constructSubprogramScopeDIE(SP, FnScope, &ContextSkelCU);
+  }
----------------
dblaikie wrote:
> 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 ^
I didn't miss it here. (I just don't have time to double-check this. During the working day, I may only have an hour on this thing.)
> Maybe I also need to understand how a Split DWARF would be different.

I can check all the "Done" checkboxes to determine if I've missed something.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136039