[PATCH] D131224: [objdump] Find debug information with Build ID/debuginfod.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:134-137
+  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``.
----------------
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?


================
Comment at: llvm/test/tools/llvm-objdump/debuginfod.test:9
+# Produce a stripped copy of the input binary.
+RUN: llvm-objcopy --strip-debug %p/Inputs/embedded-source %t/stripped
+
----------------
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.


================
Comment at: llvm/test/tools/llvm-objdump/debuginfod.test:15
+
+# Use cp to write the debug to an appropriately-named file in the llvm
+# debuginfod cache.
----------------
Looks like we're missing a word here?


================
Comment at: llvm/tools/llvm-objdump/ObjdumpOpts.td:47-49
+defm debug_file_directory :
+  Eq<"debug-file-directory", "Path to directory where to look for debug files">,
+  MetaVarName<"<dir>">;
----------------
This option doesn't seem to be documented or tested.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:57
 #include "llvm/Object/Archive.h"
+#include "llvm/Object/Binary.h"
+#include "llvm/Object/BuildID.h"
----------------
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.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1271
+// Try to fetch a debug file for the given object file using its Build ID.
+// Returns none if no additional debug information was found.
+static Optional<OwningBinary<Binary>> fetchDebugFile(const ObjectFile &Obj) {
----------------
Strictly, the name should be `None`, to match the variable name, I'd think.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1272
+// Returns none if no additional debug information was found.
+static Optional<OwningBinary<Binary>> fetchDebugFile(const ObjectFile &Obj) {
+  Optional<object::BuildIDRef> BuildID = getBuildID(&Obj);
----------------
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?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1279-1286
+  Expected<OwningBinary<Binary>> DebugBinary = createBinary(*Path);
+  if (!DebugBinary) {
+    consumeError(DebugBinary.takeError());
+    return None;
+  }
+  if (ObjectFile *DbgObj = dyn_cast<ObjectFile>(DebugBinary->getBinary());
+      DbgObj->hasDebugInfo())
----------------
It seems like some of this code isn't tested?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1281
+  if (!DebugBinary) {
+    consumeError(DebugBinary.takeError());
+    return None;
----------------
Why are we throwing away the error here, rather than reporting it? Please at least add a comment explaining why.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131224/new/

https://reviews.llvm.org/D131224