This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Added support of --gdb-index command line option.
AbandonedPublic

Authored by grimar on Sep 6 2016, 9:48 AM.

Details

Reviewers
ruiu
rafael
Summary

Large applications compiled with debug information experience slow link times, possible out-of-memory conditions at link time, and slow gdb startup times. In addition, they can contribute to significant increases in storage requirements, and additional network latency when transferring files in a distributed build environment.

gold linker supports gdb-index option to solve this:

Use the gold linker's --gdb-index option (-Wl,--gdb-index when linking with gcc or g++) at link time to create the .gdb_index section that allows GDB to locate and read the .dwo files as it needs them. It is used in combination with gcc -gsplit-dwarf option to enable the generation of split DWARF at compile time.

This patch is initial implementation of feature above. At this stage it is able to generate .gdb_index section for trivial helloworld application. Most of work is already done though I think.
Testcase uses object file produced by gcc for demonstration. Contents of .gdb_index produced by gold and lld are almost fully equal (string name offsets are different, which is expected and fine I believe).

There is no many information about the feature, so next sources were used as reference:

Diff Detail

Event Timeline

grimar updated this revision to Diff 70421.Sep 6 2016, 9:48 AM
grimar retitled this revision from to [ELF] - Added support of --gdb-index command line option..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar, evgeny777.
ruiu edited edge metadata.Sep 6 2016, 2:38 PM

It is a very large patch that has almost no comment, so it is hard to read (I didn't read through till end.) Non-code part is as important as code. It really needs overview comments so that first-time readers of the code like me can get an idea what you are trying to do with the code and what your intention is. What is the expected output, and what kinds of inputs are used to create the desired output? Why do we need such output in the first place? You want to explain these kind of things in this patch like writing a blog article about this feature.

ELF/OutputSections.cpp
65

parse

ELF/SplitDebugInfo.h
1

Fix the comment.

180

Please move definitions to the .cpp file.

emaste added a subscriber: emaste.Sep 6 2016, 4:35 PM
emaste added a comment.Sep 6 2016, 4:47 PM

Why do we need such output in the first place?

-gsplit-dwarf and --gdb-index support makes for an improved developer experience when building large applications with debug info (and using gdb). It keeps each object file's debug info in a separate .dwo file, and debug info is largely excluded from linking.

Details at https://gcc.gnu.org/wiki/DebugFission.

grimar marked 2 inline comments as done.Sep 7 2016, 9:03 AM
In D24267#535243, @ruiu wrote:

It is a very large patch that has almost no comment, so it is hard to read (I didn't read through till end.) Non-code part is as important as code. It really needs overview comments so that first-time readers of the code like me can get an idea what you are trying to do with the code and what your intention is. What is the expected output, and what kinds of inputs are used to create the desired output? Why do we need such output in the first place? You want to explain these kind of things in this patch like writing a blog article about this feature.

Yep, sorry for lack of comments.
I am working on that, also going to try to simplify a little where possible before pushing an update.
Hope to upload new version till the end of this week.

grimar updated this revision to Diff 70715.Sep 8 2016, 9:24 AM
grimar edited edge metadata.
  • Addressed review comments.
  • Made minor cleanups/simplifications.
  • Added comments.
ruiu added a comment.Sep 9 2016, 11:55 AM

I'm still reading this patch and haven't got the whole picture yet. A few swing-by comments. Please do it incrementally next time. You don't need to make up a complete, working code from day one -- but you can incrementally build a feature in the public repository, and it is actually more appreciated way of doing it than dropping a complete large patch out of the blue.

ELF/Options.td
86–87

Sort. Write it in one line.

ELF/OutputSections.cpp
82

Is CuListOffset always 6*4=24? If so, I don't see a reason to make this a variable.

In D24267#538684, @ruiu wrote:

I'm still reading this patch and haven't got the whole picture yet. A few swing-by comments. Please do it incrementally next time. You don't need to make up a complete, working code from day one -- but you can incrementally build a feature in the public repository, and it is actually more appreciated way of doing it than dropping a complete large patch out of the blue.

I am really sorry, do not want inconvenience for anyone. My suggestion is next then. What if I`ll split this patch to several ? I mean them will not be workable, first one can just generate the gdb_index section itself and some part of data. So it can be a sequence of patches which will make the functionality workable after the last one is committed.
What do you think ?

grimar abandoned this revision.Nov 7 2016, 3:04 AM

D24706, D25821, D26283 that uses llvm dwarf parsers were uploaded instead.