Page MenuHomePhabricator

[ELF] - Partial support of --gdb-index command line option (Part 1).
ClosedPublic

Authored by grimar on Sep 18 2016, 4:08 AM.

Details

Summary

This patch is extracted from D24267 which implements the --gdb-index option.
(aim is to reduce and split review parts)

In this patch partial gdb_index section is created.
For costructing the .gdb_index section 6 steps should be performed (details are in
SplitDebugInfo.cpp file header), this patch do first 3:

  1. Creates proper section header.
  2. Fills list of compilation units.
  3. Types CU list area is not supposed to be supported, so it is ignored and therefore

can be treated as implemented either.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
grimar updated this revision to Diff 71746.Sep 18 2016, 4:08 AM
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar, emaste, evgeny777.
davide added a subscriber: davide.

+ Adrian/Eric for the debug info side of things, as they're the expert in the area.

aprantl edited edge metadata.Sep 19 2016, 10:44 AM

Some minor nitpicks.
Eric, David, can you look at how this fits into the big picture for split debug info. support on ELF platforms?

ELF/SplitDebugInfo.cpp
10 ↗(On Diff #71746)

Any way this could be doxygen-ified?

17 ↗(On Diff #71746)

Since LLVM already supports split debug info I don't think there is a need to re-explain this here — But there might be a better place to document this: Maybe it makes sense to move the general introuction to SourceLevelDebugging.rst?

28 ↗(On Diff #71746)

Maybe better link to the DWARF 5 specification?
http://dwarfstd.org/Issues.php?type=closed4

59 ↗(On Diff #71746)

constructing

ELF/SplitDebugInfo.h
58 ↗(On Diff #71746)

Please convert all of these top-level class comments to Doxygen comments by using ///

ruiu added inline comments.Sep 19 2016, 3:53 PM
ELF/OutputSections.cpp
81 ↗(On Diff #71746)

According to the comment, CuListOffset can be a compile-time constant rather than computing at runtime here.

114 ↗(On Diff #71746)

I think you can just make a copy here (i.e. remove &)

ELF/SplitDebugInfo.cpp
10 ↗(On Diff #71746)

Part of this file comment was a copy from other document. You need to add citation.

ELF/SplitDebugInfo.h
58 ↗(On Diff #71746)

We do not use doxygen-style comments in any file in this directory. Maybe we should, but we probably should do it all at once in a different patch.

grimar updated this revision to Diff 71937.Sep 20 2016, 8:14 AM
  • Addressed review comments.
grimar added inline comments.Sep 20 2016, 8:14 AM
ELF/OutputSections.cpp
81 ↗(On Diff #71746)

Done.

114 ↗(On Diff #71746)

Done.

ELF/SplitDebugInfo.cpp
10 ↗(On Diff #71746)

As Rui mentioned, we do not use doxygen comments in lld at this moment.
Added quotes and references to source.

17 ↗(On Diff #71746)

Ok, I cut off most of information from this header and added just a very short intro for now.

28 ↗(On Diff #71746)

Done.

59 ↗(On Diff #71746)

Fixed.

grimar updated this revision to Diff 71939.Sep 20 2016, 8:25 AM
  • Last minute fix.
ruiu added inline comments.Sep 20 2016, 8:57 PM
ELF/OutputSections.cpp
65 ↗(On Diff #71939)

Please include a file name in the error message.

71 ↗(On Diff #71939)

Looks like this can be written in a more functional programming style like this.

CompilationUnits = readDwarf(I);

where readDwarf is a new function that returns a new vector.

Currently, it is not clear what DwarfInfoReader would do to its arguments.

86–110 ↗(On Diff #71939)

I'd save lines here.

write32le(Buf, 7);                   // Version
write32le(Buf + 4, CutListOffset);   // [what is this?]
write32le(Buf + 8, CutListOffset);   // Offset of the address area
write32le(Buf + 12, CutListOffset);  // ...
...
Buf += 24;

Or, maybe we should define a struct for the header in libObject/libSupport.

grimar updated this revision to Diff 72037.Sep 21 2016, 6:48 AM
  • Addressed review comments.
ELF/OutputSections.cpp
65 ↗(On Diff #71939)

Which one ? DebugInfoSec here is an output section:

if (Out<ELFT>::GdbIndex)
  Out<ELFT>::GdbIndex->DebugInfoSec = findSection(".debug_info");

It is used as a container of .debug_info sections to parse. I think generally it is ok that some object may not have it.
But I checked gold and found it just does not emit the .gdb_index instead of showing error if there is no .debug_info input sections at all, so I removed that error either.

71 ↗(On Diff #71939)

Rewrote.

86–110 ↗(On Diff #71939)

Done. I do not think struct for just a header of some output section worth that. I think we dont have any such for other output sections, and also it looks soon CuTypesOffset can be removed (as types data area is depricated), I expect to see it in version 8, do not think we want to have structs of different versions or something like that.

echristo edited edge metadata.Oct 4 2016, 3:26 PM

I think calling the file SplitDebugInfo.cpp is a bit of a misnomer, there's nothing related to fission/split-dwarf/etc in it other than a couple of references. Also not sure why we need yet another dwarf parser here rather than using the one in llvm?

Looks fine otherwise though.

-eric

ELF/SplitDebugInfo.cpp
290 ↗(On Diff #72037)

"parcer" -> "parser"

test/ELF/gdb-index.s
2 ↗(On Diff #72037)

"were generated in this way"

grimar updated this revision to Diff 73609.Oct 5 2016, 2:22 AM
grimar edited edge metadata.
  • Addressed review comments.
  • Reduced testcase.
grimar added a comment.Oct 5 2016, 2:23 AM

I think calling the file SplitDebugInfo.cpp is a bit of a misnomer, there's nothing related to fission/split-dwarf/etc in it other than a couple of references. Also not sure why we need yet another dwarf parser here rather than using the one in llvm?

Looks fine otherwise though.

-eric

Thanks for comments, Eric !

I called it SplitDebugInfo.cpp initially as this patch is a part of D24267 where this file contains whole functionality needed
by linker for generation --gdb-index, for example implementation of symbol hash table (GdbHashTab). So it is relative to fussion.
I am fine with renaming it to something if you think it is misnomer though. May be GdbIndex.cpp ?

We are trying to keep code minimal and fast in linker, but llvm parsers are generic and generally do more job than we need, short quick example:
Imaging I want to get address ranges and so I want to use DWARFDebugInfoEntryMinimal::getAddressRanges(const DWARFUnit *U).
For doing that I need DWARFUnit, which requires DWARFContext in constructor. Looking at comments that says
"DWARFContextInMemory is the simplest possible implementation of a DWARFContext.", I am opening its constructor and
see that it takes an object and do a scan over all sections uncompresses the compressed ones. But we do not need that as we already have
content of sections available.

D24267 which is full version of this functionality shows that we seems need not that lot of parsing code for --gdb-index option in total,
I think using own inplementation should allow to make it faster.

ELF/SplitDebugInfo.cpp
290 ↗(On Diff #72037)

Done.

test/ELF/gdb-index.s
2 ↗(On Diff #72037)

Done.

grimar updated this revision to Diff 74263.Oct 11 2016, 8:52 AM
  • Reimplemented using llvm dwarf parser.

If we land this, I`ll be able to prepare next patch for this functionality and also will suggest few changes for parsers as I found they lack some functionality I need for next steps.

ruiu added inline comments.Oct 17 2016, 4:38 PM
ELF/CMakeLists.txt
21 ↗(On Diff #74263)

Sort

ELF/Config.h
101–102 ↗(On Diff #74263)

Sort

ELF/GdbIndex.cpp
12 ↗(On Diff #74263)

This long comment doesn't match with what you do in this patch. You want to add this later.

ELF/GdbIndex.h
22 ↗(On Diff #74263)

You can remove this class and replace a non-member function readCuList(InputSection<ELFT> *).

ELF/OutputSections.cpp
90 ↗(On Diff #74263)

Write version -> Version for consistency.

90–95 ↗(On Diff #74263)

Remove . from comments because they are not sentences.

ELF/Writer.cpp
777 ↗(On Diff #74263)

I think you want to do the same as the other sections here and remove DebugInfoSec.

Out<ELFT>::DebugInfo = findSection(".debug_info");
grimar updated this revision to Diff 74970.Oct 18 2016, 3:42 AM
  • Addressed review comments.
ELF/CMakeLists.txt
21 ↗(On Diff #74263)

Done.

ELF/Config.h
101–102 ↗(On Diff #74263)

Done.

ELF/GdbIndex.cpp
12 ↗(On Diff #74263)

I believe it is fine. It describes what is gdb_index and what linker should do to support this feature.
And at the end it contains what we (in this patch) do now:

Current version of implementation has 1, 2, 3 steps. So it writes .gdb_index
header and list of compilation units. Since we so not plan to support types
CU list area, it is also empty and so far is "implemented".
Other data areas are not yet implemented.

So during next patches I plan to modify that last part and finally remove it when all be done.

ELF/GdbIndex.h
22 ↗(On Diff #74263)

Ok, though I am thinking about this patch as about skeleton which intention is to reduce changes for futher parts.
Removing this class much probably means I`ll need to restore it in next one anyways.

ELF/OutputSections.cpp
90 ↗(On Diff #74263)

Done.

90–95 ↗(On Diff #74263)

Done.

ELF/Writer.cpp
777 ↗(On Diff #74263)

Done.

This makes sense to me.

ruiu added inline comments.Oct 18 2016, 12:56 PM
ELF/GdbIndex.cpp
76 ↗(On Diff #74970)

Remove llvm::.

76–85 ↗(On Diff #74970)
78 ↗(On Diff #74970)

It seems that createObjectFile parses the file again. Can you avoid that?

88 ↗(On Diff #74970)

Replace auto with its concrete type.

94–101 ↗(On Diff #74970)

Remove Sec. Use uint32_t and uint64_t instead of ELF????::uint.

grimar updated this revision to Diff 75113.Oct 19 2016, 12:51 AM
  • Addressed review comments.
ELF/GdbIndex.cpp
76 ↗(On Diff #74970)

Done.

78 ↗(On Diff #74970)

I afraid current parsers API does not allow that yet.

Starting from next patch for this feature, I am planing to suggest changes for DWARF parsers API as it seems not possible to do things I need for next steps without some changes.
An parallel task might be optimization of parsers entrypoints for linker needs then, there is a room for that.

I suggest to land this one first because then it will be easy to justify the need of parsers changes and adding linker specific functionality.

88 ↗(On Diff #74970)

Done.

94–101 ↗(On Diff #74970)

Done.

ruiu accepted this revision.Oct 19 2016, 6:39 PM
ruiu edited edge metadata.

Please land this patch with the following fixes.

ELF/GdbIndex.h
15 ↗(On Diff #75113)

Move this to the .cpp file.

ELF/Options.td
96–102 ↗(On Diff #75113)

Sort.

ELF/Writer.cpp
775 ↗(On Diff #75113)

Remove the blank line and clang-format.

This revision is now accepted and ready to land.Oct 19 2016, 6:39 PM
This revision was automatically updated to reflect the committed changes.
grimar marked 3 inline comments as done.