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

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

kimanh added a comment.

Thanks a lot for the reviews!

After reading up more on the DWARF specs, I noticed that debug abbrev offsets were not updated for dwarf 32-bit, and 64-bit. I've changed that now in the latest patch.



================
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
----------------
dblaikie wrote:
> 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.
Alright, in that case I prefer to keep it. I'll be updating the code again for type units, so in case your opinion changes I'll take it on in a follow up patch!


================
Comment at: llvm/tools/llvm-dwp/llvm-dwp.cpp:160
+  }
+  assert(!Err && "Compile unit header is missing fields");
+
----------------
dblaikie wrote:
> 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
Yes right, I added the assertion because no failure is expected, and you're right that LLVM would throw an error if that's not checked. That makes sense to remove the Error parameter in that case, thanks!


================
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>(
----------------
dblaikie wrote:
> (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)
Thanks for pointing this out! I'm currently working on a patch to add support for type units, and also for writing v5 cu/tu index sections. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75485