This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj] Allow --hex-dump/--string-dump to dump multiple sections
ClosedPublic

Authored by MaskRay on Jun 17 2019, 10:09 PM.

Details

Summary
  1. -x foo currently dumps one foo. This change makes it dump all foo.
  2. -x foo -x foo currently dumps foo twice. This change makes it dump foo once. In addition, if foo has section index 9, -x foo -x 9 dumps foo once.
  3. Give a warning instead of an error if foo does not exist.

The new behaviors match GNU readelf.

Also, print a new line as a separator between two section dumps.
GNU readelf uses two lines, but one seems good enough.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Jun 17 2019, 10:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2019, 10:09 PM
MaskRay updated this revision to Diff 205255.Jun 17 2019, 10:13 PM

Delete an unused include

grimar added inline comments.Jun 18 2019, 1:53 AM
test/tools/llvm-readobj/hex-dump.test
2 ↗(On Diff #205255)

I'd suggest starting the lines from '#'. That will be useful for git history
when we convert all inputs to YAML and inline them here.

Also, you list -x .strtab twice, but -x 9 only once, so may be list it twice too?

5 ↗(On Diff #205255)

Can you add check lines showing that .symtab really has index == 9?

5 ↗(On Diff #205255)

strtab I mean.

17 ↗(On Diff #205255)

In this and below tests you just check that can use section index with -x for
the different targets, right? Please add a comment explaining that.

tools/llvm-readobj/ObjDumper.cpp
40 ↗(On Diff #205255)

Map of bool is the same as set I think. Can you use StringSet instead of StringMap<bool>?

88 ↗(On Diff #205255)

I'd:

if (First)
  W.startLine() << '\n';
First = false;
123 ↗(On Diff #205255)

The same.

tools/llvm-readobj/ObjDumper.h
107 ↗(On Diff #205255)

printSectionsAsString, printSectionsAsHex ?

grimar added inline comments.Jun 18 2019, 1:59 AM
tools/llvm-readobj/ObjDumper.cpp
40 ↗(On Diff #205255)

Oh, I misread the code. Please ignore this comment.

grimar added inline comments.Jun 18 2019, 2:05 AM
tools/llvm-readobj/ObjDumper.cpp
69 ↗(On Diff #205255)

I do not think you can do this, because StringMap iteration order is not guaranteed to be deterministic.

70 ↗(On Diff #205255)

The same. You should use std::map I think.

grimar added inline comments.Jun 18 2019, 2:07 AM
tools/llvm-readobj/ObjDumper.cpp
70 ↗(On Diff #205255)

Or, if your intention was to keep the insertion order in the outpuit, you probably need MapVector.

MaskRay updated this revision to Diff 205274.Jun 18 2019, 2:26 AM
MaskRay marked 15 inline comments as done.

Address review comments by grimar.

Add two missed llvm-objcopy tests

MaskRay updated this revision to Diff 205276.Jun 18 2019, 2:29 AM

Fix: if (!First)

test/tools/llvm-readobj/hex-dump.test
17 ↗(On Diff #205255)

This file was renamed from print-hex.test to be consistent with the option name (also, string-dump.test is not named print-string.test)

tools/llvm-readobj/ObjDumper.cpp
69 ↗(On Diff #205255)

Good catch! Changed to std::map.

70 ↗(On Diff #205255)

The logic is:

for each section:
  if section name is specified by -x or section index is specified by -x:
    dump()

The insertion order doesn't matter. I'll use std::map for simplicity.

grimar accepted this revision.Jun 18 2019, 2:40 AM

LGTM with a 2 nits/suggestions. But please hold on to see if other reviewers has something to say.

test/tools/llvm-readobj/hex-dump.test
2 ↗(On Diff #205255)

"Also, you list -x .strtab twice, but -x 9 only once, so may be list it twice too?"

This part was not addressed. I think you should list -x 9 twice too:

llvm-readobj -x 9 -x 9 -x .strtab -x .strtab
tools/llvm-readobj/ObjDumper.cpp
47 ↗(On Diff #205274)

I think we usually wrap the for body in such cases:

for (StringRef Section : Sections) {
  if (!Section.getAsInteger(0, SecIndex))
    SecIndices.emplace(SecIndex, false);
   else
    SecNames.emplace(Section, false);
}
70 ↗(On Diff #205255)

What I was thinking about is that in theory we could print the ""could not find section..." warnings in the same order as -x is used in the command line. Though and I also do not think it is really important/useful.

This revision is now accepted and ready to land.Jun 18 2019, 2:40 AM
MaskRay updated this revision to Diff 205286.Jun 18 2019, 2:47 AM
MaskRay marked 2 inline comments as done.

Address review comments

jhenderson requested changes to this revision.Jun 18 2019, 3:15 AM

Not really looked at the code yet, but I noticed a problem with the test, which I'd like addressing.

test/tools/llvm-readobj/hex-dump-multi.s
1 ↗(On Diff #205286)

Nit: I'd find it slightly easier to read if this had a blank line after.

test/tools/llvm-readobj/hex-dump.test
1 ↗(On Diff #205286)

There is already a "hexdump.test" file. The additional test cases you are adding should be added to that (or that should be deleted/renamed to hex-dump.test).

This revision now requires changes to proceed.Jun 18 2019, 3:15 AM
MaskRay updated this revision to Diff 205293.Jun 18 2019, 3:25 AM
MaskRay marked 2 inline comments as done.

Merge hexdump.test into hex-dump.test, i.e. print-hex.test hexdump.test -> hex-dump.test

jhenderson added inline comments.Jun 18 2019, 6:05 AM
test/tools/llvm-readobj/hex-dump.test
29 ↗(On Diff #205293)

Unnecessary extra line here?

tools/llvm-readobj/ObjDumper.cpp
38 ↗(On Diff #205293)

Could Sections be an ArrayRef?

40 ↗(On Diff #205293)

I may be being dumb, but why can't we use a StringMap? I'm not seeing where the ordering makes a difference. Or is it just that the output order might change arbitrarily?

44 ↗(On Diff #205293)

I don't think we want to accept negative numbers as section indices here. Can you change the type of SecIndex to an unsigned type? I think that will cause this to become an unsigned lookup.

Then again, using a signed type is probably harmless, since we'll warn later, it just is confusing looking at the code as is.

55 ↗(On Diff #205293)

Is the continue necessary? I think error() ends the program...

57 ↗(On Diff #205293)

Maybe NameIt and IndexIt instead of It and It2 might be a little clearer.

79 ↗(On Diff #205293)

ArrayRef?

113 ↗(On Diff #205293)

ArrayRef?

grimar added inline comments.Jun 18 2019, 6:11 AM
tools/llvm-readobj/ObjDumper.cpp
40 ↗(On Diff #205293)

Or is it just that the output order might change arbitrarily?

Yes, and tests can become flaky because of that.

MaskRay updated this revision to Diff 205321.Jun 18 2019, 6:35 AM

Address review comments

MaskRay marked 8 inline comments as done.Jun 18 2019, 6:36 AM
jhenderson accepted this revision.Jun 18 2019, 6:44 AM

LGTM.

tools/llvm-readobj/ObjDumper.cpp
53 ↗(On Diff #205321)

Interesting. I was not expecting error() to do nothing if passed a success! I don't mind this being left as is, but there is precedent elsewhere in this file to do:

if (...)
  error(...);

See printSectionsAsString for such an example.

This revision is now accepted and ready to land.Jun 18 2019, 6:44 AM
MaskRay marked an inline comment as done.Jun 18 2019, 6:53 AM
MaskRay added inline comments.
tools/llvm-readobj/ObjDumper.cpp
53 ↗(On Diff #205321)

I'll leave it as is:)

The error(...) pattern is much more common, especially in coff/macho dumpers..

MaskRay updated this revision to Diff 205328.Jun 18 2019, 6:55 AM

Consistently use error(...)

This revision was automatically updated to reflect the committed changes.
llvm/trunk/test/tools/llvm-readobj/hex-dump.test