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

[ayermolo]

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.

[ayermolo]

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.

[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.

[dblaikie]

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

2–13

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.

[ayermolo]

In D96827#2623393, @dblaikie wrote:

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.

[dblaikie]

In D96827#2623409, @ayermolo wrote:

In D96827#2623393, @dblaikie wrote:

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]

Trying through arc.

[ayermolo]

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]

Changed to use assembly.

[ayermolo]

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

[dblaikie]

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]

Renamed .s to same name as test.

[dblaikie]

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

[ayermolo]

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

[ayermolo]

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

[justice_adams]

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

[dblaikie]

In D96827#2627717, @justice_adams wrote:

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.

[ayermolo]

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

[dblaikie]

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)

[thakis]

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

[dblaikie]

In D96827#2627833, @thakis wrote:

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

Sorry, failed to push. Should be pushed now.

[thakis]

Yes, better now. Thanks!

[ayermolo]

Thank You!

llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp

 
 DWARFDataExtractor DWARFUnit::getDebugInfoExtractor() const {
   return DWARFDataExtractor(Context.getDWARFObj(), InfoSection, isLittleEndian,
                             getAddressByteSize());
 }
 
 Optional<object::SectionedAddress>
 DWARFUnit::getAddrOffsetSectionItem(uint32_t Index) const {
-  if (IsDWO) {
+  if (!AddrOffsetSectionBase) {
     auto R = Context.info_section_units();
     // Surprising if a DWO file has more than one skeleton unit in it - this
     // probably shouldn't be valid, but if a use case is found, here's where to
     // support it (probably have to linearly search for the matching skeleton CU
     // here)
-    if (hasSingleElement(R))
+    if (IsDWO && hasSingleElement(R)) {
       return (*R.begin())->getAddrOffsetSectionItem(Index);
     }
-  if (!AddrOffsetSectionBase)
     return None;
+  }
+
   uint64_t Offset = *AddrOffsetSectionBase + Index * getAddressByteSize();
   if (AddrOffsetSection->Data.size() < Offset + getAddressByteSize())
     return None;
   DWARFDataExtractor DA(Context.getDWARFObj(), *AddrOffsetSection,
                         isLittleEndian, getAddressByteSize());
   uint64_t Section;
   uint64_t Address = DA.getRelocatedAddress(&Offset, &Section);
   return {{Address, Section}};

llvm/test/DebugInfo/Inputs/splitdwarf-single-issue.cpp

+__attribute__((nodebug)) __attribute__((optnone)) void f1() {
+}
+
+__attribute__((always_inline)) inline void f2() {
+
+  f1();
+}
+
+int main() {
+
+  f2();
+}

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

+Binary and .o was generated by:
+1) clang++ -g -gsplit-dwarf=single splitdwarf-single-issue.cpp -O3 -o splitdwarf-single-issue.o -s
+2) Edit CompileDir to current directory
+3) clang++ -g -gsplit-dwarf=single splitdwarf-single-issue.s -O3 -o splitdwarf-single-issue.o -c
+4) clang++ -g -gsplit-dwarf=single splitdwarf-single-issue.o -O3 -o splitdwarf-single-issue
+
+Copy the .o and then cd into the output directory, so that the symbolizer would
+find the .o if it attempted to.
+RUN: rm -rf %t && mkdir -p %t
+RUN: cd %t
+RUN: cp %p/Inputs/splitdwarf-single-issue.o %t
+RUN: llvm-symbolizer  0x0000000000400611 -a --obj=%p/Inputs/splitdwarf-single-issue \
+RUN:   | FileCheck %s
+
+CHECK:      0x400611
+CHECK-NEXT: f2() 
+CHECK-NEXT: ././test.cpp:7:3
+CHECK-NEXT: main
+CHECK-NEXT: ././test.cpp:13:3