This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] Add -prepend_rpath install-name-tool option
ClosedPublic

Authored by keith on Oct 16 2020, 5:10 PM.

Details

Summary

This prepends a rpath as the first rpath in the binary. This is useful
when you want a rpath to take precedence over others without having to
delete all rpaths from a binary, or change each one in order.

Author: Keith Smiley <keithbsmiley@gmail.com>

Diff Detail

Event Timeline

keith created this revision.Oct 16 2020, 5:10 PM
keith requested review of this revision.Oct 16 2020, 5:10 PM

Name suggestion: -prepend_rpath or -insert_initial_rpath. By itself, "insert" feels like it should take a position to say where it insert.

keith updated this revision to Diff 298799.Oct 16 2020, 8:54 PM

Rename to -prepend_rpath

keith retitled this revision from Add -insert_rpath install-name-tool option to Add -prepend_rpath install-name-tool option.Oct 16 2020, 8:54 PM
keith edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Oct 18 2020, 5:32 PM
llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
242

tiny nit: would be good to add a comment that unlike the case where we add new load commands at the end of Obj.LoadCommands here the indexes have been invalidated and need to be updated.

keith updated this revision to Diff 299090.Oct 19 2020, 10:12 AM

Add comment about recalculation

keith marked an inline comment as done.Oct 19 2020, 10:12 AM

Thanks! I don't have merge access can you help me land this?

According to https://llvm.org/docs/DeveloperPolicy.html#commit-messages your name + e-mail are necessary (to correctly specify the author of the changes).
I'd wait for a day to see if there are any comments from other reviewers, if everything is okay I'll commit it.

keith updated this revision to Diff 299186.Oct 19 2020, 3:07 PM

Update author

keith added a comment.Oct 19 2020, 3:08 PM

I believe my author info was correct, but I've updated it using the command mentioned in the doc. Thanks!

jhenderson requested changes to this revision.Oct 20 2020, 1:11 AM

Mostly looks fine, but lots of small issues, so requesting changes to make sure this doesn't get landed before they are addressed.

Please add the new option to the CommandGuide documentation.

llvm/test/tools/llvm-objcopy/MachO/install-name-tool-prepend-rpath.test
14–16

Nit: I find it slightly easier to follow tests which use the -NEXT pattern, if the lines without -NEXT are indented slightly to line up.

18
22

Nit: comments should end with '.' (or ':' would work too)

27
31–32

Here and below, I have a personal preference for continuations with new commands to have the '|' on the previous line and then the next line to be slightly indented, as in the suggested edit. When reading the previous line, this shows that the next line is a new command and that there are no more arguments to the current line's command, whilst the indentation shows that the next line is still a continuation.

If for some reason you'd prefer not to do that, I'd recommend at least adding a couple of spaces before the '|' for readability (I just find it easier to follow the test that way).

36–39

This isn't a test case that's needed here - the behaviour for "no input file" is not specific to the new option, and is (or should be at least) tested elsewhere.

55

I'm not sure what is meant by "id" here? I think this option replaces it, IIRC?

67
llvm/tools/llvm-objcopy/CopyConfig.cpp
915–916

I'd delete this comment and the blank line before, so that the comment for RPathToAdd clearly applies to both blocks.

951

This comment isn't relevant to the code you've added. Please fix. It's also missing a trailing full stop.

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

I think this more clearly expresses what is happening. "Prepend" to me implied it was changing the string in the existing rpath.

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
233–235

Please test this whole error message, rather than a small part of it. In particular, showing that the RPath is in the message.

242
This revision now requires changes to proceed.Oct 20 2020, 1:11 AM

I believe my author info was correct, but I've updated it using the command mentioned in the doc. Thanks!

To my knowledge, we can't view your author details from the commit you have locally. You'll either need to post them in a comment or email them directly to someone (e.g. @alexshap).

llvm/test/tools/llvm-objcopy/MachO/install-name-tool-prepend-rpath.test
36–39

@jhenderson is right, this is covered by https://github.com/llvm/llvm-project-staging/blob/staging/apple/llvm/test/tools/llvm-objcopy/tool-help-message.test (see NO-INPUT-FILES, MULTIPLE-INPUT-FILES, UNKNOWN-ARG).

keith updated this revision to Diff 299371.Oct 20 2020, 8:29 AM
keith marked 14 inline comments as done.

Address review comments

keith retitled this revision from Add -prepend_rpath install-name-tool option to [llvm-objcopy] Add -prepend_rpath install-name-tool option.Oct 20 2020, 8:30 AM
keith added a comment.Oct 20 2020, 8:30 AM

Thanks for the review!

keith edited the summary of this revision. (Show Details)Oct 20 2020, 8:31 AM
jhenderson accepted this revision.Oct 21 2020, 12:46 AM

LGTM, with one suggestion, which could be deferred to a follow-up if you want.

llvm/test/tools/llvm-objcopy/MachO/install-name-tool-prepend-rpath.test
33

Reading this message, I think we should quote the rpath value, to make it clearer. Same might apply to similar messages, but doesn't need addressing in this patch.

This revision is now accepted and ready to land.Oct 21 2020, 12:46 AM
keith added inline comments.Oct 21 2020, 9:20 AM
llvm/test/tools/llvm-objcopy/MachO/install-name-tool-prepend-rpath.test
33

I can address this in a follow up for all subcommands if that's ok

This revision was landed with ongoing or failed builds.Oct 23 2020, 3:04 PM
This revision was automatically updated to reflect the committed changes.
keith marked an inline comment as done.Oct 23 2020, 3:35 PM
keith added inline comments.
llvm/test/tools/llvm-objcopy/MachO/install-name-tool-prepend-rpath.test
33