This is an archive of the discontinued LLVM Phabricator instance.

[llvm-install-name-tool] Add -id option
ClosedPublic

Authored by sameerarora101 on Jun 23 2020, 2:03 PM.

Details

Summary

Implement -id option for install-name-tool. Differences from cctool's
behavior:

  • Does NOT throw an error if multiple -id options are specified. Instead, picks the last one.
  • Throws an error in case empty id is specified.

Diff Detail

Event Timeline

sameerarora101 created this revision.Jun 23 2020, 2:03 PM
Herald added a project: Restricted Project. · View Herald Transcript
sameerarora101 edited the summary of this revision. (Show Details)Jun 23 2020, 2:03 PM
sameerarora101 edited the summary of this revision. (Show Details)
sameerarora101 marked 2 inline comments as done.Jun 23 2020, 2:09 PM
sameerarora101 added inline comments.
llvm/test/tools/llvm-objcopy/MachO/install-name-tool-id.test
11
NOTE: this behavior is different from cctool's. The cctool's version throws an error on specifying multiple -id options, but keeping @jhenderson comments in mind from -delete_rpath, we allowed for multiple -id options currently.
17
NOTE: this behavior is also different from cctool's. cctools allows for an empty id string (i.e -id ''). But we thought it would not be useful to allow for empty strings and so we throw an error right now.
jhenderson added inline comments.Jun 24 2020, 12:49 AM
llvm/test/tools/llvm-objcopy/MachO/install-name-tool-id.test
24

Am I right in thinking that you're just using the x86_64.yaml input because it doesn't have an LC_ID_DYLIB load command?

I have a personal preference to keep inputs local to their usage where possible, as it makes the test self-contained. To achieve that in this case, you can use yaml2obj's --docnum option to allow multiple YAML docs in the same test file. I'd do something like:

# RUN: yaml2obj %s --docnum=1 -o %t
<tests and checks using the first doc>

--- !mach-o
<yaml doc 1>

# RUN: yaml2obj %s --docnum=2 -o %t
<tests and checks using the second doc>

--- !mach-o
<yaml doc 2>
29–33

This is somewhat similar to the empty argument case. I'd move it to be adjacent in the test file.

llvm/tools/llvm-objcopy/CopyConfig.h
184–185

It's really that important, but I wonder if it's time to refactor out the Mach-O specific options into a MachOCopyConfig, a bit like the existing ELF config? Just something to think about, and not part of this patch.

llvm/tools/llvm-objcopy/InstallNameToolOpts.td
27

What is this KIND_SEPARATE about here and in the other options?

sameerarora101 marked 7 inline comments as done.Jun 24 2020, 7:35 AM
sameerarora101 added inline comments.
llvm/test/tools/llvm-objcopy/MachO/install-name-tool-id.test
24

really nice, thanks!

llvm/tools/llvm-objcopy/CopyConfig.h
184–185

yup, I can try creating a diff for this.

llvm/tools/llvm-objcopy/InstallNameToolOpts.td
27

KIND_SEPARATE is used to specify an option which is followed by its value. Something like option <value>. I picked this definition from here https://github.com/llvm-mirror/llvm/blob/master/include/llvm/Option/OptParser.td.

sameerarora101 marked 3 inline comments as done.Jun 24 2020, 7:36 AM

Updating test to contain the second binary.

jhenderson accepted this revision.Jun 25 2020, 12:49 AM

LGTM, with one suggestion in the test.

llvm/test/tools/llvm-objcopy/MachO/install-name-tool-id.test
69–73

Can you get rid of the LoadCommands here completely?

llvm/tools/llvm-objcopy/CopyConfig.h
184–185

Just to be clear, in case it wasn't obvious, I meant "It's NOT really that important..." - see if it's worthwhile and why we do it for the ELF case.

This revision is now accepted and ready to land.Jun 25 2020, 12:49 AM
sameerarora101 marked 2 inline comments as done.Jun 25 2020, 7:25 AM

Removing unnecessary LC from test.

Rebasing (for harbormaster)

Rebasing over latest origin/master

This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Jun 30 2020, 4:57 PM
thakis added inline comments.
llvm/test/tools/llvm-objcopy/MachO/install-name-tool-id.test
14

This breaks check-llvm for anyone who has their LLVM checkout in a directory that has "/usr" anywhere on their checkout path, for example this bot: http://45.33.8.238/linux/21750/step_12.txt

smeenai added inline comments.Jun 30 2020, 6:11 PM
llvm/test/tools/llvm-objcopy/MachO/install-name-tool-id.test
14

Thanks for the report, fixed in rG5f56da3763ac