This is an archive of the discontinued LLVM Phabricator instance.

[DWARF] - Introduce DWARFDebugPubTable class for dumping pub* sections.
ClosedPublic

Authored by grimar on Dec 16 2016, 7:42 AM.

Details

Summary

It was requested during review of D26283 (which already landed) to share the code
for parsing pubnames/pubtypes.

Patch implements that parser instead of static function which was used before.
So it should be possible to reuse in LLD now and clean up the duplication code.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 81756.Dec 16 2016, 7:42 AM
grimar retitled this revision from to [DWARF] - Introduce DWARFDebugPubTable class for dumping pub* sections..
grimar updated this object.
grimar added reviewers: clayborg, dblaikie, aprantl, davide.
grimar added subscribers: llvm-commits, grimar, evgeny777, ruiu.
dblaikie accepted this revision.Dec 16 2016, 7:52 AM
dblaikie edited edge metadata.
dblaikie added inline comments.
include/llvm/DebugInfo/DWARF/DWARFDebugPubTable.h
73–74 ↗(On Diff #81756)

Probably no need to worry about using a separate range type here - just return a const ref to 'Sets' directly?

This revision is now accepted and ready to land.Dec 16 2016, 7:52 AM
aprantl added inline comments.Dec 16 2016, 8:32 AM
include/llvm/DebugInfo/DWARF/DWARFDebugPubTable.h
22 ↗(On Diff #81756)

Please doxygen-ify these comments.

clayborg accepted this revision.Dec 16 2016, 10:55 AM
clayborg edited edge metadata.

Thanks for doing this, LGTM

davide edited edge metadata.Dec 16 2016, 11:36 AM

The logic looks correct, some minor comments inline.

include/llvm/DebugInfo/DWARF/DWARFDebugPubTable.h
58 ↗(On Diff #81756)

s/Entires/Entries/ maybe?

64–67 ↗(On Diff #81756)

Add comments to explain what GnuStyle means.

lib/DebugInfo/DWARF/DWARFContext.cpp
201–217 ↗(On Diff #81756)

You can probably express this in a more compact form (and avoid some duplication) with a mapping from DumpType to the section name. Up to you.

lib/DebugInfo/DWARF/DWARFDebugPubTable.cpp
35–37 ↗(On Diff #81756)

uint8_t IndexEntryValue = (GnuStyle) ? ... : 0;

53–55 ↗(On Diff #81756)

Maybe use the ternary operator? As in

OS << (GnuStyle ? ... : ...);
65 ↗(On Diff #81756)

ehm, is this clang formatted? Probably yes, but I'm double checking because it looks weird.

This revision was automatically updated to reflect the committed changes.
grimar added inline comments.Dec 17 2016, 1:21 AM
include/llvm/DebugInfo/DWARF/DWARFDebugPubTable.h
22 ↗(On Diff #81756)

Done.

58 ↗(On Diff #81756)

Done.

64–67 ↗(On Diff #81756)

Done.

73–74 ↗(On Diff #81756)

Ok. I am returning ArrayRef now.

lib/DebugInfo/DWARF/DWARFContext.cpp
201–217 ↗(On Diff #81756)

I'll try to think how to improve that place and suggest a separate patch.

lib/DebugInfo/DWARF/DWARFDebugPubTable.cpp
35–37 ↗(On Diff #81756)

Done.

53–55 ↗(On Diff #81756)

Done.

65 ↗(On Diff #81756)

Yes, it was formatted. I rewrote this part a bit to look more straightforward,
hopefully it is better looking now.