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

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

kimanh added a comment.

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.



================
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:
> 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")
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?


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


================
Comment at: llvm/tools/llvm-dwp/llvm-dwp.cpp:727
+      Out.SwitchSection(InfoSection);
+      for (StringRef Info : CurInfoSection) {
+        if (FoundCUUnit)
----------------
dblaikie wrote:
> 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.
Yes, we need to search for it to properly prune the section offsets as you noted below.


================
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;
----------------
dblaikie wrote:
> 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?
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 [[ http://dwarfstd.org/doc/DWARF5.pdf | 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!


================
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");
----------------
dblaikie wrote:
> Test coverage?
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102312