Page MenuHomePhabricator

[llvm-objdump] Warn if no user specified sections (-j) are found.
ClosedPublic

Authored by ychen on Jun 25 2019, 10:27 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ychen created this revision.Jun 25 2019, 10:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 25 2019, 10:27 AM
MaskRay added inline comments.Jun 25 2019, 7:48 PM
llvm/tools/llvm-objdump/llvm-objdump.cpp
451 ↗(On Diff #206481)

I remember @jhenderson said the warning order did not matter.

Then, why don't we use the simpler for (StringRef S : MissingSections)?

jhenderson added inline comments.Jun 26 2019, 2:07 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
451 ↗(On Diff #206481)

Yes, exactly. I don't see a particular reason to follow GNU on warning ordering or even text, as long as what we say makes sense.

By the way, you've got a double-negative in your patch title: "Warn if no user specified sections (-j) are not found.". I think you can get rid of the "not".

ychen retitled this revision from [llvm-objdump] Warn if no user specified sections (-j) are not found. to [llvm-objdump] Warn if no user specified sections (-j) are found..Jun 26 2019, 9:05 AM
ychen updated this revision to Diff 206705.Jun 26 2019, 10:19 AM
  • update
ychen updated this revision to Diff 206706.Jun 26 2019, 10:23 AM
  • update
ychen marked 2 inline comments as done.Jun 26 2019, 10:24 AM
grimar added inline comments.Jun 27 2019, 1:11 AM
llvm/test/tools/llvm-objdump/warn-missing-section.test
21 ↗(On Diff #206706)

This comment needs an update now.

63 ↗(On Diff #206706)

You do not need Content and Flags here and below I think.

llvm/tools/llvm-objdump/llvm-objdump.cpp
349 ↗(On Diff #206706)

I'd add a comment explaining when String can be empty.
Also, String is not a good variable name. Should it be Name or SecName may be?

443 ↗(On Diff #206706)

Instead, can you report a warning right here?

for (StringRef S : FilterSections) {
  if (FoundSectionSet.count(S))
    return;
  warn("section '" + S + "' mentioned in a -j/--section option, but not "
    "found in any input file");
}
grimar added inline comments.Jun 27 2019, 1:16 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
443 ↗(On Diff #206706)

Ah, nevermind. I see why you can't now. Please ignore this comment.

I'm happy with this once @grimar's comments have been addressed.

ychen updated this revision to Diff 206903.Jun 27 2019, 12:13 PM
  • update
ychen marked 5 inline comments as done.Jun 27 2019, 12:14 PM
MaskRay added inline comments.Jun 27 2019, 9:45 PM
llvm/tools/llvm-objdump/llvm-objdump.cpp
448 ↗(On Diff #206903)

Nit: auto -> StringRef

ychen updated this revision to Diff 206999.Jun 28 2019, 12:03 AM
  • update
ychen marked an inline comment as done.Jun 28 2019, 12:03 AM
MaskRay accepted this revision.Jun 28 2019, 12:11 AM

LG if @grimar also accepts this.

This revision is now accepted and ready to land.Jun 28 2019, 12:11 AM
grimar accepted this revision.Jun 28 2019, 1:42 AM

LGTM

jhenderson added inline comments.Jun 28 2019, 1:44 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
349 ↗(On Diff #206999)

Nit: The SHT_NULL...

But really, does this actually matter? I think it's harmless having it in the list right?

Happy for it to go in, if there's a good reason, or with this case removed.

MaskRay added inline comments.Jun 28 2019, 4:04 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
349 ↗(On Diff #206999)

I think you meant the section with index 0 does not have a name.

ychen updated this revision to Diff 207087.Jun 28 2019, 10:03 AM
ychen marked an inline comment as done.
  • update
ychen marked 2 inline comments as done.Jun 28 2019, 10:05 AM
ychen added inline comments.
llvm/tools/llvm-objdump/llvm-objdump.cpp
349 ↗(On Diff #206999)

Thank you. I should have been more clear here. The reason is that StringSet does not allow empty keys (assertion) and section 0 has no name.

Comment updated.

Not that I think it is a likely case, but does GNU objdump allow -j with an empty section name? In other words, can you do this: objdump -D -j "" test.o to dump the contents of all unnamed sections, for example? Since the section index 0 section always has no name, we don't need to (and shouldn't) warn for this case, so we need a test for that too.

llvm/tools/llvm-objdump/llvm-objdump.cpp
349 ↗(On Diff #207087)

Actually, it's theoretically valid for other sections to have no name too, so perhaps the comment needs updating to say "so avoid adding sections with no name (such as the section with index 0) here". I'd also recommend making the index term explicit like I just did.

ychen updated this revision to Diff 207350.Jul 1 2019, 9:19 AM
ychen marked an inline comment as done.
  • update
ychen marked an inline comment as done.Jul 1 2019, 9:20 AM
This revision was automatically updated to reflect the committed changes.