Page MenuHomePhabricator

[DWARF] Check for AddrOffsetSectionBase to work with DWO Units.
ClosedPublic

Authored by ayermolo on Feb 16 2021, 6:22 PM.

Details

Summary

Context: https://lists.llvm.org/pipermail/llvm-dev/2021-February/148521.html

A fix for symbolizer, and other tools like BOLT, that allows retrieving address when build with -gsplit-dwarf=single mode.

Diff Detail

Event Timeline

ayermolo created this revision.Feb 16 2021, 6:22 PM
ayermolo requested review of this revision.Feb 16 2021, 6:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2021, 6:22 PM

Per discussion with @dblaikie going to go with this approach. Need to add test + see if we can avoid redundant checks.

Looking at what various tools produce here are few cases.
CASE 1
llc output.
This file contains .debug_info, .debug_info.dwo, .debug_info.dwo, etc sections.
IsDWO --> true.
hasSingleElement --> true
AddrOffsetSectionBase -> False, and True on recursive call.
Return Address.

CASE 2
DWARF5 produced by llvm-mc. Important part here is DWARF5 and we need to dump .debug_rnglists on the .o file.
IsDWO --> False
hasSingleElement --> False
AddrOffsetSectionBase --> False
Return None
Here AddrOffsetSectionBase is false due to how the DWO CU is created. I think if we went through getNonSkeletonUnitDIE we would have that link.

CASE 3
DWP IsDWO --> True
hasSingleElement --> False
AddrOffsetSectionBase --> False
Return None
We are dumping dwo sections by feeding dwp file, so no link to binary with .debug_addr. Although I do want to add API to create that link. Similar to what happens under the hood in getNonSkeletonUnitDIE/parseDWO. This way tools, like BOLT, that deal with DWARF info on DIE level can transparently do it.

CASE 4
DWARF Fission Single Mode. When DWO CUs are created with getNonSkeletonUnitDIE, then processed.
IsDWO --> True
hasSingleElement --> True
AddrOffsetSectionBase --> True
Return None. This is the case we want to fix. If AddrOffsetSectionBase is true it should use it, and not go in to recursive call because hasSingleElement is True.

CASE 5
DWARF Fission Split Mode. Again if there is a link created by getNonSkeletonUnitDIE.
IsDWO --> True
hasSingleElement --> False Since we processing dwo file that only has dwo sections
AddrOffsetSectionBase --> True
Return Address

So TLDR.
If AddrOffsetSectionBase is true we should just use that.
If it's false and IsDWO is true we need to check hasSingleElement. If that is true do recursive call, otherwise return None.

ayermolo updated this revision to Diff 330115.Mar 11 2021, 5:39 PM
ayermolo retitled this revision from [DWARF][WIP] Check for AddrOffsetSectionBase to work with DWO Units. to [DWARF] Check for AddrOffsetSectionBase to work with DWO Units..
ayermolo edited the summary of this revision. (Show Details)
ayermolo added a reviewer: dblaikie.

