[llvm-objdump] Add warning if --disassemble-functions specifies an unknown symbol

[mmpozulp]

Summary

Fixes Bug 41904 https://bugs.llvm.org/show_bug.cgi?id=41904

Re-land r362768 after it was reverted in r362826.

[mmpozulp]

I've taken a bit of a heavy-hammer approach here. In order to use set_difference() on StringSet I had to extend the StringMap and StringSet APIs. Are the downsides of having a bigger API warranted by the ability to use set_difference() on StringSet? If so, I can break it out into a different patch. An alternative would be to write this patch without calling set_difference(), but that imposes some code duplication.

[jhenderson]

I like what you're doing, and I think having count and insert operations on a StringSet make a lot of sense from a somewhat naive perspective, but I think those should be in a separate change, with unit tests and with reviewers who know that class better than I do. You can cite this review in that one for the motivation (also citing that it makes it more STL-like).

I've added a few more reviewers, partly because I'm away all of next week.

llvm/test/tools/llvm-objdump/X86/warn-missing-disasm-func.test

1 ↗

(On Diff #200837)

Nit: missing trailing full stop.

10 ↗

(On Diff #200837)

By the way, the trailing "..." isn't needed in any case I've seen so far for yaml2obj.

12 ↗

(On Diff #200837)

I think the more common approach is for warnings and errors to start with lower case and not have a trailing full stop.

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

379 ↗

(On Diff #200837)

StringRef would be more appropriate here, I think.

1412–1413 ↗

(On Diff #200837)

I think it would be clearer here if you don't use auto.

1415 ↗

(On Diff #200837)

No need for the const & for a StringRef.

[mmpozulp]

Incorporate feedback from @jhenderson, including moving ADT changes (which must land before this patch) to a separate patch https://reviews.llvm.org/D62369

[jhenderson]

Looks good, apart from one small question. I'm away after the end of the day, so you'll need to get someone else to land this for you. Alternatively, request commit access as documented here: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

FYI, you can also mark patches as requiring committing before or after other patches by using the "edit related objects" button.

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

381 ↗

(On Diff #201139)

Why are you flushing here?

[mmpozulp]

Incorporate feedback from @jhenderson to remove unneeded errs().flush().

[jhenderson]

Thanks, LGTM. Did you already commit this? If so, could you post a link to the revision that you committed? If not, so that you know, you didn't need to wait for another LGTM, since I already gave you one which you addressed.

[MaskRay]

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

1413 ↗

(On Diff #201343)

The if (!MissingDisasmFuncsSet.empty()) is redundant.

[rupprecht]

LGTM except for MaskRay's comment about the unnecessary if check

[mmpozulp]

I'm fighting with svn. See my comment in https://reviews.llvm.org/D62369.

[mmpozulp]

Incorporate feedback from @MaskRay to delete redundant if-statement.

[mmpozulp]

Committed as r362768. Thanks!

[mmpozulp]

Committed as r362838.

llvm/trunk/test/tools/llvm-objdump/X86/warn-missing-disasm-func.test

+## Warn if --disassemble-functions specifies an unknown symbol.
+# RUN: yaml2obj %s | llvm-objdump - --disassemble-functions=foo 2>&1 | FileCheck %s
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+
+# CHECK: warning: failed to disassemble missing function foo

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

 // The flags and output of this program should be near identical to those of
 // binutils objdump.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm-objdump.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SetOperations.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/CodeGen/FaultMaps.h"
 #include "llvm/DebugInfo/DWARF/DWARFContext.h"
 #include "llvm/DebugInfo/Symbolize/Symbolize.h"
 #include "llvm/Demangle/Demangle.h"
 #include "llvm/MC/MCAsmInfo.h"
LLVM_ATTRIBUTE_NORETURN void error(Twine Message) {
   exit(1);
 }
 
 void warn(StringRef Message) {
   WithColor::warning(errs(), ToolName) << Message << ".\n";
   errs().flush();
 }
 
+void warn(Twine Message) {
+  WithColor::warning(errs(), ToolName) << Message << "\n";
+}
+
 LLVM_ATTRIBUTE_NORETURN void report_error(StringRef File, Twine Message) {
   WithColor::error(errs(), ToolName)
       << "'" << File << "': " << Message << ".\n";
   exit(1);
 }
 
 LLVM_ATTRIBUTE_NORETURN void report_error(Error E, StringRef File) {
   assert(E);
for (const auto &ExportEntry : COFFObj->export_directories()) {
         AllSymbols[Sec->second].emplace_back(VA, Name, ELF::STT_NOTYPE);
       } else
         AbsoluteSymbols.emplace_back(VA, Name, ELF::STT_NOTYPE);
     }
   }
 
   // Sort all the symbols, this allows us to use a simple binary search to find
   // a symbol near an address.
+  StringSet<> FoundDisasmFuncsSet;
   for (std::pair<const SectionRef, SectionSymbolsTy> &SecSyms : AllSymbols)
     array_pod_sort(SecSyms.second.begin(), SecSyms.second.end());
   array_pod_sort(AbsoluteSymbols.begin(), AbsoluteSymbols.end());
 
   for (const SectionRef &Section : ToolSectionFilter(*Obj)) {
     if (FilterSections.empty() && !DisassembleAll &&
         (!Section.isText() || Section.isVirtual()))
       continue;
for (unsigned SI = 0, SE = Symbols.size(); SI != SE; ++SI) {
       // the list.
       if (!DisasmFuncsSet.empty() &&
           !DisasmFuncsSet.count(std::get<1>(Symbols[SI])))
         continue;
 
       uint64_t Start = std::get<0>(Symbols[SI]);
       if (Start < SectionAddr || StopAddress <= Start)
         continue;
+      else
+        FoundDisasmFuncsSet.insert(std::get<1>(Symbols[SI]));
 
       // The end is the section end, the beginning of the next symbol, or
       // --stop-address.
       uint64_t End = std::min<uint64_t>(SectionAddr + SectSize, StopAddress);
       if (SI + 1 < SE)
         End = std::min(End, std::get<0>(Symbols[SI + 1]));
       if (Start >= End || End <= StartAddress)
         continue;
#endif
             ++RelCur;
           }
         }
 
         Index += Size;
       }
     }
   }
+  StringSet<> MissingDisasmFuncsSet =
+      set_difference(DisasmFuncsSet, FoundDisasmFuncsSet);
+  for (StringRef MissingDisasmFunc : MissingDisasmFuncsSet.keys())
+    warn("failed to disassemble missing function " + MissingDisasmFunc);
 }
 
 static void disassembleObject(const ObjectFile *Obj, bool InlineRelocs) {
   if (StartAddress > StopAddress)
     error("Start address should be less than stop address");
 
   const Target *TheTarget = getTarget(Obj);