<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Mon, Sep 12, 2016 at 5:40 AM George Rimar <<a href="mailto:grimar@accesssoftek.com">grimar@accesssoftek.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">grimar added a comment.<br class="gmail_msg">
<br class="gmail_msg">
First of all, thank you for review, David ! My comments are below.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/DebugInfo/DWARF/DWARFGdbIndex.h:26<br class="gmail_msg">
@@ +25,3 @@<br class="gmail_msg">
+<br class="gmail_msg">
+  struct CompUnit {<br class="gmail_msg">
+    uint64_t Offset; // Offset of a CU in the .debug_info section<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> 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?<br class="gmail_msg">
Done.<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: include/llvm/DebugInfo/DWARF/DWARFGdbIndex.h:30<br class="gmail_msg">
@@ +29,3 @@<br class="gmail_msg">
+  };<br class="gmail_msg">
+  std::vector<CompUnit> CuList;<br class="gmail_msg">
+<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> SmallVector? (similarly for the others)<br class="gmail_msg">
Done.<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/DebugInfo/DWARF/DWARFGdbIndex.cpp:93-94<br class="gmail_msg">
@@ +92,4 @@<br class="gmail_msg">
+  Version = Data.getU32(&Offset);<br class="gmail_msg">
+  if (Version != 7)<br class="gmail_msg">
+    report_fatal_error("only dumping of version 7 is supported");<br class="gmail_msg">
+<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> 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)<br class="gmail_msg">
><br class="gmail_msg">
> 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.<br class="gmail_msg">
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.<br class="gmail_msg"></blockquote><div><br></div><div>dump is void, but parse is not - though it seems like parse failures, as I said, usually just result in not printing anything.<br><br>If you want to save off the state and print a failure, that's cool. Test cases desired, etc.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/DebugInfo/DWARF/DWARFGdbIndex.cpp:102<br class="gmail_msg">
@@ +101,3 @@<br class="gmail_msg">
+<br class="gmail_msg">
+  assert(Offset == CuListOffset);<br class="gmail_msg">
+  uint32_t CuListSize = (CuTypesOffset - CuListOffset) / 16;<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> 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.<br class="gmail_msg">
Agree, done.<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/DebugInfo/DWARF/DWARFGdbIndex.cpp:111-113<br class="gmail_msg">
@@ +110,5 @@<br class="gmail_msg">
+<br class="gmail_msg">
+  // This is a sequence of triplets of 64-bit little-endian values. In a<br class="gmail_msg">
+  // triplet, the first value is the CU offset, the second value is the type<br class="gmail_msg">
+  // offset in the CU, and the third value is the type signature.<br class="gmail_msg">
+  uint32_t CuTypesListSize = (AddressAreaOffset - CuTypesOffset) / 24;<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> 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")<br class="gmail_msg">
Please see my comment below.<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/DebugInfo/DWARF/DWARFGdbIndex.cpp:116<br class="gmail_msg">
@@ +115,3 @@<br class="gmail_msg">
+  if (CuTypesListSize != 0)<br class="gmail_msg">
+    report_fatal_error("dumping of CU types list is not supported");<br class="gmail_msg">
+<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> 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?<br class="gmail_msg">
Yes there is a reason, sorry that did not mention it when wrote this patch initially, I updated the comments.<br class="gmail_msg"></blockquote><div><br>Thanks!<br><br>Ah, the comments are a bit confusing (just English grammar issues I think). Maybe something as simple as:<br><br>// CU Types are no longer needed as DWARF skeleton type units never made it into the standard.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: test/DebugInfo/dwarfdump-dump-gdbindex.test:3-10<br class="gmail_msg">
@@ +2,10 @@<br class="gmail_msg">
+<br class="gmail_msg">
+; test.cpp:<br class="gmail_msg">
+;  double foo1;<br class="gmail_msg">
+;  float bar1;<br class="gmail_msg">
+;  void method1() {}<br class="gmail_msg">
+;  int main() { return 0; }<br class="gmail_msg">
+; test2.cpp:<br class="gmail_msg">
+;  double foo2;<br class="gmail_msg">
+;  char method2() {}<br class="gmail_msg">
+; Compiled with:<br class="gmail_msg">
----------------<br class="gmail_msg">
dblaikie wrote:<br class="gmail_msg">
> 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.<br class="gmail_msg">
><br class="gmail_msg">
> 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))<br class="gmail_msg">
I see what you mean, I find my solution to be reasonable, sorry.<br class="gmail_msg">
<br class="gmail_msg">
If you take a look on D24267 which adds support of .gdb_index to lld or even if we talk about format itself: <a href="https://sourceware.org/gdb/onlinedocs/gdb/Index-Section-Format.html" rel="noreferrer" class="gmail_msg" target="_blank">https://sourceware.org/gdb/onlinedocs/gdb/Index-Section-Format.html</a><br class="gmail_msg">
We can notice that format is not just simple.<br class="gmail_msg">
<br class="gmail_msg">
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.</blockquote><div><br></div><div>That's probably not something to test here, though - this dumps the contents of the table even if the hashing/filling is incorrect. In the implementation of the index, dumping complex cases that trigger the various hashing behavior would be appropriate.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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.<br class="gmail_msg">
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.<br class="gmail_msg"></blockquote><div><br></div><div>I'm interested in making sure the test for parsing is focused on that and it's clear what's being tested and why.<br><br>To make this more concrete: What benefit does having double, float, and int in the first file provide? I see that double is a shared type, float and int are non-shared types - would one suffice, rather than two? Is there interesting coverage in having a non-shared type in both files (float or int in one, bool in the other) or would one suffice? </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D21503" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D21503</a><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div></div>