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

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
3

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?

6

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

6

strtab I mean.

18

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

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

60

I'd:

if (First)
  W.startLine() << '\n';
First = false;
87

The same.

tools/llvm-readobj/ObjDumper.h
107–110

printSectionsAsString, printSectionsAsHex ?

grimar added inline comments.Jun 18 2019, 1:59 AM
tools/llvm-readobj/ObjDumper.cpp
40

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

grimar added inline comments.Jun 18 2019, 2:05 AM
tools/llvm-readobj/ObjDumper.cpp
53

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

54

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

grimar added inline comments.Jun 18 2019, 2:07 AM
tools/llvm-readobj/ObjDumper.cpp
54

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
18

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
53

Good catch! Changed to std::map.

54

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
3

"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

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);
}
54

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
2

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

test/tools/llvm-readobj/hex-dump.test
2

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
30

Unnecessary extra line here?

tools/llvm-readobj/ObjDumper.cpp
38

Could Sections be an ArrayRef?

40

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

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.

54

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

56–64

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

78

ArrayRef?

112

ArrayRef?

grimar added inline comments.Jun 18 2019, 6:11 AM
tools/llvm-readobj/ObjDumper.cpp
40

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
51

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
51

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.
test/tools/llvm-readobj/hex-dump.test