[PATCH] D32779: DWARF: Implementation of v5 string offsets table (.debug_str_offsets[.dwo] section)/consumer

[David Blaikie via Phabricator via llvm-commits]

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

A few things to tidy up, but looks plausible.



================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:134
+    while (Offset - ContributionBase < ContributionSize) {
+      uint64_t StringOffset = 0;
+      OS << format("0x%8.8x: ", Offset);
----------------
Move the declaration to the first use:

  uint64_t StringOffset = getRelocatedValue(...


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:138-142
+      if (Format == DWARF32) {
+        OS << format("%8.8x\n", StringOffset);
+      } else {
+        OS << format("%16.16x\n", StringOffset);
+      }
----------------
Probably drop the {} from single line blocks - that tends to be how most of the LLVM  code is written.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:322-340
+    // If we have at least one unit with DWARF v5 or greater, we assume that
+    // the DWO string offsets section is formatted like the regular DWARF v5
+    // string offsets section, i.e. a series of contributions by compile and
+    // type units, each preceded by a header. Otherwise, we treat it as a
+    // monolithic sequence of string offsets.
+    if (getMaxVersion() >= 5)
+      dumpStringOffsetsSection(OS, "debug_str_offsets.dwo",
----------------
Might be worthsinking this all into the dumpStringOffsetsSection function? (potentially pulling out the current function contents as dumpDwarf5StringOffsetsSection or the like?)


================
Comment at: test/DebugInfo/dwarfdump-str-offsets-invalid.test:1-9
+; Verify that llvm-dwarfdump handles invalid string offset tables.
+
+RUN: llvm-dwarfdump %p/Inputs/dwarfdump-str-offsets-invalid-1.x86_64.o | FileCheck %s
+RUN: llvm-dwarfdump %p/Inputs/dwarfdump-str-offsets-invalid-2.x86_64.o | FileCheck %s
+RUN: llvm-dwarfdump %p/Inputs/dwarfdump-str-offsets-invalid-3.x86_64.o | FileCheck %s
+RUN: llvm-dwarfdump %p/Inputs/dwarfdump-str-offsets-invalid-4.x86_64.o | FileCheck %s
+
----------------
Should these print error messages of some kind?  I mean I realize most of dwarfdump is pretty relaxed about error handling,  so perhaps it doesn't make much sense to add more just here - but worth a thought.


================
Comment at: test/DebugInfo/dwarfdump-str-offsets.test:57-67
+CHECK-NEXT: 0x00000000: Contribution size = 12, Version = 5
+CHECK-NEXT: 0x00000008: 00000000
+CHECK-NEXT: 0x0000000c: 00000018
+CHECK-NEXT: 0x00000010: 00000027
+CHECK-NEXT: 0x00000014: Contribution size = 12, Version = 5
+CHECK-NEXT: 0x0000001c: 00000000
+CHECK-NEXT: 0x00000020: 00000036
----------------
Reckon it'd be worth printing the strings that these offsets refer to here? (since the strings are indirected and printed in the debug_info (which has to go through this table and then on to the strings table) it seems weird to have them missing here, maybe?


https://reviews.llvm.org/D32779