Page MenuHomePhabricator

DebugInfo: Use base address selection entries for debug_loc
ClosedPublic

Authored by dblaikie on Oct 7 2019, 6:19 PM.

Details

Summary

Unify the range and loc emission (for both DWARFv4 and DWARFv5 style lists) and take advantage of that unification to use strategic base addresses for loclists.

Needs more testing, but llvm-dwarfdump doesn't currently support LLE_base_addressx, for instance. But Pavel's looking at some changes there, so I'm holding off in case his work addresses it, or at least I can work on it afterwards so as not to conflict if I tried to do so now.

Anyone know whether they have consumers (LLDB, the Sony debugger) that would need to be updated for either the v4 changes (use of base address specifiers in classic debug_loc lists) or v5 (base_addressx, etc, etc)? GDB can't cope with the DWARFv5 stuff, but seems fine with the v4 version.

Diff Detail

Event Timeline

dblaikie created this revision.Oct 7 2019, 6:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 6:19 PM
labath added a comment.Oct 8 2019, 5:30 AM

LLDB seems to have support for base address selection in v4 debug_loc. It does not have support for v5 LLE_base_address(x) stuff, but the whole of v5 location list support is kind of wonky, which also is why I am looking at getting it to use the llvm version of the parser.

As for llvm-dwarfdump, feel free to add new encodings there. My plan is to add support for all LLE encodings, but since I also need to figure out a way to refactor all of that stuff, it may take a while before I get to that. Having one or two new encodings appear in the mean time should only be a minor nuisance.

probinson accepted this revision.Oct 8 2019, 9:23 AM

Anyone know whether they have consumers (LLDB, the Sony debugger) that would need to be updated for either the v4 changes (use of base address specifiers in classic debug_loc lists) or v5 (base_addressx, etc, etc)?

I'll ask re Sony debugger. I have no direct visibility to that code.

This revision is now accepted and ready to land.Oct 8 2019, 9:23 AM

BTW the BinaryFormat part LGTM and can go in on its own if you like. Should have been done that way in the first place.

probinson requested changes to this revision.Oct 8 2019, 10:08 AM

For some reason a previous comment caused it to set Accept, and the only way I know to undo it is to set Request Changes. Sorry about that.

This revision now requires changes to proceed.Oct 8 2019, 10:08 AM
SouraVX removed a subscriber: SouraVX.Oct 8 2019, 12:19 PM
SouraVX added a subscriber: SouraVX.

I'll ask re Sony debugger. I have no direct visibility to that code.

My debugger guys say they have code to handle it and some hand-coded tests, so they are cautiously optimistic that nothing bad will happen.

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
2328 ↗(On Diff #223716)

Would it be more readable this way?

if (!UseDwarf5) {
  Base = NewBase;
  BaseIsSet = true;
  Asm-OutStreamer->EmitIntValue(-1, Size);
  // etc
} else if (NewBase != Begin || P.second.size() > 1) {
  Base = NewBase;
  BaseIsSet = true;
  Asm->OutStreamer->AddComment(StringifyEnum(BaseAddressx);
  // etc
}

As there are only 2 lines in common. (My eye caught if (!UseDwarf5 and two lines later if (UseDwarf5) and did a double-take.)

dblaikie marked an inline comment as done.Oct 11 2019, 2:49 PM

LLDB seems to have support for base address selection in v4 debug_loc. It does not have support for v5 LLE_base_address(x) stuff, but the whole of v5 location list support is kind of wonky, which also is why I am looking at getting it to use the llvm version of the parser.

Yeah, that sort of summarizes GDB's support too.

As for llvm-dwarfdump, feel free to add new encodings there. My plan is to add support for all LLE encodings, but since I also need to figure out a way to refactor all of that stuff, it may take a while before I get to that. Having one or two new encodings appear in the mean time should only be a minor nuisance.

Had to take a few goes at this to see if there was a good mid-point of refactoring & think I found one that coalesces some of the codepaths for verbose, non-verbose, and inline dumping - insofar as seemed reasonable, I tried to make things more similar to debug_rnglists (in several cases just at least making the code look similar, even though it's not shared yet).

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
2328 ↗(On Diff #223716)

Sure, looks good to me! I know this whole function's got several cases to think about & is a bit unwieldy.

This revision was not accepted when it landed; it landed in state Needs Revision.Oct 11 2019, 2:59 PM
This revision was automatically updated to reflect the committed changes.

I haven't fully debugged this, but it looks like this change caused a failure on the Windows LLDB bot. There was already another failure, so you probably didn't get an email:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/9772

The Buildbot is still failing because of this test: http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/9884/steps/test/logs/stdio

When running the test locally, the output is:

(lldb) file E:\_work\22\b\tools\lldb\lldb-test-build.noindex\lang\c\local_variables\TestLocalVariables.test_c_local_variables_dwarf\a.out
Current executable set to 'E:\_work\22\b\tools\lldb\lldb-test-build.noindex\lang\c\local_variables\TestLocalVariables.test_c_local_variables_dwarf\a.out' (x86_64).
(lldb) br s -f main.c -l 13
Breakpoint 1: where = a.out`foo + 9 at main.c:13:3, address = 0x0000000140001029
(lldb) r
Process 43368 launched: 'E:\_work\22\b\tools\lldb\lldb-test-build.noindex\lang\c\local_variables\TestLocalVariables.test_c_local_variables_dwarf\a.out' (x86_64)
Process 43368 stopped
* thread #1, stop reason = breakpoint 1.1
    frame #0: 0x00007ff6a0141029 a.out`foo(j=<unavailable>) at main.c:13:3
   10     unsigned i = j;
   11     bar(i);
   12     i = 10;
-> 13     bar(i); // Set break point at this line.
   14   }
   15
   16   int main(int argc, char** argv)
(lldb) thread list
Process 43368 stopped
* thread #1: tid = 0x9a78, 0x00007ff6a0141029 a.out`foo(j=<unavailable>) at main.c:13:3, stop reason = breakpoint 1.1
(lldb) breakpoint list -f
Current breakpoints:
1: file = 'main.c', line = 13, exact_match = 0, locations = 1, resolved = 1, hit count = 1
  1.1: where = a.out`foo + 9 at main.c:13:3, address = 0x00007ff6a0141029, resolved, hit count = 1

(lldb) frame variable i
(unsigned int) i = <variable not available>

I haven't fully debugged this, but it looks like this change caused a failure on the Windows LLDB bot. There was already another failure, so you probably didn't get an email:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/9772

BTW, there were some failures on linux after this patch too, caused by lldb's incomplete support for base address selection entries. r374769 was enough to fix the failures I was seeing on linux, with the location parsing being as scattered as it is, it is possible I did not catch all cases.

It looks like this caused a very large increase in binary size (627M->686M). Is that expected/has anyone else observed this?