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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
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.
Comment Actions
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.
Comment Actions
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 | Drop the {} around single line block. ( https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements ) | |
| 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. |
Comment Actions
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.
Comment Actions
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)
Comment Actions
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.
Comment Actions
@dblaikie You were right, it's possible. Just need to use non-relocated address and remove some sections. If all good, will commit.
Comment Actions
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.
Comment Actions
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
Comment Actions
Oh right. Windows...
@dblaikie if you are busy I can put up a diff.
Doing should work I think.
- CHECK: 0x11
- CHECK-NEXT: f2()
- CHECK-NEXT: splitdwarf-single-issue.cpp:7:3
- CHECK-NEXT: main
- CHECK-NEXT: splitdwarf-single-issue.cpp:13:3
Comment Actions
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)
Drop the {} around single line block. ( https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements )