[PATCH] D102312: [llvm-dwp] Skip type unit debug info sections

Fri May 21 18:25:06 PDT 2021

dblaikie added a comment.

In D102312#2762469 <https://reviews.llvm.org/D102312#2762469>, @kimanh wrote:

> In D102312#2758304 <https://reviews.llvm.org/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 <http://dwarfstd.org/doc/DWARF5.pdf>).
> 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!

================
Comment at: llvm/test/tools/llvm-dwp/X86/wrong-unit-type-info-v4.s:11
+  .byte	8                               # Address Size (in bytes)
+  .byte	1                               # Abbrev [1] 0xb:0x22 DW_TAG_compile_unit
+.Ldebug_info_dwo_end0:
----------------
Update this comment to correct the tag number and name?

================
Comment at: llvm/tools/llvm-dwp/llvm-dwp.cpp:156-159
+      // Type offset.
+      InfoData.getU32(&Offset, &Err);
+      if (Err)
+        return make_error<DWPError>("Type unit is missing type offset.");
----------------
kimanh wrote:
> dblaikie wrote:
> > 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.
> 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!
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")

================
Comment at: llvm/tools/llvm-dwp/llvm-dwp.cpp:271
+static Expected<CompileUnitIdentifiers>
+getCUIdentifiers(InfoSectionUnitHeader &Header, StringRef Abbrev,
+                 StringRef Info, StringRef StrOffsets, StringRef Str) {
----------------
Does "header" need to be passed by non-const reference? Could it be const ref?

================
Comment at: llvm/tools/llvm-dwp/llvm-dwp.cpp:727
+      Out.SwitchSection(InfoSection);
+      for (StringRef Info : CurInfoSection) {
+        if (FoundCUUnit)
----------------
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.

================
Comment at: llvm/tools/llvm-dwp/llvm-dwp.cpp:739-741
+          auto &C = Entry.Contributions[getContributionIndex(DW_SECT_INFO)];
+          C.Offset = InfoSectionOffset;
+          C.Length = Header.Length + 4;
----------------
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?

================
Comment at: llvm/tools/llvm-dwp/llvm-dwp.cpp:767-769
+      if (!FoundCUUnit) {
+        return make_error<DWPError>("no compile unit found in file: " + Input);
+      }
----------------
Usually we skip braces on single statement blocks in LLVM.

================
Comment at: llvm/tools/llvm-dwp/llvm-dwp.cpp:779-781
+    if (CurInfoSection.size() != 1)
+      return make_error<DWPError>("expected exactly one occurrence of a debug "
+                                  "info section in a .dwp file");
----------------
Test coverage?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102312/new/

https://reviews.llvm.org/D102312