[PATCH] D21503: [llvm-dwarfdump] - Teach dwarfdump to dump gdb-index section.

[David Blaikie via llvm-commits]

dblaikie added inline comments.

================
Comment at: include/llvm/DebugInfo/DWARF/DWARFGdbIndex.h:26
@@ +25,3 @@
+
+  struct CompUnit {
+    uint64_t Offset; // Offset of a CU in the .debug_info section
----------------
Rather than having another variation of the name 'dwarf compile unit', perhaps this name should have something that makes it clear it's an entyr in the index/table - maybe add 'Entry' as a suffix as with the other entry types defined below?

================
Comment at: include/llvm/DebugInfo/DWARF/DWARFGdbIndex.h:30
@@ +29,3 @@
+  };
+  std::vector<CompUnit> CuList;
+
----------------
SmallVector? (similarly for the others)

================
Comment at: lib/DebugInfo/DWARF/DWARFGdbIndex.cpp:93-94
@@ +92,4 @@
+  Version = Data.getU32(&Offset);
+  if (Version != 7)
+    report_fatal_error("only dumping of version 7 is supported");
+
----------------
Rather than a fatal error, might be better to be able to continue. (check out how error handling is done for invalid DWARF DIEs, etc - though I think that might be /too/ silent (I think it just quietly fails to parse and then does nothing), so may not be the best model/motivation)

I think some of the parse functions return a boolean about whether they succeeded - you could have it return an llvm::Error that's more descriptive and handle that in the dumper.

================
Comment at: lib/DebugInfo/DWARF/DWARFGdbIndex.cpp:102
@@ +101,3 @@
+
+  assert(Offset == CuListOffset);
+  uint32_t CuListSize = (CuTypesOffset - CuListOffset) / 16;
----------------
Rather than asserting, this should be a conditional check (& another error path/return) - the input may be bogus and we shouldn't assert just because we're given a mangled input file.

================
Comment at: lib/DebugInfo/DWARF/DWARFGdbIndex.cpp:111-113
@@ +110,5 @@
+
+  // This is a sequence of triplets of 64-bit little-endian values. In a
+  // triplet, the first value is the CU offset, the second value is the type
+  // offset in the CU, and the third value is the type signature.
+  uint32_t CuTypesListSize = (AddressAreaOffset - CuTypesOffset) / 24;
----------------
Might be worth including the bit from the spec that says what this is, otherwise the description seems to be a bit context-free ("what are these triplets for?" "something to do with CUs and types, but I have to squint to guess that from the end of the description")

================
Comment at: lib/DebugInfo/DWARF/DWARFGdbIndex.cpp:116
@@ +115,3 @@
+  if (CuTypesListSize != 0)
+    report_fatal_error("dumping of CU types list is not supported");
+
----------------
Not being able to dump it seems different from halting execution if you /see/ it? Should it be ignored or a rendered in a "simplified" format ("10 CU type records found but dumping them is not currently supported" or something)? Is there a reason not to support it?

================
Comment at: test/DebugInfo/dwarfdump-dump-gdbindex.test:3-10
@@ +2,10 @@
+
+; test.cpp:
+;  double foo1;
+;  float bar1;
+;  void method1() {}
+;  int main() { return 0; }
+; test2.cpp:
+;  double foo2;
+;  char method2() {}
+; Compiled with:
----------------
What particular things is this test case exercising? It seems like it has a lot of entries in the index, when I would expect fewer/that the test could be a bit narrower in scope, but I may not be understanding what's different about each of the entities you've got in these files.

Do you need multiple different functions? multiple different basic types? (I assume it doesn't really need a 'main', but if you need a function, main could be that function (if you don't need a function, you could link it as a shared library, rather than an executable program - so it'd still get an index, but wouldn't need main))


https://reviews.llvm.org/D21503