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

George Rimar via llvm-commits llvm-commits at lists.llvm.org
Mon Sep 12 05:40:50 PDT 2016


grimar added a comment.

First of all, thank you for review, David ! My comments are below.


================
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
----------------
dblaikie wrote:
> 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?
Done.

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

================
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");
+
----------------
dblaikie wrote:
> 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.
Ok, I see. Actually I reviewed files in the same folder and I think dump() is always a void(). At the same time I probably agree that fatal error is far from the best solution. So I reworked it to store a error and report about it later. I think it is a bit more natural way of handling errors in such tools. I mean just storing error state of sub-component and revealing it later during printing look fine for me.

================
Comment at: lib/DebugInfo/DWARF/DWARFGdbIndex.cpp:102
@@ +101,3 @@
+
+  assert(Offset == CuListOffset);
+  uint32_t CuListSize = (CuTypesOffset - CuListOffset) / 16;
----------------
dblaikie wrote:
> 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.
Agree, done.

================
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;
----------------
dblaikie wrote:
> 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")
Please see my comment below.

================
Comment at: lib/DebugInfo/DWARF/DWARFGdbIndex.cpp:116
@@ +115,3 @@
+  if (CuTypesListSize != 0)
+    report_fatal_error("dumping of CU types list is not supported");
+
----------------
dblaikie wrote:
> 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?
Yes there is a reason, sorry that did not mention it when wrote this patch initially, I updated the comments.

================
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:
----------------
dblaikie wrote:
> 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))
I see what you mean, I find my solution to be reasonable, sorry.

If you take a look on D24267 which adds support of .gdb_index to lld or even if we talk about format itself: https://sourceware.org/gdb/onlinedocs/gdb/Index-Section-Format.html
We can notice that format is not just simple.

I mean I really want to see a few function names and few type names, and want to see some shared type, like "double" I have. At least I want it because we have symbol table, that has own rules for filling. For example I would like to check symbol table filling hash formula and even do not sure that amount of values I have in this test is enough for that. It allows to test few multiple names and few multiple types. It allows to test "double" which is shared type. Generally I do not find this test to be excessive. I think testing of few hash calculated slots is at least good to have.
Probably my words above would work more in case we would talk about generation and not about parsing index, but I think parsing is also worth to check, so I am really do not see reasons to reduce something here at least while testcase is simple like we have now.


https://reviews.llvm.org/D21503





More information about the llvm-commits mailing list