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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
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 | ||
---|---|---|
2 | This file is missing the copyright header. | |
llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
1269–1280 | Could this be added as a method to ObjectFile? |
llvm/docs/CommandGuide/llvm-objdump.rst | ||
---|---|---|
134–137 ↗ | (On Diff #460831) | 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 | ||
9 ↗ | (On Diff #460831) | 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. |
15 ↗ | (On Diff #460831) | Looks like we're missing a word here? |
llvm/tools/llvm-objdump/ObjdumpOpts.td | ||
42–44 | 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. | |
1268 | Strictly, the name should be None, to match the variable name, I'd think. | |
1269 | 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? | |
1276–1283 | It seems like some of this code isn't tested? | |
1278 | 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.
llvm/docs/CommandGuide/llvm-objdump.rst | ||
---|---|---|
134–137 ↗ | (On Diff #460831) | 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 | ||
9 ↗ | (On Diff #460831) | 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. |
llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
1278 | Changed this to output a warning in this case; added a corresponding test. |
One nitpick suggestion, otherwise LGTM.
llvm/test/tools/llvm-objdump/debuginfod.test | ||
---|---|---|
9 ↗ | (On Diff #460831) | 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. |
56 ↗ | (On Diff #463332) | 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.
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.
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. |
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 file is missing the copyright header.