This is an archive of the discontinued LLVM Phabricator instance.

Add a (nonfunctional) -dyld_info flag to llvm-objdump.
ClosedPublic

Authored by aprantl on Nov 10 2021, 9:02 AM.

Details

Summary

Darwin otool implements this flag as a one-stop solution for displaying bind and rebase info. As I am working on upstreaming chained fixup support this command will be useful to write testcases.

Diff Detail

Event Timeline

aprantl created this revision.Nov 10 2021, 9:02 AM
aprantl requested review of this revision.Nov 10 2021, 9:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2021, 9:02 AM

Please remember to add new options to the .rst file for llvm-otool.

What's the motivation for adding the option without implementation at this point?

llvm/test/tools/llvm-objdump/MachO/dyld_info.test
3

Probably want to add --implicit-check-not={{.}} to catch any new/additional output too.

I have a slight personal preference for putting the | at the end of the continued line, to indicate that the next line will be the start of a new command entirely, but I'm not fussed either way.

llvm/tools/llvm-objdump/MachODump.cpp
1187–1189

I don't think we usually bother with these sorts of comments? NB: I haven't bothered looking at the context, so there may be precedent in this file for it.

What's the motivation for adding the option without implementation at this point?

This patch is part of a series of patches to upstream chained fixup support in MachOObjectFile and I need an implementation of -dyld_info to write tests for that feature. I'm trying break the patch into small pieces to make reviewing easier. With every patch the implementation will become more complete.

llvm/tools/llvm-objdump/MachODump.cpp
1187–1189

You're right, the rest of the file also doesn't use them.

aprantl updated this revision to Diff 386550.Nov 11 2021, 9:39 AM

Address review feedback.

jhenderson added inline comments.Nov 11 2021, 11:58 PM
llvm/test/tools/llvm-objdump/MachO/dyld_info.test
7–8

Consider also CHECK-NEXT, for full clarity (not needed for test explicitness, thanks to --strict-whitespace, but might make it easier to follow what is the exact output for those not looking at the FileCheck command line).

llvm/tools/llvm-objdump/MachODump.cpp
1188
llvm/tools/llvm-objdump/ObjdumpOpts.td
300–301
  1. Other Mach-O specific options state something like "(requires --macho)" in their help text.
  2. Other options don't look to have a trailing full stop.
llvm/tools/llvm-objdump/OtoolOpts.td
44

You're removing this option from the list of llvm-otool unsupported options, but as I understand it, you've added the option to llvm-objdump, but not llvm-otool. I'm not sure that's quite right?

aprantl updated this revision to Diff 410419.Feb 21 2022, 5:20 PM
aprantl marked 3 inline comments as done.

Address review feedback

keith accepted this revision.Feb 21 2022, 7:07 PM
keith added inline comments.
llvm/docs/CommandGuide/llvm-objdump.rst
305

It looks like all of the llvm-objdump flags today use dashes and not underscores, I know this matches apple's otool instead, but I wonder if it would be better to be consistent with the other ones in this version

This revision is now accepted and ready to land.Feb 21 2022, 7:07 PM
jhenderson accepted this revision.Feb 22 2022, 12:51 AM

LGTM from my point of view.

aprantl added inline comments.Feb 22 2022, 9:44 AM
llvm/docs/CommandGuide/llvm-objdump.rst
305

this matches apple's otool instead

that's the only reason for the name, yes.

MaskRay accepted this revision.Feb 22 2022, 10:27 AM
This revision was automatically updated to reflect the committed changes.