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.
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.
|1 ↗||(On Diff #454647)|
This file is missing the copyright header.
Could this be added as a method to ObjectFile?
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?
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.
Looks like we're missing a word here?
This option doesn't seem to be documented or tested.
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.
Strictly, the name should be None, to match the variable name, I'd think.
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?
It seems like some of this code isn't tested?
Why are we throwing away the error here, rather than reporting it? Please at least add a comment explaining why.
Address review comments.
Various changes to reduce diffs of later commits in stack.
Yes, if you specify --debuginfod without curl available, it will only search the local debuginfod cached directory. This has mostly just testing utility.
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.
Changed this to output a warning in this case; added a corresponding test.
One nitpick suggestion, otherwise LGTM.
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.
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]]':
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.
IIRC, it will depend only on the stubbed-out HTTPClient unless LLVM_ENABLE_CURL is set in CMake.