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

Mon Sep 12 16:18:50 PDT 2016

On Mon, Sep 12, 2016 at 5:40 AM George Rimar <grimar at accesssoftek.com>
wrote:

> 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.
>

dump is void, but parse is not - though it seems like parse failures, as I
said, usually just result in not printing anything.

If you want to save off the state and print a failure, that's cool. Test
cases desired, etc.

>
> ================
> 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.
>

Thanks!

Ah, the comments are a bit confusing (just English grammar issues I think).
Maybe something as simple as:

// CU Types are no longer needed as DWARF skeleton type units never made it
into the standard.

>
> ================
> 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.

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.

> 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.
>

I'm interested in making sure the test for parsing is focused on that and
it's clear what's being tested and why.

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?

>
>
> https://reviews.llvm.org/D21503
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160912/ba8b706f/attachment.html>