This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj] Provide Hash Histogram for all ELFDumper implementations
ClosedPublic

Authored by paulkirth on Oct 31 2022, 10:59 AM.

Details

Summary

Previously, the GNUELFDumper was the only implementer for Hash Histograms.
This patch moves the implementation for printHashHistogram and
printGnuHashHistogram into the ELFDumper base class, and allows each
derived class to specialize how it outputs that information.

This change also prevents the JSONELFDumper from emitting invalid JSON,
since the shared implementation in LLVMELFDumper no longer emits a
warning message to the output stream.

Diff Detail

Event Timeline

paulkirth created this revision.Oct 31 2022, 10:59 AM
Herald added a project: Restricted Project. · View Herald Transcript
paulkirth requested review of this revision.Oct 31 2022, 10:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2022, 10:59 AM

I'd rather not do this, and would strongly prefer we simply do it right the first time.

paulkirth updated this revision to Diff 484081.Dec 19 2022, 2:42 PM
paulkirth retitled this revision from [readobj] Ignore hash histogram in JSON output to [llvm-readobj] Provide Hash Histogram for all ELFDumper implementations.
paulkirth edited the summary of this revision. (Show Details)

Rebase

  • Replace empty implementation with a shared impl in the ELFDumper base class and printing APIs for derived classes to specialize.
  • Add test to check the formatting for the LLVM style output
paulkirth updated this revision to Diff 484090.Dec 19 2022, 2:49 PM

Rebase to fix patch application

paulkirth updated this revision to Diff 484092.Dec 19 2022, 2:51 PM

Fix formatting

So this patch appears to add support for the hash histogram printing for both GNU and non-GNU hash sections, but the testing only appears to test one of these two cases. I think you need tests for both cases.

llvm/include/llvm/Support/ScopedPrinter.h
233

This should probably be spun off into a separate patch and have testing in the unit tests.

llvm/test/tools/llvm-readobj/ELF/hash-histogram.test
15
llvm/tools/llvm-readobj/ELFDumper.cpp
2706

Whilst moving, please add the missing "." to this comment.

2737

Ditto.

2741

Ditto.

2787

Ditto.

7126–7128

Any particular reason you've made the Count member const in these two methods?

7133
7151
paulkirth updated this revision to Diff 502277.Mar 3 2023, 3:31 PM

Rebase.

  • split out ScopedPrinter changes.
  • remove const from ArrayRef parameters
  • fix typos in comments
  • use preincrement in loops
  • add test for both types of HashHistogram
paulkirth marked 8 inline comments as done.Mar 3 2023, 3:32 PM
paulkirth added inline comments.
llvm/tools/llvm-readobj/ELFDumper.cpp
7126–7128

Thanks for pointing that out. I think it was just habit from writing const Foo& forever, but that doesn't make sense in this case.

7151

This loop is taken directly from the GNUELFDumper, should I update this in the existing implementations too?

paulkirth updated this revision to Diff 502294.Mar 3 2023, 4:00 PM
paulkirth marked 2 inline comments as done.

Rebase

paulkirth updated this revision to Diff 502710.Mar 6 2023, 10:33 AM

Fix formatting

jhenderson added inline comments.Mar 7 2023, 12:51 AM
llvm/test/tools/llvm-readobj/ELF/hash-histogram.test
16–17

There's no need for these two extra cases: they are there for the earlier llvm-readelf check to ensure the --histogram and -I aliases are equivalent to --elf-hash-histogram.

llvm/tools/llvm-readobj/ELFDumper.cpp
7129

Looking at this again, this and the GNU hash histogram version immediately below are practically identical. Can you try sharing the code rather than duplicating it, please?

Whilst you're at it, you might want to do the same for the GNU output format versions of these two functions.

7151

I wouldn't change code that isn't already changing/moving, but if it is, you can make the tidy-up at the same time.

paulkirth updated this revision to Diff 503155.Mar 7 2023, 2:46 PM
paulkirth marked 3 inline comments as done.

Address comments.

  • Use preincrement in moved code.
  • Share more code between Gnu and non-Gnu versions
  • Remove redundant test cases
jhenderson accepted this revision.Mar 8 2023, 12:31 AM

Two minor points, otherwise LGTM.

llvm/tools/llvm-readobj/ELFDumper.cpp
2742
4958

I have a marginal preference to make this argument StringRef TableDescription (with values of "" and `.gnu.hash' respectively, since there's only one place the behaviour is different between the two output styles, as far as I can tell.

This revision is now accepted and ready to land.Mar 8 2023, 12:31 AM
paulkirth added inline comments.Mar 8 2023, 9:45 AM
llvm/tools/llvm-readobj/ELFDumper.cpp
4958

Sure, but if they have the same interface, then they can be overrides from the base class, as they are now.

jhenderson added inline comments.Mar 17 2023, 1:10 AM
llvm/tools/llvm-readobj/ELFDumper.cpp
4958

Okay, that's fair.

paulkirth updated this revision to Diff 506271.Mar 17 2023, 8:45 PM
paulkirth marked 3 inline comments as done.

Rebase.

  • fix comment string for /*IsGnu=*/
jhenderson accepted this revision.Mar 20 2023, 2:33 AM
This revision was landed with ongoing or failed builds.Mar 20 2023, 5:28 PM
This revision was automatically updated to reflect the committed changes.