Page MenuHomePhabricator

Supporting all entities declared in lexical scope in LLVM debug info
ClosedPublic

Authored by aaboud on Jan 7 2016, 3:30 PM.

Details

Summary

This is the full implementation in LLVM of the proposal:
http://lists.llvm.org/pipermail/llvm-dev/2015-December/093313.html

Notice that it contains also the changes from D12426 & D12913.
If it helps, I can do the commits separately, but I would need approval for all three patches in order to do so.

In addition to the few LIT tests I added, I needed to fix few others to affect the changes, including entry order changes that occurred.

Diff Detail

Repository
rL LLVM

Event Timeline

aaboud updated this revision to Diff 44269.Jan 7 2016, 3:30 PM
aaboud retitled this revision from to Supporting all entities declared in lexical scope in LLVM debug info.
aaboud updated this object.
aaboud set the repository for this revision to rL LLVM.
aaboud added a subscriber: llvm-commits.
dblaikie edited edge metadata.Jan 28 2016, 6:46 PM

This is looking really good and/or I'm starting to get the hang of what's going on here (& I think our design discussions have converged a lot too - I can see all the code I expect to see, delaying adding entities to the scopes until we know whether to add it to the concrete or abstract, adding in the abstract_origin as well, etc)

A few practical comments, but I think we're pretty close to having this sorted out. Thanks, as always, for your patience!

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
573 ↗(On Diff #44269)

Rename "HasNoneScopeChild" to "HasNonScopeChild" (or "HasNonScopeChildren") - that took me a while to figure out what it was trying to express, but I think that's what you were going for, "none" as in "not any of" wasn't intended, but "has non-scope child" (children that are something other than a scope) is - then it make sense :)

713 ↗(On Diff #44269)

drop all the parens around the variables here

DIE *LBDie = LSInfo.AbstractLSDie ? LSInfo.AbstractLSDie : LSInfo.ConcreteLSDie;
716 ↗(On Diff #44269)

Drop {} from single line loop

733 ↗(On Diff #44269)

Drop {} from single line loop

733 ↗(On Diff #44269)

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?

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
467 ↗(On Diff #44269)

replace assert(false) with llvm_unreachable (& drop the "return" after it - it's dead/not needed)

500 ↗(On Diff #44269)

Can drop the {} here if you like (as noted below, this isn't a certain/consistent rule in the LLVM codebase)

523 ↗(On Diff #44269)

Can drop the {} if you like - this one is less clear. Some parts/developers of LLVM drop {} around any single statement block (anywhere it's valid, basically), some only drop them around single /line/ blocks.

561 ↗(On Diff #44269)

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?

1297 ↗(On Diff #44269)

Drop {} from single line block

lib/CodeGen/AsmPrinter/DwarfFile.h
66 ↗(On Diff #44269)

You can just use in class initializers for this:

DIE *ConcreteLSDie = nullptr;
DIE *AbstractLSDie = nullptr;
aaboud marked 8 inline comments as done.Feb 2 2016, 2:37 PM

Thanks David for the comments.
I fixed most of them, see answers below.

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
573 ↗(On Diff #44269)

Yes that what I meant.
Sorry for misleading with the wrong word in the name, I keep doing this mistake between the "None" and "Non" :)

733 ↗(On Diff #44269)

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.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
467 ↗(On Diff #44269)

I just figure out that I do not need this function.
Just replaced it with DILocalScope::getSubprogram().

561 ↗(On Diff #44269)

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?

aaboud updated this revision to Diff 46709.Feb 2 2016, 2:41 PM
aaboud edited edge metadata.
aaboud removed rL LLVM as the repository for this revision.
aaboud marked an inline comment as done.

Modified code according to David comments.
Also, now code supports both solutions from the proposal, the one with DW_AT_abstract_origin on lexical blocks and the workaround solution.
The default is the workaround solution, as LLDB and older GDB debuggers will not provide information with the abstract-origin solution.

To enable the abstract-origin solution user can compile with "-use-abstract-origin-on-ls" flag.

dblaikie added inline comments.Feb 2 2016, 2:52 PM
lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
733 ↗(On Diff #44269)

Sure, that can wait

20 ↗(On Diff #46709)

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)

573 ↗(On Diff #46709)

This should be llvm_unreachable rather than assert(false)

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
467 ↗(On Diff #44269)

Great!

561 ↗(On Diff #44269)

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.

aaboud updated this revision to Diff 47986.Feb 15 2016, 5:53 AM
aaboud marked an inline comment as done.

Applied few changes according to David comments.

  1. Now LIT tests checks for -split-dwarf=Enable case.
  2. Removed the workaround solution.
  3. Replaced an assertion with "llvm_unreachable()" function.

Sorry for the late response, please see new patch and below comments.

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
20 ↗(On Diff #46709)

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?

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
547 ↗(On Diff #46709)

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?

dblaikie accepted this revision.Feb 17 2016, 9:00 AM
dblaikie edited edge metadata.

Just simplify the finishLocalScopeDefinitions call as suggested & please commit after that. (no need for another round of review, the change is simple/obvious enough)

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
547 ↗(On Diff #47986)

Just looking at the code, I think the right thing here is just to call:

I.second->finishLocalScopeDefinitions()

We shouldn't have any local scope definitions in the skeleton CU, so the other call is just a no-op anyway, so far as I can tell.

This revision is now accepted and ready to land.Feb 17 2016, 9:00 AM
aaboud updated this revision to Diff 48614.Feb 21 2016, 4:35 AM
aaboud edited edge metadata.

There was a minor issue with the implementation, which I fixed in this patch and improved the LIT tests to catch it as well.
Because the local entry information was saved in the DwarfFile, it could be accessed by all CompileUnits, and the way it was handled caused two different CompileUnits to handle same local entry twice leading into hitting assertion during compilation.
The fix was to move the data to the DwarfCompileUnit class, i.e. in this final patch there is no modification to the DwarfFile.h/cpp files.

Also, ran Clang-format and fixed few lines accordingly.

David, please give it a fast look, nothing changed except moving the structures from one file to another.

looks good - thanks! Please commit when ready

This revision was automatically updated to reflect the committed changes.