[PATCH] D75485: Support DW_FORM_strx* in llvm-dwp.

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Seems OK, I think. Few minor wording/coding changes to deal with.



================
Comment at: llvm/test/tools/llvm-dwp/X86/handle_strx.test:1
+RUN: llvm-dwp %p/../Inputs/handle_strx/dw5.dwo -o %t 2>/dev/null
+RUN: llvm-dwarfdump --verbose %t 2>/dev/null | FileCheck --check-prefix=READ_STRX %s
----------------
kimanh wrote:
> dblaikie wrote:
> > SouraVX wrote:
> > > How about instead of putting raw objects files `dw5.dwo`, put it as asm file and use `llvm-mc` to generate object and `llvm-dwarfdump` and so forth.
> > > 
> > This was already discussed here: https://reviews.llvm.org/D75485#inline-688717
> I kept tamur's test case as discussed in https://reviews.llvm.org/D75485#inline-688717 but used .s files for the new test cases. Let me know if you want me to change anything.
Yeah, I wouldn't mind if tamur's test was changed to .s, but I don't insist on it.


================
Comment at: llvm/tools/llvm-dwp/llvm-dwp.cpp:125
+    return make_error<DWPError>(
+        "compile unit header exceeds .debug_info section range: " +
+        utostr(Offset + Header.Length) + " >= " + utostr(InfoData.size()));
----------------
This suggests the header itself is too large for the debug_info section, but what it's testing is that the length specified in the header is too large - so maybe rephrasing this to something like: "compile unit exceeds .debug_info section range"? 


================
Comment at: llvm/tools/llvm-dwp/llvm-dwp.cpp:160
+  }
+  assert(!Err && "Compile unit header is missing fields");
+
----------------
This is an assertion because the length has been checked ahead of time, yeah? So there's no failure expected?

I guess the assertion was added to suppress an llvm::Error about the Error being unchecked? The problem is that in a non-assertions build that checking will fail because the Error won't be checked because the assertion isn't executed.

Perhaps it'd be OK to call the extraction functions without an Error parameter? (`getU32(&Offset)` etc) since the bounds have already been checked


================
Comment at: llvm/tools/llvm-dwp/llvm-dwp.cpp:275
+  uint64_t Offset = Header.HeaderSize;
+  if (Header.Version >= 5 && Header.UnitType != dwarf::DW_UT_split_compile)
+    return make_error<DWPError>(
----------------
(are you using/interested in DWARF type units (-fdebug-types-section)? In DWARFv5 those were moved into the .debug_info section, so this would need to be updated to handle them at some ponit)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75485