[llvm-objdump] Find debug information with Build ID/debuginfod.

[mysterymath]

Summary

Uses the library introduced in https://reviews.llvm.org/D132504 to add build ID fetching to llvm-objdump. This allows viewing source when disassembling stripped objects.

[mysterymath]

Documentation.

[mysterymath]

Fix a possible NPE, more comments.

[mysterymath]

Add test cases.

[mysterymath]

Make naming consistent.

[phosek]

I think that the changes to build ID/debuginfod and llvm-objdump should be split into two patches, since the former is just a refactoring while the latter is a new functionality.

llvm/lib/Object/BuildID.cpp

1 ↗

(On Diff #454647)

This file is missing the copyright header.

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

1270–1281

Could this be added as a method to ObjectFile?

[mysterymath]

Move hasDebugInfo() to ObjectFile.
Add missing copyright header.

llvm/lib/Object/BuildID.cpp

1 ↗

(On Diff #454647)

Wow whoops. Done.

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

1270–1281

Done, and this also made it obvious that there's already a reliable isDebugSection to base this off of.

[mysterymath]

Split out refactoring into separate change.

[mysterymath]

Use correct alias for --debug-file-directory argument.

[mysterymath]

Fix test when CURL disabled.

[mysterymath]

Add docs.

[jhenderson]

llvm/docs/CommandGuide/llvm-objdump.rst

134–137

Just trying to understand this: is this saying that LLVM_ENABLE_CURL and the server URLs effectively just impact the default value of this option, but that you can still specify --debuginfod without that being used?

llvm/test/tools/llvm-objdump/debuginfod.test

10

Is there a particular reason to use a canned binary instead of building from assembly with debug information (i.e. I believe llvm-mc -g)? If so, could you please include the original source file and instructions on how to compile it to create the canned input.

16

Looks like we're missing a word here?

llvm/tools/llvm-objdump/ObjdumpOpts.td

47–49

This option doesn't seem to be documented or tested.

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

57

My reading of https://llvm.org/docs/CodingStandards.html#include-as-little-as-possible (primarily the last paragraph), is that if these headers are included by one of the other headers, you don't need to #include them here. It seems unlikely to me that llvm-objdump doesn't already include this file indirectly, for example.

1269

Strictly, the name should be None, to match the variable name, I'd think.

1270

I believe a later patch in the series renames this method. Is there any particular reason not to use the final name here, to reduce git blame noise in the future?

1277–1284

It seems like some of this code isn't tested?

1279

Why are we throwing away the error here, rather than reporting it? Please at least add a comment explaining why.

[mysterymath]

Address review comments.
Various changes to reduce diffs of later commits in stack.

llvm/docs/CommandGuide/llvm-objdump.rst

134–137

Yes, if you specify --debuginfod without curl available, it will only search the local debuginfod cached directory. This has mostly just testing utility.

llvm/test/tools/llvm-objdump/debuginfod.test

10

The binaries produced by llvm-mc don't include build IDs; these are currently produced by lld. I don't know of a LLVM-only way to readably and reliably produce a build ID section.
I've added a link to the existing embedded-sources.test, which provides the information used to regenerate this file. I had to run the regeneration command as part of this change to ensure that a build ID was present, since the canned binary predates their generation by default.

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

1279

Changed this to output a warning in this case; added a corresponding test.

[mysterymath]

Update commit message.

[jhenderson]

One nitpick suggestion, otherwise LGTM.

llvm/test/tools/llvm-objdump/debuginfod.test

10

You might be able to write it as raw binary data as an llvm-mc section, but I suspect that won't work for various reasons, so sounds good. Thanks for the explanation.

57

I'd recommend using FileCheck's -D option to provide the file name explicitly for the warning message (e.g. FileCheck -DFILE=%t.err ... then warning: '[[FILE]]':

[mysterymath]

Use FileCheck -D.

[MaskRay]

The patch is not uploaded via arc diff 'HEAD^' or web interface (git format-patch -1 --stdout). I got:

% curl -L 'https://reviews.llvm.org/D131224?download=1' | patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 10725    0 10725    0     0  47865      0 --:--:-- --:--:-- --:--:-- 48094
patch unexpectedly ends in middle of line
patch: **** Only garbage was found in the patch input.
% arc patch D131224
...
 COMMITTED  Successfully committed patch.

 Cherry Pick Failed!
 Exception 
Command failed with error #1!
COMMAND
git cherry-pick -- arcpatch-D132504

STDOUT
On branch arcpatch-D131224
You are currently cherry-picking commit eae436e3098c.

[mysterymath]

Integrate from previous.

[mysterymath]

In D131224#3825238, @MaskRay wrote:

The patch is not uploaded via arc diff 'HEAD^' or web interface (git format-patch -1 --stdout). I got:

% curl -L 'https://reviews.llvm.org/D131224?download=1' | patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 10725    0 10725    0     0  47865      0 --:--:-- --:--:-- --:--:-- 48094
patch unexpectedly ends in middle of line
patch: **** Only garbage was found in the patch input.
% arc patch D131224
...
 COMMITTED  Successfully committed patch.

 Cherry Pick Failed!
 Exception 
Command failed with error #1!
COMMAND
git cherry-pick -- arcpatch-D132504

STDOUT
On branch arcpatch-D131224
You are currently cherry-picking commit eae436e3098c.

Strange; I've been using the vanilla arc diff workflow. Maybe it went out of date from the changes to the prior, and I hadn't yet reuploaded? Just sent a fresh version of the patch.

[mysterymath]

Arc diff specifically against previous commit.

[mysterymath]

Restore original patch.

[mysterymath]

Repair test on Windows.

[thakis]

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

41

Please see the discussion on D113717, in particular the summary at https://reviews.llvm.org/D113717#3295350

llvm-objdump probably shouldn't depend on HTTPClient, at least by default.

[mysterymath]

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

41

IIRC, it will depend only on the stubbed-out HTTPClient unless LLVM_ENABLE_CURL is set in CMake.
This is parallel to the configure-based optional dependency of binutils and elfutils objdump's on libdebuginfod (and transitively, libcurl).

llvm/docs/CommandGuide/llvm-objdump.rst

.. option:: --arch-name=<string>
 
   Specify the target architecture when disassembling. Use :option:`--version`
   for a list of available targets.
 
 .. option:: -C, --demangle
 
   Demangle symbol names in the output.
 
+.. option:: --debug-file-directory <path>
+
+  Provide a path to a directory with a `.build-id` subdirectory to search for
+  debug information for stripped binaries. Multiple instances of this argument
+  are searched in the order given.
+
+.. option:: --debuginfod, --no-debuginfod
+
+  Whether or not to try debuginfod lookups for debug binaries. Unless specified,
+  debuginfod is only enabled if libcurl was compiled in (``LLVM_ENABLE_CURL``)
+  and at least one server URL was provided by the environment variable
+  ``DEBUGINFOD_URLS``.
+
 .. option:: --debug-vars=<format>
 
   Print the locations (in registers or memory) of source-level variables
   alongside disassembly. ``format`` may be ``unicode`` or ``ascii``, defaulting
   to ``unicode`` if omitted.
 
 .. option:: --debug-vars-indent=<width>
 

llvm/docs/CommandGuide/llvm-symbolizer.rst

.. option:: --build-id
   string. Mutually exclusive with :option:`--obj`.
 
 .. option:: --color [=<always|auto|never>]
 
   Specify whether to use color in :option:`--filter-markup` mode. Defaults to
   ``auto``, which detects whether standard output supports color. Specifying
   ``--color`` alone is equivalent to ``--color=always``.
 
+.. option:: --debug-file-directory <path>
+
+  Provide a path to a directory with a `.build-id` subdirectory to search for
+  debug information for stripped binaries. Multiple instances of this argument
+  are searched in the order given.
+
 .. option:: --debuginfod, --no-debuginfod
 
   Whether or not to try debuginfod lookups for debug binaries. Unless specified,
   debuginfod is only enabled if libcurl was compiled in (``LLVM_ENABLE_CURL``)
   and at least one server URL was provided by the environment variable
   ``DEBUGINFOD_URLS``.
 
 .. _llvm-symbolizer-opt-C:

llvm/test/tools/llvm-objdump/debuginfod.test

+# This test uses the local debuginfod cache to test the llvm-objdump integration
+# with the debuginfod client.
+REQUIRES: x86-registered-target
+
+RUN: rm -rf %t
+RUN: mkdir %t
+
+# Produce a stripped copy of the input binary.
+# Note: See embedded-source.test for details about the input file.
+RUN: llvm-objcopy --strip-debug %p/Inputs/embedded-source %t/stripped
+
+# Printing source for the stripped binary should fail.
+RUN: env DEBUGINFOD_CACHE_PATH=%t llvm-objdump -d --source %t/stripped | \
+RUN:   FileCheck %s --check-prefix=NOTFOUND
+
+# Use cp to write the debug binary to an appropriately-named file in the llvm
+# debuginfod cache.
+RUN: cp %p/Inputs/embedded-source %t/llvmcache-7361776989772977641
+
+# Write a broken "binary" under %t/broken.
+RUN: mkdir %t/broken
+RUN: echo "bad" > %t/broken/llvmcache-7361776989772977641
+
+# Write the stripped binary under %t/stripped-cache.
+RUN: mkdir %t/stripped-cache
+RUN: cp %t/stripped %t/stripped-cache/llvmcache-7361776989772977641
+
+# Write to a debug info directory as well.
+RUN: mkdir -p %t/debug/.build-id/15
+RUN: cp %p/Inputs/embedded-source %t/debug/.build-id/15/12f769114c011387393822af15dd660c080295.debug
+
+# Don't use debuginfod by default without any URLs.
+RUN: env DEBUGINFOD_CACHE_PATH=%t llvm-objdump -d --source %t/stripped | \
+RUN:   FileCheck %s --check-prefix=NOTFOUND
+
+# Don't use debuginfod if disabled.
+RUN: env DEBUGINFOD_CACHE_PATH=%t DEBUGINFOD_URLS=http://foo \
+RUN:   llvm-objdump -d --source --no-debuginfod %t/stripped | \
+RUN:   FileCheck %s --check-prefix=NOTFOUND
+
+# Look up build IDs locally without debuginfod.
+RUN: llvm-objdump -d --source --no-debuginfod --debug-file-directory %t/debug \
+RUN:      %t/stripped | \
+RUN:   FileCheck %s --check-prefix=FOUND
+
+# Use debuginfod without URLs if requested.
+RUN: env DEBUGINFOD_CACHE_PATH=%t llvm-objdump -d --source --debuginfod \
+RUN:   %t/stripped | \
+RUN:   FileCheck %s --check-prefix=FOUND
+
+# Produce a warning if a bad binary is fetched, but do not fail.
+RUN: env DEBUGINFOD_CACHE_PATH=%t/broken llvm-objdump -d --source --debuginfod \
+RUN:   %t/stripped 2> %t.err | \
+RUN:   FileCheck %s --check-prefix=NOTFOUND
+RUN: FileCheck %s --check-prefix=BADBINARYERROR -DPATH=%t --input-file %t.err
+BADBINARYERROR: warning: '[[PATH]]/broken{{[/\\]}}llvmcache-7361776989772977641': The file was not recognized as a valid object file
+
+# Use the original binary if the fetched binary has no debug info.
+RUN: env DEBUGINFOD_CACHE_PATH=%t/stripped-cache llvm-objdump -d --source \
+RUN:   --debuginfod %t/stripped 2> %t.err | \
+RUN:   FileCheck %s --check-prefix=NOTFOUND
+RUN: count 0 < %t.err
+
+NOTFOUND-NOT: int main(int argc, char *argv[]) {
+FOUND: int main(int argc, char *argv[]) {

llvm/tools/llvm-objdump/CMakeLists.txt

add_llvm_tool(llvm-objdump
   OffloadDump.cpp
   WasmDump.cpp
   XCOFFDump.cpp
   DEPENDS
   ObjdumpOptsTableGen
   OtoolOptsTableGen
   )
 
+target_link_libraries(llvm-objdump PRIVATE LLVMDebuginfod)
+
 if(LLVM_HAVE_LIBXAR)
   target_link_libraries(llvm-objdump PRIVATE ${XAR_LIB})
 endif()
 
 add_llvm_tool_symlink(llvm-otool llvm-objdump)
 
 if(LLVM_INSTALL_BINUTILS_SYMLINKS)
   add_llvm_tool_symlink(objdump llvm-objdump)
 endif()
 
 if(LLVM_INSTALL_CCTOOLS_SYMLINKS)
   add_llvm_tool_symlink(otool llvm-otool)
 endif()

llvm/tools/llvm-objdump/ObjdumpOpts.td

 include "llvm/Option/OptParser.td"
 
+multiclass B<string name, string help1, string help2> {
+  def NAME: Flag<["--"], name>, HelpText<help1>;
+  def no_ # NAME: Flag<["--"], "no-" # name>, HelpText<help2>;
+}
+
 multiclass Eq<string name, string help> {
   def NAME : Separate<["--"], name>;
   def NAME #_eq : Joined<["--"], name #"=">,
                   Alias<!cast<Separate>(NAME)>,
                   HelpText<help>;
 }
 
 def help : Flag<["--"], "help">,
def archive_headers : Flag<["--"], "archive-headers">,
   HelpText<"Display archive header information">;
 
 def : Flag<["-"], "a">, Alias<archive_headers>,
   HelpText<"Alias for --archive-headers">;
 
 def demangle : Flag<["--"], "demangle">, HelpText<"Demangle symbol names">;
 def : Flag<["-"], "C">, Alias<demangle>, HelpText<"Alias for --demangle">;
 
+defm debug_file_directory :
+  Eq<"debug-file-directory", "Path to directory where to look for debug files">,
+  MetaVarName<"<dir>">;
+
+defm debuginfod : B<"debuginfod", "Use debuginfod to find debug files", "Don't use debuginfod to find debug files">;
+
 def disassemble : Flag<["--"], "disassemble">,
   HelpText<"Disassemble all executable sections found in the input files">;
 def : Flag<["-"], "d">, Alias<disassemble>, HelpText<"Alias for --disassemble">;
 
 def disassemble_all : Flag<["--"], "disassemble-all">,
   HelpText<"Disassemble all sections found in the input files">;
 def : Flag<["-"], "D">, Alias<disassemble_all>,
   HelpText<"Alias for --disassemble-all">;

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

 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/DebugInfo/DWARF/DWARFContext.h"
 #include "llvm/DebugInfo/Symbolize/SymbolizableModule.h"
 #include "llvm/DebugInfo/Symbolize/Symbolize.h"
+#include "llvm/Debuginfod/BuildIDFetcher.h"
+#include "llvm/Debuginfod/Debuginfod.h"
+#include "llvm/Debuginfod/HTTPClient.h"
 #include "llvm/Demangle/Demangle.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCDisassembler/MCDisassembler.h"
 #include "llvm/MC/MCDisassembler/MCRelocationInfo.h"
 #include "llvm/MC/MCInst.h"
 #include "llvm/MC/MCInstPrinter.h"
 #include "llvm/MC/MCInstrAnalysis.h"
 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/MC/MCObjectFileInfo.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/MCTargetOptions.h"
 #include "llvm/MC/TargetRegistry.h"
 #include "llvm/Object/Archive.h"
+#include "llvm/Object/BuildID.h"
 #include "llvm/Object/COFF.h"
 #include "llvm/Object/COFFImportFile.h"
 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Object/ELFTypes.h"
 #include "llvm/Object/FaultMapParser.h"
 #include "llvm/Object/MachO.h"
 #include "llvm/Object/MachOUniversal.h"
 #include "llvm/Object/ObjectFile.h"

 DebugVarsFormat objdump::DbgVariables = DVDisabled;
 
 int objdump::DbgIndent = 52;
 
 static StringSet<> DisasmSymbolSet;
 StringSet<> objdump::FoundSectionSet;
 static StringRef ToolName;
 
+std::unique_ptr<BuildIDFetcher> BIDFetcher;
+ExitOnError ExitOnErr;
+
 namespace {
 struct FilterResult {
   // True if the section should not be skipped.
   bool Keep;
 
   // True if the index counter should be incremented, even if the section should
   // be skipped. For example, sections may be skipped if they are not included
   // in the --section flag, but we still want those to count toward the section
static void createFakeELFSections(ObjectFile &Obj) {
   else if (auto *Elf32BEObj = dyn_cast<ELF32BEObjectFile>(&Obj))
     Elf32BEObj->createFakeSections();
   else if (auto *Elf64BEObj = cast<ELF64BEObjectFile>(&Obj))
     Elf64BEObj->createFakeSections();
   else
     llvm_unreachable("Unsupported binary format");
 }
 
+// Tries to fetch a more complete version of the given object file using its
+// Build ID. Returns None if nothing was found.
+static Optional<OwningBinary<Binary>>
+fetchBinaryByBuildID(const ObjectFile &Obj) {
+  Optional<object::BuildIDRef> BuildID = getBuildID(&Obj);
+  if (!BuildID)
+    return None;
+  Optional<std::string> Path = BIDFetcher->fetch(*BuildID);
+  if (!Path)
+    return None;
+  Expected<OwningBinary<Binary>> DebugBinary = createBinary(*Path);
+  if (!DebugBinary) {
+    reportWarning(toString(DebugBinary.takeError()), *Path);
+    return None;
+  }
+  return std::move(*DebugBinary);
+}
+
 static void disassembleObject(const Target *TheTarget, ObjectFile &Obj,
                               MCContext &Ctx, MCDisassembler *PrimaryDisAsm,
                               MCDisassembler *SecondaryDisAsm,
                               const MCInstrAnalysis *MIA, MCInstPrinter *IP,
                               const MCSubtargetInfo *PrimarySTI,
                               const MCSubtargetInfo *SecondarySTI,
                               PrettyPrinter &PIP, SourcePrinter &SP,
                               bool InlineRelocs) {
if (!IP)
     reportError(Obj->getFileName(),
                 "no instruction printer for target " + TripleName);
   IP->setPrintImmHex(PrintImmHex);
   IP->setPrintBranchImmAsAddress(true);
   IP->setSymbolizeOperands(SymbolizeOperands);
   IP->setMCInstrAnalysis(MIA.get());
 
   PrettyPrinter &PIP = selectPrettyPrinter(Triple(TripleName));
-  SourcePrinter SP(Obj, TheTarget->getName());
+  ObjectFile *DbgObj = Obj;
+  OwningBinary<Binary> DebugBinary;
+  if (!Obj->hasDebugInfo()) {
+    if (Optional<OwningBinary<Binary>> DebugBinaryOpt =
+            fetchBinaryByBuildID(*Obj)) {
+      if (ObjectFile *FetchedObj =
+              dyn_cast<ObjectFile>(DebugBinaryOpt->getBinary())) {
+        if (FetchedObj->hasDebugInfo()) {
+          DebugBinary = std::move(*DebugBinaryOpt);
+          DbgObj = FetchedObj;
+        }
+      }
+    }
+  }
+  SourcePrinter SP(DbgObj, TheTarget->getName());
 
   for (StringRef Opt : DisassemblerOptions)
     if (!IP->applyTargetSpecificCLOption(Opt))
       reportError(Obj->getFileName(),
                   "Unrecognized disassembler option: " + Opt);
 
   disassembleObject(TheTarget, *Obj, Ctx, DisAsm.get(), SecondaryDisAsm.get(),
                     MIA.get(), IP.get(), STI.get(), SecondarySTI.get(), PIP, SP,
if (InputArgs.hasArg(VersionFlag)) {
     cl::PrintVersionMessage();
     if (!Is("otool")) {
       outs() << '\n';
       TargetRegistry::printRegisteredTargetsForVersion(outs());
     }
     return 0;
   }
 
+  // Initialize debuginfod.
+  const bool ShouldUseDebuginfodByDefault =
+      HTTPClient::isAvailable() &&
+      !ExitOnErr(getDefaultDebuginfodUrls()).empty();
+  std::vector<std::string> DebugFileDirectories =
+      InputArgs.getAllArgValues(OBJDUMP_debug_file_directory);
+  if (InputArgs.hasFlag(OBJDUMP_debuginfod, OBJDUMP_no_debuginfod,
+                        ShouldUseDebuginfodByDefault)) {
+    HTTPClient::initialize();
+    BIDFetcher =
+        std::make_unique<DebuginfodFetcher>(std::move(DebugFileDirectories));
+  } else {
+    BIDFetcher =
+        std::make_unique<BuildIDFetcher>(std::move(DebugFileDirectories));
+  }
+
   if (Is("otool"))
     parseOtoolOptions(InputArgs);
   else
     parseObjdumpOptions(InputArgs);
 
   if (StartAddress >= StopAddress)
     reportCmdLineError("start address should be less than stop address");