[PATCH] D15976: Supporting all entities declared in lexical scope in LLVM debug info
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 2 14:52:45 PST 2016
dblaikie added inline comments.
================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:20
@@ -18,1 +19,3 @@
+static cl::opt<bool> AbstractOriginOnLS(
+ "use-abstract-origin-on-ls", cl::Hidden,
----------------
I'd really prefer not to have this option/backcompat feature.
Do you have a need to support older GDBs (or LLDB) with this change? Otherwise I think this is a sufficiently narrow case we could probably take the change optimistically and let other debuggers deal with it as a bug. I don't think it'll disrupt that many users. (though I could be wrong/happy to deal with it differently if we have contributing developers who feel strongly that they need a workaround for their users until their platform debuggers catch up)
I realize it doesn't cost too much (your implementation has made it quite simple to support both) - this isn't all teh cost. We'd have to plumb the option up through into the frontend, or make it a debugger tuning feature (how would we tune it for GDB if we want to support old GDBs here? I would imagine this should be a "do the good thing by default, but fall back to the bad thing if we're specifically targeting known bad compilers, like LLDB" - though I think Adrian mentioned he'd be willing to just take this as an LLDB bug, but I'm not 100% sure)
================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:573
@@ +572,3 @@
+ else
+ assert(false && "Unexpected DI node.");
+ DU->addLocalDclDieToLexicalScope(Scope, D);
----------------
This should be llvm_unreachable rather than assert(false)
================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:733
@@ +732,3 @@
+ // to the corresponding abstract local scope.
+ for (auto &L : LSInfo.InlineLSDies) {
+ addDIEEntry(*L, dwarf::DW_AT_abstract_origin, *LSInfo.AbstractLSDie);
----------------
aaboud wrote:
> dblaikie wrote:
> > dblaikie wrote:
> > > Drop {} from single line loop
> > It's possible that this part of the code (& the InlineLSDies member of LSInfo) is not needed - whenever we create an inline instance we /know/ there's a concrete instance and we know (I think) that it will have the abstract-only DIEs on it that mean it needs an abstract_origin, right? So if we put it on up-front, it'll save us having to cache the list of inlined instances, etc, perhaps?
> Could work, assuming that we created the abstract lexical scope before we create the inline lexical scope.
> However, to figure out that the lexical scope is an inline, we need to add a check that would not make the code clean.
>
> So, I suggest that I implement this suggestion as a follow up, after commenting this implementation.
> Then, you can give it a second thought to decide if you still want to stick with the suggestion, or stay with the current code.
Sure, that can wait
================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:467
@@ +466,3 @@
+ }
+ assert(false && "Unexpected scope.");
+ return nullptr;
----------------
aaboud wrote:
> dblaikie wrote:
> > replace assert(false) with llvm_unreachable (& drop the "return" after it - it's dead/not needed)
> I just figure out that I do not need this function.
> Just replaced it with DILocalScope::getSubprogram().
Great!
================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:561
@@ +560,3 @@
+ for (const auto &I : CUMap)
+ forBothCUs(*I.second, [&](DwarfCompileUnit &CU) {
+ CU.finishLocalScopeDefinitions();
----------------
aaboud wrote:
> dblaikie wrote:
> > What does the output of this loop look like for split-dwarf? I would expect that this change should have no impact on skeleton DIEs (including the GMLT-like DIEs produced as children of the skeleton CU DIE), yes?
> >
> > Indeed, it looks like DwarfCompileUnit::finishLocalScopeDefinitions only uses the contents of the DwarfFile anyway - so perhaps the function should be moved up to the DwarfFile level (rather than calling down into the DwarfCompileUnit, then up into the DwarfFile) & only run on the non-skeleton file since it is/should be a no-op on the skeleton unit?
> I admit, I have no clue how split-dwarf looks like.
> I guessed that I might need to handle it like I did here, but if you are convinced that it is redundant, I will follow your suggestion.
>
> Also, the "DwarfCompileUnit ::finishLocalScopeDefinitions()" method invokes the "DwarfUnit ::addDIEEntry()".
> If you still think that we move the "finishLocalScopeDefinitions()" method to DwarfFile, then how should we access the "DwarfUnit ::addDIEEntry()" method from there?
Could you try this out with split-dwarf? Or would you like me to take a look?
If we were to put it in DwarfFile, we would just specifically access the non-skeleton DwarfUnit to add DIEs, etc. there are other parts of the code that do this, if I recall correctly.
http://reviews.llvm.org/D15976
More information about the llvm-commits
mailing list