[llvm-dwp] Skip type unit debug info sections

[kimanh]

Summary

This patch makes llvm-dwp skip debug info sections that may not be encoding a compile unit.
In DWARF5, debug info sections are also used for type units. As in preparation to support type units,
make llvm-dwp aware of other uses of debug info sections but skip them for now.

The patch first records all .debug_info sections, then goes through them one by one and records
the cu debug info section for writing the index unit, and copies that section to the final dwp output
info section. If it's not a compile unit, skip.

[kimanh]

Upload whole range of commits.

[kimanh]

Another try uploading whole range.

[kimanh]

.

[kimanh]

adapt comment

[kimanh]

Minor: add comment to unit type.

[kimanh]

Minor: another comment.

[kimanh]

Hello Dave,

I'm splitting up the CL in https://reviews.llvm.org/D101818 into a chain of 3 CLs, this is the first one. I'll add more info on the original comment that you made, ptal!

[dblaikie]

Maybe a bunch of the complexity in this patch comes from supporting multiple .debug_info.dwo sections in a .dwo file - could you check if that's needed? Could we fix that issue in LLVM instead of supporting it in llvm-dwp.

llvm/tools/llvm-dwp/llvm-dwp.cpp

156–159

Why different error handling here than for the rest of the header field parsing?

I'd have thought the MinHeaderLength check above would be adequate/intended to mean that all the rest of the header fields could be parsed without error checking?

Ah, but you don't know the length anymore at that point - since you have to wait until you've read the unit kind. OK.

But this error checking would be incomplete - since it's not the offset field that would fail to parse - it's the addrSize later on where we'd run out of bytes to read and fail there instead of here.

Test coverage would probably demonstrate this/help motivate changes to make this error checking correct.

603–604

The need to keep multiple info sections on input I think could reasonably be consider to be a bug in LLVM's output. At least as far as I'm concerned - we should produce a single .debug_info section with all the type and compile units in it. I don't think it's worth supporting multiple .debug_info sections in llvm-dwp - we should fix LLVM instead, if possible.

[dblaikie]

Hmm, now that I look at LLVM's output - I don't see multiple .debug_info.dwo sections. Do you?

Maybe you were testing GCC dwo files? I'm OK not working with those... though I guess I did add functionality to support them to llvm-dwp for type units. I think it's probably better that GCC improve their output (it improves compression too, compared to splitting things into separate sections).

Hmm, I'm going back and forth - we do support multiple .debug_types.dwo sections, so for consistency it probably makes sense to support multiple .debug_info.dwo sections too... ). Thinking...

[kimanh]

In D102312#2758304, @dblaikie wrote:

Hmm, now that I look at LLVM's output - I don't see multiple .debug_info.dwo sections. Do you?

Maybe you were testing GCC dwo files? I'm OK not working with those... though I guess I did add functionality to support them to llvm-dwp for type units. I think it's probably better that GCC improve their output (it improves compression too, compared to splitting things into separate sections).

Hmm, I'm going back and forth - we do support multiple .debug_types.dwo sections, so for consistency it probably makes sense to support multiple .debug_info.dwo sections too... ). Thinking...

I was actually testing with LLVM dwo files (and no, I didn't see multiple .debug_info sections for LLVM). However, the DWARFv5 specification mentions multiple .debug_info sections:

"While a split DWARF object may contain multiple .debug_info.dwo sections, one for the compilation unit, and one for each type unit, a package file contains a single .debug_info.dwo section." (F.3, DWARFv5 spec).
On top of that this was also supported for the .debug_types section, so I added it for the .debug_info sections too. Looking at GCC, it indeed generates multiple .debug_info sections for the .dwo file.

I'll happily remove the support for multiple sections, if you think that's better for reducing the complexity. For our use case, we won't use GCC output, so we'd be fine not supporting it.

llvm/tools/llvm-dwp/llvm-dwp.cpp

156–159

Yes, exactly, it's because of the unit type that I need to know before knowing about the final length.

That's true (incomplete error checking), will change it!

[kimanh]

Add test for type unit parsing and changed error checking
for type unit header.

[dblaikie]

In D102312#2762469, @kimanh wrote:

In D102312#2758304, @dblaikie wrote:

Hmm, now that I look at LLVM's output - I don't see multiple .debug_info.dwo sections. Do you?

Maybe you were testing GCC dwo files? I'm OK not working with those... though I guess I did add functionality to support them to llvm-dwp for type units. I think it's probably better that GCC improve their output (it improves compression too, compared to splitting things into separate sections).

Hmm, I'm going back and forth - we do support multiple .debug_types.dwo sections, so for consistency it probably makes sense to support multiple .debug_info.dwo sections too... ). Thinking...

I was actually testing with LLVM dwo files (and no, I didn't see multiple .debug_info sections for LLVM). However, the DWARFv5 specification mentions multiple .debug_info sections:

"While a split DWARF object may contain multiple .debug_info.dwo sections, one for the compilation unit, and one for each type unit, a package file contains a single .debug_info.dwo section." (F.3, DWARFv5 spec).
On top of that this was also supported for the .debug_types section, so I added it for the .debug_info sections too. Looking at GCC, it indeed generates multiple .debug_info sections for the .dwo file.

I'll happily remove the support for multiple sections, if you think that's better for reducing the complexity. For our use case, we won't use GCC output, so we'd be fine not supporting it.

Nah, no worries - you're right, we should support it since the spec says so and we do support the GCC behavior already for v4. Thanks for the refresher!

llvm/test/tools/llvm-dwp/X86/wrong-unit-type-info-v4.s

11 ↗

(On Diff #344761)

Update this comment to correct the tag number and name?

llvm/tools/llvm-dwp/llvm-dwp.cpp

156–159

Looks like this issue is still pending (the error message is still not tested and the issue of where this would fail, etc, is still outstanding & hopefully illuminated by checking this with a test)

(also error messages are usually all lower case without a trailing '.', because they'll be concatenated after "error: " in classicy unix style error message output, eg: "error: type unit is missing a type offset")

271

Does "header" need to be passed by non-const reference? Could it be const ref?

727

Perhaps if CurInfoSection.size() == 1 we could skip all this searching and assume the info section is in that one section somewhere? (this'd work better for LLVM's output that only uses one section & I think it puts the types first, so this search would always be maximally suboptimal - having to search through all the units in LLVM's output and finding the CU on the very last one)

Oh, I guess it can't end early - because this has necessary side effects of finding the actual CU? OK then.

739–741

Looks like this has side effects (changing the values in the Entry? oh, but only a local copy of it? but that copy gets put in the IndexEntries map... )

Which is necessary to prune down the range for the CU index to only describe the CU itself?

767–769

Usually we skip braces on single statement blocks in LLVM.

779–781

Test coverage?

[kimanh]

Addressed comments:

• Fixing tag number and name for one test
• Remove braces for one-liner if case

[kimanh]

Thanks again for the reviews! One test request is open (with a question on how to best write the test), I think otherwise I addressed/answered the questions.

llvm/tools/llvm-dwp/llvm-dwp.cpp

156–159

I've actually changed the code, and added a test case, see invalid_tu_header_length.s (but that was on Friday, so maybe you looked at a previous diff?). However, the dot is still there, so I'll remove the dot. Or am I missing something, maybe?

271

We are actually changing the header in this function (by setting the signature in ll.316 (for DWARFv4, the signature is in the abbrev section).

727

Yes, we need to search for it to properly prune the section offsets as you noted below.

739–741

Yes, we're generating a copy of it (I basically imitated the behavior in 'addAllTypes´: create a copy and update the .debug_info offsets for this particular entry).

Yes, for pruning down to only describe the CU itself and later for the TU itself.
Looking at the DWARF5 document, Appendix F10 and F11, the CU and TU indices for the debug_info section do not overlap (for example). But please correct me if I'm misundrestanding something!

779–781

I tried to add a *.s test for this case but I'm not entirely sure how to create one properly. If I add multiple .debug_info sections, after running llvm-mc there will only be one.

Is there a way to create a test with multiple .debug_info sections but one index than actually writing the byte code?

[dblaikie]

llvm/tools/llvm-dwp/llvm-dwp.cpp

779–781

I think the way this happens with GCC is that it produces a single .o file that has the usual comdat groups for type units - then it runs objcopy to split out the .dwo sections, and objcopy doesn't propagate the comdat groups but it does keep the separate sections.

Probably the easiest thing to do might be to have an input file use a comdat group? (try compiling without Split DWARF and check the assembly - you'll see the .debug_info section directive for the type unit will have something like .section .debug_types.dwo,"G",@progbits,wt.b0cf918beae249c9,comdat - so maybe you can do something like that - the consumer (llvm-dwp) won't care that it happens to be in a section group, I shouldn't think - also, maybe use an obvious made-up value for the hash (like 0xFDFDFDFDFD or whatever))

[kimanh]

Add missing test on error reporting multiple debug_sections in a dwp file.

[kimanh]

llvm/tools/llvm-dwp/llvm-dwp.cpp

779–781

Thanks for the guidance, that worked! I have added a test in the latest revision.

[dblaikie]

Looks good, thanks!

[kimanh]

Rebase on main to re-trigger pre-merge checks.

[kimanh]

Thanks a lot for all the guidance and the help with the CLs!

I have rebased the patches and they pass the pre-merge checks. Could you help me to land them?

llvm/test/tools/llvm-dwp/X86/cu_and_tu_info_section_v5.s

 # This test checks if llvm-dwp can find the compilation unit if
 # both type and compile units are available in the debug info section (v5)
 
 # RUN: llvm-mc --triple=x86_64-unknown-linux --filetype=obj --split-dwarf-file=%t.dwo -dwarf-version=5 %s -o %t.o
 # RUN: llvm-dwp %t.dwo -o %t.dwp
-# RUN: llvm-dwarfdump %t.dwp | FileCheck %s
+# RUN: llvm-dwarfdump -debug-info -debug-tu-index %t.dwp | FileCheck %s
 
 # CHECK-DAG: .debug_info.dwo contents
-# CHECK: 0x00000000: Compile Unit: length = 0x00000011, format = DWARF32, version = 0x0005, unit_type = DW_UT_split_compile, abbr_offset = 0x0000, addr_size = 0x08, DWO_id = {{.*}}
-
-  .section  .debug_info.dwo,"e",@progbits
-  .long .Ldebug_info_dwo_end0-.Ldebug_info_dwo_start0 # Length of Unit
+# CHECK-NOT: Type Unit:
+# CHECK: 0x00000000: Compile Unit: length = 0x00000011, format = DWARF32, version = 0x0005, unit_type = DW_UT_split_compile, abbr_offset = 0x0000, addr_size = 0x08, DWO_id = {{.*}} (next unit at 0x00000015)
+    .section  .debug_info.dwo,"e",@progbits
+    .long .Ldebug_info_dwo_end0-.Ldebug_info_dwo_start0 # Length of Unit
 .Ldebug_info_dwo_start0:
-  .short  5                               # DWARF version number
-  .byte 6                               # DWARF Unit Type (DW_UT_split_type)
-  .byte 8                               # Address Size (in bytes)
-  .long 0                               # Offset Into Abbrev. Section
-  .quad -4287463584810542331            # Type Signature
-  .long 31                              # Type DIE Offset
+    .short  5                               # DWARF version number
+    .byte 6                               # DWARF Unit Type (DW_UT_split_type)
+    .byte 8                               # Address Size (in bytes)
+    .long 0                               # Offset Into Abbrev. Section
+    .quad 5657452045627120676             # Type Signature
+    .long 25                              # Type DIE Offset
+    .byte 1                               # Abbrev [1] DW_TAG_type_unit
+    .byte 2                               # Abbrev [2] DW_TAG_structure_type
+    .byte 0                               # End Of Children Mark
 .Ldebug_info_dwo_end0:
-  .long .Ldebug_info_dwo_end1-.Ldebug_info_dwo_start1 # Length of Unit
+    .section  .debug_info.dwo,"e",@progbits
+    .long .Ldebug_info_dwo_end1-.Ldebug_info_dwo_start1 # Length of Unit
 .Ldebug_info_dwo_start1:
-  .short  5                               # DWARF version number
-  .byte 5                               # DWARF Unit Type (DW_UT_split_compile)
-  .byte 8                               # Address Size (in bytes)
-  .long 0                               # Offset Into Abbrev. Section
-  .quad 1152943841751211454
-  .byte  1                               # Abbrev [1] 0x14:0x2be DW_TAG_compile_unit
+    .short  5                               # DWARF version number
+    .byte 5                               # DWARF Unit Type (DW_UT_split_compile)
+    .byte 8                               # Address Size (in bytes)
+    .long 0                               # Offset Into Abbrev. Section
+    .quad -1506010254921578184
+    .byte  3                               # Abbrev [3] DW_TAG_compile_unit
 .Ldebug_info_dwo_end1:
-  .section  .debug_abbrev.dwo,"e",@progbits
-  .byte 1                               # Abbreviation Code
-  .byte 17                              # DW_TAG_compile_unit
+    .section  .debug_abbrev.dwo,"e",@progbits
+    .byte 1                               # Abbreviation Code
+    .byte 65                              # DW_TAG_type_unit
+    .byte 1                               # DW_CHILDREN_yes
+    .byte 0                               # EOM(1)
+    .byte 0                               # EOM(2)
+    .byte 3                               # Abbreviation Code
+    .byte 17                              # DW_TAG_compile_unit
+    .byte 0                               # DW_CHILDREN_no
+    .byte 0                               # EOM(1)
+    .byte 0                               # EOM(2)
+    .byte 0                               # EOM(3)

llvm/tools/llvm-dwp/llvm-dwp.cpp

if (Header.Version >= 5) {
     // Size: Version (2), UnitType (1), AddrSize (1), DebugAbbrevOffset (4),
     // Signature (8)
     MinHeaderLength = 16;
   } else {
     // Size: Version (2), DebugAbbrevOffset (4), AddrSize (1)
     MinHeaderLength = 7;
   }
   if (Header.Length < MinHeaderLength) {
-    return make_error<DWPError>(
-        "compile unit length is too small: expected at least " +
-        utostr(MinHeaderLength) + " got " + utostr(Header.Length) + ".");
+    return make_error<DWPError>("unit length is too small: expected at least " +
+                                utostr(MinHeaderLength) + " got " +
+                                utostr(Header.Length) + ".");
   }
   if (Header.Version >= 5) {
     Header.UnitType = InfoData.getU8(&Offset);
     Header.AddrSize = InfoData.getU8(&Offset);
     Header.DebugAbbrevOffset = InfoData.getU32(&Offset);
     Header.Signature = InfoData.getU64(&Offset);
     if (Header.UnitType == dwarf::DW_UT_split_type) {
       // Type offset.
       InfoData.getU32(&Offset, &Err);
       if (Err)
         return make_error<DWPError>("Type unit is missing type offset.");
     }
   } else {
     // Note that, address_size and debug_abbrev_offset fields have switched
     // places between dwarf version 4 and 5.
     Header.DebugAbbrevOffset = InfoData.getU32(&Offset);
     Header.AddrSize = InfoData.getU8(&Offset);
   }
 
getIndexedString(dwarf::Form Form, DataExtractor InfoData, uint64_t &InfoOffset,
   StrOffsetsOffset += debugStrOffsetsHeaderSize(StrOffsetsData, Version);
 
   uint64_t StrOffset = StrOffsetsData.getU32(&StrOffsetsOffset);
   DataExtractor StrData(Str, true, 0);
   return StrData.getCStr(&StrOffset);
 }
 
 static Expected<CompileUnitIdentifiers>
 getCUIdentifiers(InfoSectionUnitHeader &Header, StringRef Abbrev,
                  StringRef Info, StringRef StrOffsets, StringRef Str) {
   DataExtractor InfoData(Info, true, 0);
   uint64_t Offset = Header.HeaderSize;
   if (Header.Version >= 5 && Header.UnitType != dwarf::DW_UT_split_compile)
     return make_error<DWPError>(
         std::string("unit type DW_UT_split_compile type not found in "
                     "debug_info header. Unexpected unit type 0x" +
                     utostr(Header.UnitType) + " found"));
static Error handleSection(
   else if (OutSection == StrSection)
     CurStrSection = Contents;
   else if (OutSection == TypesSection)
     CurTypesSection.push_back(Contents);
   else if (OutSection == CUIndexSection)
     CurCUIndexSection = Contents;
   else if (OutSection == TUIndexSection)
     CurTUIndexSection = Contents;
   else if (OutSection == InfoSection)
     CurInfoSection.push_back(Contents);
   else {
     Out.SwitchSection(OutSection);
     Out.emitBytes(Contents);
   }
   return Error::success();
 }
 
 static Error
for (const auto &Input : Inputs) {
     writeStringsAndOffsets(Out, Strings, StrOffsetSection, CurStrSection,
                            CurStrOffsetSection, Header);
 
     uint32_t &InfoSectionOffset =
         ContributionOffsets[getContributionIndex(DW_SECT_INFO)];
     if (CurCUIndexSection.empty()) {
       bool FoundCUUnit = false;
       Out.SwitchSection(InfoSection);
       for (StringRef Info : CurInfoSection) {
         if (FoundCUUnit)
           break;
         uint64_t UnitOffset = 0;
         while (Info.size() > UnitOffset) {
           Expected<InfoSectionUnitHeader> HeaderOrError =
               parseInfoSectionUnitHeader(Info.substr(UnitOffset, Info.size()));
           if (!HeaderOrError)
             return HeaderOrError.takeError();
           InfoSectionUnitHeader &Header = *HeaderOrError;
 
           UnitIndexEntry Entry = CurEntry;
           auto &C = Entry.Contributions[getContributionIndex(DW_SECT_INFO)];
           C.Offset = InfoSectionOffset;
           C.Length = Header.Length + 4;
 
           if (Header.Version < 5 ||
               Header.UnitType == dwarf::DW_UT_split_compile) {
             Expected<CompileUnitIdentifiers> EID = getCUIdentifiers(
                 Header, AbbrevSection, Info.substr(UnitOffset, C.Length),
                 CurStrOffsetSection, CurStrSection);
 
             if (!EID)
if (CurCUIndexSection.empty()) {
             InfoSectionOffset += C.Length;
             FoundCUUnit = true;
             break;
           }
           UnitOffset += Header.Length + 4;
         }
       }
 
       if (!FoundCUUnit) {
         return make_error<DWPError>("no compile unit found in file: " + Input);
       }
 
       // Add types from the .debug_types section from DWARF < 5.
       addAllTypes(Out, TypeIndexEntries, TypesSection, CurTypesSection,
                   CurEntry,
                   ContributionOffsets[getContributionIndex(DW_SECT_EXT_TYPES)]);
 
       continue;
     }
 
     if (CurInfoSection.size() != 1)
       return make_error<DWPError>("expected exactly one occurrence of a debug "
                                   "info section in a .dwp file");
     StringRef DwpSingleInfoSection = CurInfoSection.front();
 
     DWARFUnitIndex CUIndex(DW_SECT_INFO);
     DataExtractor CUIndexData(CurCUIndexSection, Obj.isLittleEndian(), 0);
     if (!CUIndex.parse(CUIndexData))
       return make_error<DWPError>("failed to parse cu_index");
     if (CUIndex.getVersion() != 2)
       return make_error<DWPError>(