This is an archive of the discontinued LLVM Phabricator instance.

[llvm-dwarfdump] - Teach dwarfdump to dump gdb-index section.
ClosedPublic

Authored by grimar on Jun 19 2016, 4:10 AM.

Details

Summary

gold linker's --gdb-index option currently is able to create the .gdb_index section that allows GDB to locate and read the .dwo files as it needs them,
this helps reduce the total size of the object files processed by the linker.

More info about that:
https://gcc.gnu.org/wiki/DebugFission
https://sourceware.org/gdb/onlinedocs/gdb/Index-Section-Format.html

Patch teaches dwarfdump tool to dump this section.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 61197.Jun 19 2016, 4:10 AM
grimar retitled this revision from to [llvm-dwarfdump] - Teach dwarfdump to dump gdb-index section..
grimar updated this object.
grimar added reviewers: rafael, ruiu, dblaikie, davide.
grimar added subscribers: llvm-commits, grimar.
davide added inline comments.Jun 19 2016, 12:50 PM
include/llvm/DebugInfo/DIContext.h
130 ↗(On Diff #61197)

Unsorted?

include/llvm/DebugInfo/DWARF/DWARFContext.h
46 ↗(On Diff #61197)

Ditto.

156 ↗(On Diff #61197)

You can probably group with the other get*Index()

lib/DebugInfo/DWARF/DWARFGdbIndex.cpp
12 ↗(On Diff #61197)

This include is dead (I think).

22 ↗(On Diff #61197)

I may be wrong, but this seems formatted weirdly. Did you clang-format the patch?

137 ↗(On Diff #61197)

Space after dot. Also, open addressed (without the --) maybe?

grimar updated this revision to Diff 61240.Jun 20 2016, 2:33 AM
grimar marked 6 inline comments as done.
  • Addressed review comments.

Thanks for review, Davide ! My comments below.

include/llvm/DebugInfo/DIContext.h
130 ↗(On Diff #61197)

Fixed.

include/llvm/DebugInfo/DWARF/DWARFContext.h
46 ↗(On Diff #61197)

Fixed.

156 ↗(On Diff #61197)

Fixed.

lib/DebugInfo/DWARF/DWARFGdbIndex.cpp
12 ↗(On Diff #61197)

You right. Removed.

22 ↗(On Diff #61197)

Yes, this file is formatted. What I noticed that clang-formatting is uncommon for this tool. But I guess we want it for new code.

137 ↗(On Diff #61197)

Fixed. "open - addressed" was taken from documentation (https://sourceware.org/gdb/onlinedocs/gdb/Index-Section-Format.html), I do not know is better.

davide added inline comments.Jun 22 2016, 12:35 PM
include/llvm/DebugInfo/DWARF/DWARFContext.h
227 ↗(On Diff #61240)

Still unsorted.

280 ↗(On Diff #61240)

Still unsorted.

327 ↗(On Diff #61240)

Still unsorted.

grimar updated this revision to Diff 61647.Jun 23 2016, 1:42 AM
grimar marked 3 inline comments as done.
  • Addressed Davide's review comments.
include/llvm/DebugInfo/DWARF/DWARFContext.h
227 ↗(On Diff #61240)

Fixed.

280 ↗(On Diff #61240)

Fixed.

327 ↗(On Diff #61240)

Fixed.

tools/llvm-dwarfdump/llvm-dwarfdump.cpp
76 ↗(On Diff #61240)

Also fixed here. Though in this place I would sort all options alphabetically probably. Without trying to group them.

davide added inline comments.Jul 3 2016, 9:51 PM
include/llvm/DebugInfo/DWARF/DWARFGdbIndex.h
64 ↗(On Diff #61647)

Missing // LLVM_LIB_DEBUGINFO_DWARFGDBINDEX_H

lib/DebugInfo/DWARF/DWARFGdbIndex.cpp
99 ↗(On Diff #61647)

Same, "only dumping of version 7 is supported"

123 ↗(On Diff #61647)

A better error message would be "Dumping of CU types list is not supported"

grimar updated this revision to Diff 62665.Jul 4 2016, 4:50 AM
  • Addressed review comments.
grimar added inline comments.Jul 4 2016, 5:09 AM
include/llvm/DebugInfo/DWARF/DWARFGdbIndex.h
64 ↗(On Diff #61647)

Added. Though only one more header from 19 in the same folder has that, so it does not look common :)

davide edited edge metadata.Jul 4 2016, 8:26 AM

I don't have additional comments on this one. Please wait for Rafael before submitting.

dblaikie added inline comments.Jul 8 2016, 5:05 PM
lib/DebugInfo/DWARF/DWARFGdbIndex.cpp
70 ↗(On Diff #62665)

consider 'auto' here?

80 ↗(On Diff #62665)

Rather than using this ErrMessage string, could the parse function return an llvm::Error? (I can't remember if I've plumbed through any of that in such places yet)

147 ↗(On Diff #62665)

I'd suggest writing this (& other similar) loop as a normal for loop - more obvious what it's doing, and that there's no intended/needed side effect of changing the value of SymTableSize.

grimar updated this revision to Diff 63502.Jul 11 2016, 6:49 AM
grimar edited edge metadata.
  • Review comments addressed.
lib/DebugInfo/DWARF/DWARFGdbIndex.cpp
70 ↗(On Diff #62665)

Done.

80 ↗(On Diff #62665)

Looks it a bit more common to use report_fatal_error in dwarfdump tool. So I used that instead.

147 ↗(On Diff #62665)

Done.

grimar added a comment.Sep 9 2016, 1:27 AM

Used in D24267. Ping.

dblaikie added inline comments.Sep 9 2016, 8:02 AM
include/llvm/DebugInfo/DWARF/DWARFGdbIndex.h
26 ↗(On Diff #63502)

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?

30 ↗(On Diff #63502)

SmallVector? (similarly for the others)

lib/DebugInfo/DWARF/DWARFGdbIndex.cpp
93–94 ↗(On Diff #63502)

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.

102 ↗(On Diff #63502)

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.

111–113 ↗(On Diff #63502)

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")

116 ↗(On Diff #63502)

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?

test/DebugInfo/dwarfdump-dump-gdbindex.test
3–10 ↗(On Diff #63502)

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

grimar updated this revision to Diff 70998.Sep 12 2016, 5:39 AM
  • Addressed review comments.

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

include/llvm/DebugInfo/DWARF/DWARFGdbIndex.h
26 ↗(On Diff #63502)

Done.

30 ↗(On Diff #63502)

Done.

lib/DebugInfo/DWARF/DWARFGdbIndex.cpp
93–94 ↗(On Diff #63502)

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.

102 ↗(On Diff #63502)

Agree, done.

111–113 ↗(On Diff #63502)

Please see my comment below.

116 ↗(On Diff #63502)

Yes there is a reason, sorry that did not mention it when wrote this patch initially, I updated the comments.

test/DebugInfo/dwarfdump-dump-gdbindex.test
3–10 ↗(On Diff #63502)

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.

grimar updated this revision to Diff 71750.Sep 18 2016, 7:28 AM
  • Addressed comments
  • Reduced testcase

I think this is OK to land. David?

dblaikie accepted this revision.Sep 22 2016, 3:10 PM
dblaikie edited edge metadata.

Looks alright - some basic cleanup to do before committing.

(we might consider the format changing over time to make it a bit more terse/legible about what it's meant to be describing rather than the mechanical format that it's using to represent that (eg: dumping the string offsets might not be generically useful, just printing the strings might suffice))

lib/DebugInfo/DWARF/DWARFGdbIndex.cpp
114 ↗(On Diff #71750)

Prefer 'i' (lowercase) for loop indexes

128 ↗(On Diff #71750)

Prefer 'i' for loop indexes.

146 ↗(On Diff #71750)

Prefer 'i' for loop indexes

157 ↗(On Diff #71750)

Generally we use 'i' for loop indexes.

158–159 ↗(On Diff #71750)

Maybe just emplace_back()?

160–161 ↗(On Diff #71750)

Maybe auto?

This revision is now accepted and ready to land.Sep 22 2016, 3:10 PM

Looks alright - some basic cleanup to do before committing.

Thanks ! I'll commit soon.

(we might consider the format changing over time to make it a bit more terse/legible about what it's meant to be describing rather than the mechanical format that it's using to represent that (eg: dumping the string offsets might not be generically useful, just printing the strings might suffice))

I think if format will change, we should add the testcases, but not remove/modify old ones. I think that offsets dumping is not just usefull but it is the main aim of that testcase because it is the only information we extract from binary, when the string values is what we calculate by ourselves in this dumper. So I would prefer not to print the strings values rather than ignoring offsets dumping, but I think it is useful to look at them either, that works like a proof for fact that we dumped everything correctly.

lib/DebugInfo/DWARF/DWARFGdbIndex.cpp
114 ↗(On Diff #71750)

Hmm, I`ll do that change, but it looks to be violation of LLVM codestyle for me.

This revision was automatically updated to reflect the committed changes.