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

[Kim-Anh Tran via Phabricator via llvm-commits]

kimanh added a comment.

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.



================
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.");
----------------
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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102312