This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readelf] Stop suppressing static symbol output when --dyn-symbols is specified with --symbols
ClosedPublic

Authored by jhenderson on Jan 21 2019, 6:52 AM.

Details

Summary

In rL287786, a bug was introduced into llvm-readelf where it didn't print the static symbol table if both --symbols and --dyn-symbols was specified, even if there was no dynamic symbol table. This is obviously incorrect.

This patch fixes this issue, by delegating the decision of what static and dynamic symbol tables should be printed to the final dumper, rather than trying to decide in the command-line option handling layer. The decision was made to follow the approach taken in this patch because the LLVM style dumper uses a different order to the original GNU style behaviour (and GNU readelf). Other approaches resulted in behaviour changes for other dumpers which felt wrong. In particular, I wanted to avoid changing the order of the output for --symbols --dyn-symbols for LLVM style, keep what is emitted by --symbols unchanged for all dumpers, and to avoid having different orders of .dynsym and .symtab dumping for GNU --symbols and --symbols --dyn-symbols.

Diff Detail

Repository
rL LLVM

Event Timeline

jhenderson created this revision.Jan 21 2019, 6:52 AM
sbc100 added inline comments.Jan 21 2019, 7:49 PM
tools/llvm-readobj/COFFDumper.cpp
1397 ↗(On Diff #182788)

Why not keep this as the default implementation rather than duplicating it 3 times? Of the 4 object formats it looks like only ELF needs different behaviour.

jhenderson marked an inline comment as done.Jan 22 2019, 6:31 AM
jhenderson added inline comments.
tools/llvm-readobj/COFFDumper.cpp
1397 ↗(On Diff #182788)

The aim was to remove printDynamicSymbols completely from the public interface, but I guess I could put it as a private member in the interface instead.

jhenderson edited the summary of this revision. (Show Details)

Add default implementation of printSymbols, and rebase following landing of D56910/rL351789.

rupprecht accepted this revision.Jan 22 2019, 4:00 PM

LGTM with just a question

tools/llvm-readobj/ELFDumper.cpp
3181 ↗(On Diff #182934)

IIUC, printSymbols(/*PrintSymbols=*/true, /*PrintDynamicSymbols=*/false) will still print dynamic symbols here, is that intentional? I think it is, based on the patch description of rL287786 -- if so, can you leave a comment somewhere, maybe also mention it in command line help string?

This revision is now accepted and ready to land.Jan 22 2019, 4:00 PM
grimar accepted this revision.Jan 23 2019, 1:18 AM

LGTM too, with Jordan's comment addressed.

jhenderson marked an inline comment as done.Jan 23 2019, 1:44 AM
jhenderson added inline comments.
tools/llvm-readobj/ELFDumper.cpp
3181 ↗(On Diff #182934)

Sure, I can add a comment. It is intentional, since GNU readelf prints both static and dynamic symbol tables in this case. Not sure I can easily mention it in the help string, since it only affects GNU output style and not LLVM style, but I'll see if I can come up with a way.

Add comment and update help text.

Still LGTM.

This revision was automatically updated to reflect the committed changes.