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

[ychen]

Summary

Match GNU objdump.

https://bugs.llvm.org/show_bug.cgi?id=41898

[MaskRay]

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]

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.

[jhenderson]

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]

• update

[ychen]

• update

[grimar]

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]

llvm/tools/llvm-objdump/llvm-objdump.cpp

443 ↗

(On Diff #206706)

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

[jhenderson]

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

[ychen]

• update

[MaskRay]

llvm/tools/llvm-objdump/llvm-objdump.cpp

448 ↗

(On Diff #206903)

Nit: auto -> StringRef

[ychen]

• update

[MaskRay]

LG if @grimar also accepts this.

[grimar]

LGTM

[jhenderson]

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]

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]

• update

[ychen]

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.

[jhenderson]

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]

• update

[jhenderson]

LGTM.

llvm/trunk/test/tools/llvm-objdump/warn-missing-section.test

+## This test checks the warning message when no user specified sections are
+## found in the object file.
+
+## Test relocatable file.
+# RUN: yaml2obj --docnum=1 %s -o %t.1.o
+# RUN: yaml2obj --docnum=2 %s -o %t.2.o
+
+## - Warn for one section is found case.
+# RUN: llvm-objdump --reloc --section=single %t.2.o 2>&1 \
+# RUN:   | FileCheck --check-prefix=SINGLE-WARN %s
+
+## - Don't warn twice for duplicate missing sections.
+# RUN: llvm-objdump --reloc --section=single --section=single %t.2.o 2>&1 \
+# RUN:   | FileCheck --check-prefixes=SINGLE-WARN --implicit-check-not=warning: %s
+
+## - Don't warn if any user specified section is found.
+# RUN: llvm-objdump --reloc --section=.text1 --section=.text2 %t.2.o \
+# RUN:   | FileCheck --implicit-check-not=warning: %s
+
+## - Warn for each specified section if none of them are found.
+# RUN: llvm-objdump --reloc --section=multi1 --section=multi2 %t.2.o 2>&1 \
+# RUN:   | FileCheck --check-prefix=MULTI-WARN %s
+
+## - Don't warn if the specified section has no name.
+# RUN: llvm-objdump --reloc --section="" %t.2.o 2>&1 \
+# RUN:   | FileCheck --implicit-check-not=warning: %s
+
+## - Warning for --section is applicable for various other options.
+# RUN: llvm-objdump --section-headers --section=single %t.2.o 2>&1 \
+# RUN:   | FileCheck --check-prefix=SINGLE-WARN %s
+# RUN: llvm-objdump --full-contents --section=single %t.2.o 2>&1 \
+# RUN:   | FileCheck --check-prefix=SINGLE-WARN %s
+
+## Test archive file.
+# RUN: rm -f %t.a
+# RUN: llvm-ar rc %t.a %t.1.o %t.2.o
+
+## - Warn for one section is found case.
+# RUN: llvm-objdump --reloc --section=single %t.a 2>&1 \
+# RUN:   | FileCheck --check-prefix=SINGLE-WARN %s
+
+## - Don't warn if any user specified section is found.
+# RUN: llvm-objdump --reloc --section=.text1 %t.a \
+# RUN:   | FileCheck --implicit-check-not=warning: %s
+
+## - Warn for each specified section if none of them are found.
+# RUN: llvm-objdump --reloc --section=multi1 --section=multi2 %t.a 2>&1 \
+# RUN:   | FileCheck --check-prefix=MULTI-WARN %s
+
+
+# SINGLE-WARN: warning: section 'single' mentioned in a -j/--section option, but not found in any input file
+# MULTI-WARN:      warning: section 'multi1' mentioned in a -j/--section option, but not found in any input file
+# MULTI-WARN-NEXT: warning: section 'multi2' mentioned in a -j/--section option, but not found in any input file
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+- Name:    .text1
+  Type:    SHT_PROGBITS
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+- Name:    .text2
+  Type:    SHT_PROGBITS

llvm/trunk/tools/llvm-objdump/llvm-objdump.cpp

static cl::opt<bool>
     Wide("wide", cl::desc("Ignored for compatibility with GNU objdump"),
          cl::cat(ObjdumpCat));
 static cl::alias WideShort("w", cl::Grouping, cl::aliasopt(Wide));
 
 static cl::extrahelp
     HelpResponse("\nPass @FILE as argument to read options from FILE.\n");
 
 static StringSet<> DisasmFuncsSet;
+static StringSet<> FoundSectionSet;
 static StringRef ToolName;
 
 typedef std::vector<std::tuple<uint64_t, StringRef, uint8_t>> SectionSymbolsTy;
 
 static bool shouldKeep(object::SectionRef S) {
   if (FilterSections.empty())
     return true;
-  StringRef String;
-  std::error_code error = S.getName(String);
+  StringRef SecName;
+  std::error_code error = S.getName(SecName);
   if (error)
     return false;
-  return is_contained(FilterSections, String);
+  // StringSet does not allow empty key so avoid adding sections with
+  // no name (such as the section with index 0) here.
+  if (!SecName.empty())
+    FoundSectionSet.insert(SecName);
+  return is_contained(FilterSections, SecName);
 }
 
 SectionFilter ToolSectionFilter(object::ObjectFile const &O) {
   return SectionFilter([](object::SectionRef S) { return shouldKeep(S); }, O);
 }
 
 void error(std::error_code EC) {
   if (!EC)
LLVM_ATTRIBUTE_NORETURN void report_error(Error E, StringRef ArchiveName,
   // archive instead of "???" as the name.
   if (!NameOrErr) {
     consumeError(NameOrErr.takeError());
     report_error(std::move(E), ArchiveName, "???", ArchitectureName);
   } else
     report_error(std::move(E), ArchiveName, NameOrErr.get(), ArchitectureName);
 }
 
+static void warnOnNoMatchForSections() {
+  SetVector<StringRef> MissingSections;
+  for (StringRef S : FilterSections) {
+    if (FoundSectionSet.count(S))
+      return;
+    // User may specify a unnamed section. Don't warn for it.
+    if (!S.empty())
+      MissingSections.insert(S);
+  }
+
+  // Warn only if no section in FilterSections is matched.
+  for (StringRef S : MissingSections)
+    warn("section '" + S + "' mentioned in a -j/--section option, but not "
+         "found in any input file");
+}
+
 static const Target *getTarget(const ObjectFile *Obj = nullptr) {
   // Figure out the target triple.
   Triple TheTriple("unknown-unknown-unknown");
   if (TripleName.empty()) {
     if (Obj)
       TheTriple = Obj->makeTriple();
   } else {
     TheTriple.setTriple(Triple::normalize(TripleName));
if (!ArchiveHeaders && !Disassemble && DwarfDumpType == DIDT_Null &&
     return 2;
   }
 
   DisasmFuncsSet.insert(DisassembleFunctions.begin(),
                         DisassembleFunctions.end());
 
   llvm::for_each(InputFilenames, dumpInput);
 
+  warnOnNoMatchForSections();
+
   return EXIT_SUCCESS;
 }