[PATCH] D87018: [DebugInfo] Fix emitting DWARF64 DWO compilation units and string offset tables (9/19).

James Henderson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Sep 4 03:36:22 PDT 2020


jhenderson accepted this revision.
jhenderson added a comment.

LGTM, with your suggested changes to the length check.



================
Comment at: llvm/test/DebugInfo/X86/debug-info-dwo-dwarf64.ll:7
+; CHECK:      .debug_info.dwo contents:
+; CHECK-NEXT: Compile Unit: length = 0x{{([[:xdigit:]]{16})}}, format = DWARF64, version = 0x0005, unit_type = DW_UT_split_compile, abbr_offset = 0x0000, addr_size = 0x08
+
----------------
ikudrin wrote:
> jhenderson wrote:
> > ikudrin wrote:
> > > jhenderson wrote:
> > > > ikudrin wrote:
> > > > > dblaikie wrote:
> > > > > > Perhaps the abbr_offset should be printed with more digits to reflect the longer encoding size? (I think that's been the general approach elsewhere in the dump output format, yes?)
> > > > > As I don't care about the exact length that much, and as the `abbr_offset` was reported as 4-digit before my changes, and as abbreviation tables are not expected to be longer in DWARF64 compared to DWARF32, I did not change the format of that field. For me, the most reliable part is the format reported directly, and also the fact that the parser can correctly read all the expected information.
> > > > Optional more concise alternative to the existing regex, making use of FileCheck's new precision for numeric variables (strictly speaking, this pattern would allow a 17-digit number with no leading 0s, but that might be beyond what's needed). Not sure if you'd prefer it or not - happy either way.
> > > Maybe it is better to simplify this CHECK line even more? We do not care about the exact value of the length anyway.
> > Works for me too, if we've got some coverage showing the dumping code correctly formats the length field for a compile unit using 64-bit DWARF?
> You mean, like `llvm/test/tools/llvm-dwarfdump/X86/debug_info_min_dwarf64.s`?
That should suffice, yes :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87018



More information about the llvm-commits mailing list