If possible, we tend to use assembly for tests these days, compiling the assembly with llvm-mc to an object file then running the test. Could this use that technique? (& please include a comment in the test file (which can contain the assembly - so it's all in one file) with the specific source code used to compile to the assembly, the commands used to get there, etc.

Does this have to use a linked binary? or does it reproduce on a straight .o file? If it needs to be linked... yeah, we don't have much for that short of checking in the linked binary I think.

ayermolo updated this revision to Diff 330299.Mar 12 2021, 10:41 AM
dblaikie accepted this revision.Mar 12 2021, 1:41 PM

Looks good - just a couple of minor fixes to make before committing.

(I assume by your patch that this is only reproducible with a linked binary? please confirm that (in a comment here) before committing)

llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
201–203
llvm/test/DebugInfo/Inputs/splitdwarf-single-issue.cpp
1–12 ↗(On Diff #330299)

Probably move this inline in the .test file - easier to read in one place and understand why the behavior being tested is what it is based on the input.

This revision is now accepted and ready to land.Mar 12 2021, 1:41 PM

Looks good - just a couple of minor fixes to make before committing.

(I assume by your patch that this is only reproducible with a linked binary? please confirm that (in a comment here) before committing)

Sorry for not replying. I was trying to see if I could fix the build failure. It works on my CentOS machine.
Yes I think it's only reproducible with the binary/object combo. You need to have .debug_addr to be in different file than DWO CU. The case where they are all in same file is covered by LLC, Case 1, and works.

Looks good - just a couple of minor fixes to make before committing.

(I assume by your patch that this is only reproducible with a linked binary? please confirm that (in a comment here) before committing)

Sorry for not replying. I was trying to see if I could fix the build failure. It works on my CentOS machine.
Yes I think it's only reproducible with the binary/object combo. You need to have .debug_addr to be in different file than DWO CU. The case where they are all in same file is covered by LLC, Case 1, and works.

Oh, you could still have two separate files, though, without it being a linked binary? IF you took the assembly from LLVM for split DWARF (which does have all the assembly for both .o and .dwo in one assembly file) and then manually split that into two .s files and assembled each - could you then reproduce the problem? If you can do it that way, please change the test to contain the two .s files (or one .s file with conditional directives so the .o/.dwo can be compiled to different files)

ayermolo updated this revision to Diff 330710.Mar 15 2021, 10:19 AM

Trying through arc.

ayermolo updated this revision to Diff 330713.EditedMar 15 2021, 10:22 AM

Trying through ARC, removed modified file that snuck in.
Seeing if going through web interface was somehow messing up a diff that would cause unit tests fail here.
More on going llvm-mc route later.

ayermolo updated this revision to Diff 330779.Mar 15 2021, 1:12 PM

Changed to use assembly.

Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: Restricted Project, sstefan1. · View Herald Transcript

@dblaikie You were right, it's possible. Just need to use non-relocated address and remove some sections. If all good, will commit.

ayermolo marked an inline comment as done.Mar 15 2021, 1:16 PM
dblaikie accepted this revision.Mar 15 2021, 1:19 PM

Looks good, but please rename the .s files to match the test file name (like Inputs/symbolize-debug-fission-single-{1,2}.s or the like). It'll make them easier to find.

ayermolo updated this revision to Diff 330793.Mar 15 2021, 1:30 PM

Renamed .s to same name as test.

Can you commit this yourself, or would you like me to commit it for you?

That would be great. I don't have commit access yet.

This revision was landed with ongoing or failed builds.Mar 15 2021, 2:48 PM
This revision was automatically updated to reflect the committed changes.

Thanks for committing, and refactoring test. Didn't know about --defsym.

Hi @ayermolo , looks like

llvm/test/DebugInfo/X86/symbolize-debug-fission-single.test

Is causing some windows buildbots to fail. Mind taking a look?
http://lab.llvm.org:8011/#/builders/123/builds/3391

Hi @ayermolo , looks like

llvm/test/DebugInfo/X86/symbolize-debug-fission-single.test

Is causing some windows buildbots to fail. Mind taking a look?
http://lab.llvm.org:8011/#/builders/123/builds/3391

Looks like the path separator, should have a fix shortly.

Oh right. Windows...
@dblaikie if you are busy I can put up a diff.
Doing should work I think.

  1. CHECK: 0x11
  2. CHECK-NEXT: f2()
  3. CHECK-NEXT: splitdwarf-single-issue.cpp:7:3
  4. CHECK-NEXT: main
  5. CHECK-NEXT: splitdwarf-single-issue.cpp:13:3

Hopefully fixed in 350633bab94f8ba70d9427715bbcbf9e3c6abf20 (essentially the same as your fix @ayermolo - I just felt like the {{.*}} made it clear it was intentional that some extra characters on the line are being skipped over)

Still broken after that change: http://45.33.8.238/win/35045/step_11.txt

Sorry, failed to push. Should be pushed now.

Yes, better now. Thanks!