[PATCH] D15976: Supporting all entities declared in lexical scope in LLVM debug info

[Amjad Aboud via llvm-commits]

aaboud added a comment.

Sorry for the late response, please see new patch and below 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,
----------------
dblaikie wrote:
> 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)
OK, I agree.
I just wanted to show you (record) the changes need to be done to support older debuggers.
I will stick to the new behavior, and assume LLDB will have a fix soon to suit this new behavior.

If we get any complains from Clang users we can reconsider support the workaround.

Do you agree to this?

================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:547
@@ +546,3 @@
+  for (const auto &I : CUMap)
+    forBothCUs(*I.second, [&](DwarfCompileUnit &CU) {
+      CU.finishLocalScopeDefinitions();
----------------
OK, I updated the test to run also with "split-dwarf=ENABLE", and even check manually against older behavior, and in both cases the debug info is emitted to the ".debug_dwo" section.

So, I guess the current implementation is fine, what do you think?


http://reviews.llvm.org/D15976