This is an archive of the discontinued LLVM Phabricator instance.

[install-name-tool] Add --delete_all_rpaths to llvm-install-name-tool
ClosedPublic

Authored by thieta on Oct 1 2020, 9:47 AM.

Details

Summary

We had a need of deleting all the rpaths in binaries we ship (we later add just one) and instead of parsing the output of objdump -p and calling install-name-tool over and over this seemed like a easy solution to the problem. I know this tool was created to mirror upstream apple - but don't see any problem of it adding additional functionality IMHO.

This patch misses some documentation but I wanted to make sure that people thought that this was an alright thing to add.

Tagged a few reviewers that have worked with install-name-tool before.

Diff Detail

Event Timeline

thieta created this revision.Oct 1 2020, 9:47 AM
thieta requested review of this revision.Oct 1 2020, 9:47 AM

Can you also update install-name-tool-delete-rpath.test with a test case for this?

This seems fine to me. In general for our binutils-like tools, we allow extra flags for useful behavior as long as they don't have short prefixes (that may be someday used by GNU binutils, and we may want to match), which you have avoided here.

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
149

tiny tiny nit: this extra blank line seems unnecessary.

LG

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
145

nit: for a single-line if we don't use braces (just for consistency with the other places)

This revision is now accepted and ready to land.Oct 2 2020, 11:38 AM

(and yeah, we need a test for this option)

Since this is specific to the tool llvm-install-name-tool. You can change the subject to [llvm-install-name-tool] Add --delete_all_rpaths to make readers clear.

MaskRay added inline comments.Oct 2 2020, 3:31 PM
llvm/tools/llvm-objcopy/CopyConfig.h
233

--delete_all_rpaths

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

Does this need "-" ?

thieta updated this revision to Diff 295976.Oct 3 2020, 6:27 AM
thieta retitled this revision from Add --delete_all_rpaths to llvm-install-name-tool to [install-name-tool] Add --delete_all_rpaths to llvm-install-name-tool.

Fixed all review feedback and added tests.

thieta marked 4 inline comments as done.Oct 3 2020, 6:29 AM

Marked comments as done.

The test fails doesn't seem related. But let me know if anyone thinks otherwise.

llvm/test/tools/llvm-objcopy/MachO/install-name-tool-delete-rpath.test
48

the line break appears to be unnecessary

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

it would be good to have at least one test which checks -delete_all_rpaths (with single dash)

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
140–141

you can apply clang-format -style=llvm to reformat the code (here and above)

thieta updated this revision to Diff 296031.Oct 4 2020, 2:37 AM
  • made sure one call is using -- and one is using -
  • clang-format'ed
  • line breaks fix

If this is deemed done after this - can someone please commit it for me since I don't have commit access.

thieta marked 3 inline comments as done.Oct 4 2020, 2:38 AM

maked comments as done

Thanks, looks good, I'd wait for ~1 day to see if other reviewers have any comments, if everything is fine I can commit it for you

+1 to being okay to extend the tools with other useful options. Before we rush headlong into this option, I wonder whether we should instead allow the existing delete option to take a wildcard/glob pattern/regex thing. That way you could just delete e.g. "*" to remove all rpaths, or "*.foo" to delete all ending in ".foo" etc. From a compatibility concern, it might mean we need to add an option similar to e.g. --regex in llvm-objcopy which enables the pattern matching.

What do others think?

If people think this is being overly general, the proposed option in this patch is fine by me too.

thieta added a comment.Oct 6 2020, 5:48 AM

I don't have a strong opinion on what's best - both will solve my problem, if people think it would be better to use a regex I can rework the patch to do that (as long as there is a good regex implementation in LLVM I can use already).

I don't have a strong opinion regarding this either, usually there are not so many rpaths.
To me the current version looks reasonable, if you'd like I think we can proceed, but it's up to you.

+1 to being okay to extend the tools with other useful options. Before we rush headlong into this option, I wonder whether we should instead allow the existing delete option to take a wildcard/glob pattern/regex thing. That way you could just delete e.g. "*" to remove all rpaths, or "*.foo" to delete all ending in ".foo" etc. From a compatibility concern, it might mean we need to add an option similar to e.g. --regex in llvm-objcopy which enables the pattern matching.

What do others think?

If people think this is being overly general, the proposed option in this patch is fine by me too.

Same here -- that seems like it would be a better patch.

I don't have a strong opinion on what's best - both will solve my problem, if people think it would be better to use a regex I can rework the patch to do that (as long as there is a good regex implementation in LLVM I can use already).

There is -- see NameOrPattern and NameMatcher in CopyConfig.h. You would also have to add --regex and/or --wildcard flags, which AIUI are not part of install_name_tool. So that would also be a flag incompatibility, but maybe a better one.

There is some validation that all RPathsToRemove are used, so you will have to handle that differently -- it might be easier to just drop that if wildcards/regexes are used.

@rupprecht - in general, I'm not against using regex, however, as I mentioned above, it seems like it's not a very frequent use case + there are not so many rpaths (this is quite different from matching symbol names or section names).
Adding more options (--regex) + the required plumbing + tests coverage has its price, honestly I'm not convinced that it's strictly necessary in this particular case (when e.g. a binary has 2 or 3 rpaths).

just want to add a few more words to explain what I meant: on the one hand this would provide more flexibility on the other hand this would introduce one more option + increase the code size.
If this flexibility is necessary (which is not clear) - yeah, we can add it.

thieta added a comment.Oct 9 2020, 3:17 AM

Hey - I am keen on getting this done. Can we come to a consensus on what the best way is to handle this?

jhenderson accepted this revision.Oct 9 2020, 3:47 AM

I'm okay to stick with this approach. I don't think it hurts to have such an option, even if we do decide in the future to add a wildcard/regex option. @rupprecht, are you happy?

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
144

Nit: comments should end with a full-stop. You've probably also prematurely wrapped this comment.

Can this be merged? @rupprecht ?

rupprecht accepted this revision.Oct 12 2020, 1:22 PM

Sorry for the delay -- yep, going this route (for now, at least) is fine with me too.

Can you add this flag to llvm/docs/CommandGuide/llvm-install-name-tool.rst?

thieta updated this revision to Diff 297775.Oct 12 2020, 11:53 PM

Added documentation in the rst file. Anyone with commit access - feel free to commit this for me.

@thieta - thanks for the patch and for your patience. According to https://llvm.org/docs/DeveloperPolicy.html#commit-messages we would need your name + e-mail (to correctly specify the author of the changes).

Added documentation in the rst file. Anyone with commit access - feel free to commit this for me.

Also fwiw, I think you should ask for commit access.

In D88674#2326869, @alexshap wrote:

@thieta - thanks for the patch and for your patience. According to https://llvm.org/docs/DeveloperPolicy.html#commit-messages we would need your name + e-mail (to correctly specify the author of the changes).

Tobias Hieta <tobias@hieta.se>

thanks!

Added documentation in the rst file. Anyone with commit access - feel free to commit this for me.

Also fwiw, I think you should ask for commit access.

Yeah I should take care of that. Will send that email now.

Normally, I'd be happy to land patches for people, but I've got too much else on my plate currently, so if somebody else could, that would be great!

P.S. @thieta, if you get commit access before somebody gets that far, don't forget to include the line Differential Revision: https://reviews.llvm.org/D88674 in the commit message (with nothing else on that line) so that this patch auto-closes afterwards.