[PATCH] D89605: Add -prepend_rpath install-name-tool option

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-prepend-rpath.test:13-15
+# NEW-RPATH: cmd LC_RPATH
+# NEW-RPATH-NEXT: cmdsize
+# NEW-RPATH-NEXT: first_rpath
----------------
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.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-prepend-rpath.test:17
+
+# NEW-RPATH: cmd LC_RPATH
+# NEW-RPATH-NEXT: cmdsize
----------------



================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-prepend-rpath.test:21
+
+## Prepend with dylib loads
+# RUN: yaml2obj %p/Inputs/strip-all.yaml -o %t.dylib
----------------
Nit: comments should end with '.' (or ':' would work too)


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-prepend-rpath.test:26
+
+# DYLIB: cmd LC_RPATH
+# DYLIB-NEXT: cmdsize
----------------



================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-prepend-rpath.test:30-31
+
+# RUN: not llvm-install-name-tool -prepend_rpath first_rpath %t.i386 2>&1 \
+# RUN: | FileCheck --check-prefix=DUPLICATE-RPATH %s
+
----------------
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).


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-prepend-rpath.test:35-38
+# RUN: not llvm-install-name-tool -prepend_rpath @executable_path/. 2>&1 \
+# RUN: | FileCheck --check-prefix=NO-INPUT %s
+
+# NO-INPUT: no input file specified
----------------
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.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-prepend-rpath.test:54
+
+## Prepend and id RPATH:
+# RUN: not llvm-install-name-tool -prepend_rpath foo \
----------------
I'm not sure what is meant by "id" here? I think this option replaces it, IIRC?


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-prepend-rpath.test:66
+
+# RPATH-SIZE: cmd LC_RPATH
+# RPATH-SIZE-NEXT: cmdsize 24
----------------



================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:910-911
           RPath.str().c_str(), RPath.str().c_str());
 
+    // Cannot add and delete the same rpath at the same time.
+    if (is_contained(Config.RPathToPrepend, RPath))
----------------
I'd delete this comment and the blank line before, so that the comment for `RPathToAdd` clearly applies to both blocks.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:953
 
+    // Cannot specify the same rpath under both -add_rpath and -rpath
+    auto It4 = find_if(Config.RPathToPrepend, Match);
----------------
This comment isn't relevant to the code you've added. Please fix. It's also missing a trailing full stop.


================
Comment at: llvm/tools/llvm-objcopy/InstallNameToolOpts.td:22
+def prepend_rpath : Option<["-", "--"], "prepend_rpath", KIND_SEPARATE>,
+                    HelpText<"Prepend new rpath">;
+
----------------
I think this more clearly expresses what is happening. "Prepend" to me implied it was changing the string in the existing rpath.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:233-235
+      return createStringError(errc::invalid_argument,
+                               "rpath " + RPath +
+                                   " would create a duplicate load command");
----------------
Please test this whole error message, rather than a small part of it. In particular, showing that the `RPath` is in the message.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:242
+
+  // Unlike appending rpaths the indexes of subsequent load commands must
+  // be recalculated after prepending one.
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89605/new/

https://reviews.llvm.org/D89605