Page MenuHomePhabricator

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

Authored by mysterymath on Aug 4 2022, 5:24 PM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2022, 5:24 PM

Documentation.

Fix a possible NPE, more comments.

Add test cases.

Make naming consistent.

mysterymath published this revision for review.Aug 22 2022, 4:21 PM
mysterymath edited the summary of this revision. (Show Details)
mysterymath added reviewers: phosek, mcgrathr.
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2022, 4:22 PM

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 marked an inline comment as done.

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.

Split out refactoring into separate change.

mysterymath edited the summary of this revision. (Show Details)

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

Fix test when CURL disabled.

mysterymath marked an inline comment as done.Thu, Sep 8, 2:01 PM
jhenderson added inline comments.Tue, Sep 20, 1:39 AM
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 marked 8 inline comments as done.

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.

Update commit message.

mysterymath retitled this revision from [objdump] Find debug information with Build ID/debuginfod. to [llvm-objdump] Find debug information with Build ID/debuginfod..Tue, Sep 27, 1:55 PM
jhenderson accepted this revision.Wed, Sep 28, 12:37 AM

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]]':

This revision is now accepted and ready to land.Wed, Sep 28, 12:37 AM
mysterymath marked an inline comment as done.

Use FileCheck -D.

MaskRay added a comment.EditedThu, Sep 29, 2:42 PM

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.

Integrate from previous.

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.

Arc diff specifically against previous commit.

Restore original patch.

Repair test on Windows.

thakis added a subscriber: thakis.Mon, Oct 3, 4:47 PM
thakis added inline comments.
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 added inline comments.Mon, Oct 3, 4:58 PM
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).