Page MenuHomePhabricator

[DebugInfo][DWARF] Emit a single location instead of a location list
ClosedPublic

Authored by Orlando on Thu, May 7, 6:26 AM.

Details

Summary

for variables in nested scopes (including inlined functions) if there is a
single location which covers the entire scope and the scope is contained in a
single block.

With this change there are still scenarios where we can, but do not, emit a
single location for a variable. Still, this change significantly reduces the
size of .debug_loc for a clang-3.4 build and reduces the size of the .debug_info
section slightly too.

Compiler vs section size
*----------------------------------------------* 
|             | trunk     | patched   | change |
|             | (bytes)   | (bytes)   | (%)    |
|-------------|-----------|-----------|--------|
| .debug_loc  | 152128993 | 104924892 | -31.03 |
| .debug_info | 346249712 | 345249350 | -00.29 |
| file size   | 621497992 | 573304448 | -07.75 |
*----------------------------------------------*

As @jmorse mentioned here [0], it would be nice to not have to linearly scan,
but I don't think that can be avoided right now.

[0]: https://reviews.llvm.org/D77639#1969189

Diff Detail

Event Timeline

Orlando created this revision.Thu, May 7, 6:26 AM
Orlando edited the summary of this revision. (Show Details)Thu, May 7, 7:06 AM
aprantl added inline comments.Thu, May 7, 10:06 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1540

Does this do the right thing if the scope is "interrupted" by another scope within MBB?

This is really nice.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1540

Hm, are you describing a situation like this, where all the code is fit into one MBB?

{
  dbg_value "x"
  {
    dbg_value "y"
    ...
  }
}

If the inner scope is an inlined frame, I think it'd be ok for the location of 'x' to be valid throughout, since the debugger shouldn't consider 'x' in scope. If its a nested {} scope, 'x' should still be visible within it.

So I think this is ok. Might be good to add tests for this, though.

aprantl added inline comments.Thu, May 7, 12:53 PM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1540

I was thinking about

MBB:
  insn 1 # scope A
  insn 2 # scope A
  insn 3 # completely unrelated scope B, instruction was moved here by a transformation
  insn 4 # scope A
...
Orlando marked an inline comment as done.Mon, May 11, 3:11 AM

Thank you both for taking a look.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1540

I'm confident that we handle @vsk's example, but less sure about @aprantl's. I'll write up tests for both.

Orlando updated this revision to Diff 263383.Tue, May 12, 2:21 AM

I've updated the diff to include 'single-location-inlined-param.mir' and 'single-location-interrupted-scope.mir' which should cover both of those cases IIUC.

vsk added a comment.Tue, May 12, 4:19 PM

Thanks, I'm convinced this is doing the right thing now.

llvm/test/DebugInfo/X86/single-location-inlined-param.mir
31

Why are the DILocalVariables for 'local' and 'number' needed? I'm a bit concerned that their presence could make the check lines brittle.

llvm/test/DebugInfo/X86/single-location-interrupted-scope.mir
35

Wow, this is really neat. I see you've deleted the other DILocalVariables to simplify the check lines -- consider leaving a note about that here.

Orlando updated this revision to Diff 263644.Wed, May 13, 1:53 AM
Orlando marked 5 inline comments as done.

+ Addressed your inline comments.

I wrote up an example of another improvement we could make here https://bugs.llvm.org/show_bug.cgi?id=45889.

vsk accepted this revision.Wed, May 13, 12:06 PM

Thanks, lgtm. Could you wait for @aprantl to double-check this as well?

This revision is now accepted and ready to land.Wed, May 13, 12:06 PM
aprantl added inline comments.Thu, May 14, 10:36 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1540

I'm confident that we handle @vsk's example, but less sure about @aprantl's. I'll write up tests for both.

What came out of that investigation?

Orlando marked 2 inline comments as done.Fri, May 15, 2:22 AM
Orlando added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1540

Ah, sorry, I replied to this with a non-inline comment. I added two more tests, 'single-location-inlined-param.mir' and 'single-location-interrupted-scope.mir', which should cover both of these situations.

aprantl accepted this revision.Fri, May 15, 1:18 PM
Orlando marked an inline comment as done.Mon, May 18, 1:51 AM

Thank you both, and @jmorse for doing a lot of the heavy lifting on this before I got to it.

This revision was automatically updated to reflect the committed changes.