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

[DianQK via Phabricator via llvm-commits]

DianQK added a comment.

In D136039#3861089 <https://reviews.llvm.org/D136039#3861089>, @dblaikie wrote:

> 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)

Thank you very much for your patient and detailed comments.
I think it's a good idea.

And I solved part of the problem. The rest may need more additional time (Maybe tomorrow? I'm not sure about my free time).



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:450
+  if (!*ContextCU) {
+    *ContextCU = DD->lookupCU(SPDie->getUnitDie());
+  }
----------------
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.


================
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.
+
----------------
dblaikie wrote:
> 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.
Thanks to `cross-cu-inlining-2.ll`, I did it! I have removed the global variable.

The global variable is used to ensure a fixed order of DIE creation.
(Just because I found this issue from the example of global variables.)

I'm not sure if this can be reproduced with C or C++/clang produced IR. (I'll try again later.)


================
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()
----------------
dblaikie wrote:
> Any particular reason you need 5 calls rather than, say, 1?
I use this when debugging something. I deleted those.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136039