[PATCH] D81527: [llvm-install-name-tool] Add `delete_rpath` option
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 11 00:37:38 PDT 2020
jhenderson added inline comments.
================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-delete-rpath.test:4-5
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objdump -p %t | \
+# RUN: FileCheck %s --check-prefix=ALL-RPATHS
+
----------------
I could possibly be persuaded, but I'm not convinced you need this input validation check, if I'm honest. Presumably yaml2obj will have some testing that shows the output is as expected for this case, so all this is doing is showing you can write your input correctly. But even that is not necessary - your later test cases actually do this already, since llvm-install-name-tool will emit an error if an entry is missing in the input that is to be deleted, and FileCheck will emit an error if it is unexpectedly missing from the output.
================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-delete-rpath.test:13
+## Deleting single RPath entry:
+# RUN: llvm-install-name-tool -delete_rpath @executable_a/. %t
+# RUN: llvm-objdump -p %t | \
----------------
It's generally more traditional for llvm-objcopy tests to use a separate output file rather than modifying in place. This means you don't have to repeatedly recreate the same input in later tests. In some cases, we even demonstrate that the input file hasn't been modified, though I'm not convinced by the necessity of that here.
Same applies for all test cases in this test.
================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-delete-rpath.test:15
+# RUN: llvm-objdump -p %t | \
+# RUN: FileCheck %s --check-prefix=RPATHS
+
----------------
FWIW, you don't really need to indent quite so far. Just an extra two spaces on top of the previous line's indentation would be sufficient.
================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-delete-rpath.test:17
+
+# RPATHS-NOT: path @executable_a/.
+
----------------
You should validate that the other paths are still present.
================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-delete-rpath.test:18-19
+# RPATHS-NOT: path @executable_a/.
+
+
+## Deleting multiple RPath entries:
----------------
I'm not going to push on this, but I don't think the double blank lines really help improve readability of the test.
================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-delete-rpath.test:26-28
+# RPATHS-MULTIPLE-NOT: path @executable_a/.
+# RPATHS-MULTIPLE-NOT: path @executable_b/.
+# RPATHS-MULTIPLE-NOT: path @executable_c/.
----------------
It's worth remembering that FileCheck's CHECK-NOT patterns only check the region between the previous and next non-NOT matches (including the start/end of file as appropriate). This style of checking would therefore not prevent llvm-install-name-tool from reordering them - executable_d could appear before executable_a, executable_b and executable_c and this test would pass.
A better approach would be to use FileCheck's --implicit-check-not. This is equivalent to specifying CHECK-NOT in between every positive match. Anything that contains the pattern specified in implicit-check-not which is not explicitly matched by another check will cause the test to fail.
================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-delete-rpath.test:32
+
+## Deleting a non-existing RPath:
+# RUN: not llvm-install-name-tool -delete_rpath @executable_a/. %t 2>&1 | \
----------------
non-existing -> nonexistent
(according to my brief StackExchange search...)
================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-delete-rpath.test:47
+
+## Adding and Deleting RPATH at the same time:
+# RUN: not llvm-install-name-tool -add_rpath @executable_b/. \
----------------
Deleting -> deleting
================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.cpp:833-836
+ if (llvm::is_contained(Config.RPathsToRemove, RPath))
+ return createStringError(errc::invalid_argument,
+ "-delete_rpath %s specified more than once",
+ RPath.str().c_str());
----------------
smeenai wrote:
> sameerarora101 wrote:
> > jhenderson wrote:
> > > This seems like an unnecessary limitation to me. I'd just expect it to ignore the later ones (you could use a set rather than a vector to store the set to remove in to ensure that).
> > So currently, I have modeled the behavior according to Apples' `install_name_tool` and it throws this error in case the user specifies a `delete_rpath` entry twice. I can change it though if this is not we want?
> Yeah. There's some tension here between following standard objcopy conventions vs. emulating the behavior of Apple's install_name_tool (which this is an LLVM version of). In general, we thought it was best to match install_name_tool's behavior as much as possible, although I don't feel too strongly about this particular error case.
Not being an Apple developer, I don't feel like I'm best placed to say what the behaviour should be. I'd prefer @alexshap or other Apple developers to give their thoughts. How important is it to emulate error message behaviour? Are the errors actually useful?
One of the issues with prohibiting something being specified more than once is that people will occasionally be using response files or a build system or whatever where they can't easily change the existing commands that are specified, but can add to the end (I've run into this on one or two occasions). By following the more traditional approach of "last one wins", it's possible to workaround this difficulty, whereas this error makes it impossible.
My 2 pence are that this one is not particularly useful, whereas the other one I commented on might have some value for reasons you've already outlined.
================
Comment at: llvm/tools/llvm-objcopy/InstallNameToolOpts.td:22
+def delete_rpath: Option<["-", "--"], "delete_rpath", KIND_SEPARATE>,
+ HelpText<"Delete old rpath">;
+
----------------
jhenderson wrote:
> I'd get rid of "old" from the text. We don't say "old" for e.g. --remove-section in objcopy.
Actually, perhaps it should be "Delete specified rpath", since you have to explicitly specify the path that is being deleted.
================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:26-45
+ LoadCommandPred RemovePred = [](const LoadCommand &) { return false; };
+ DenseSet<StringRef> RPathsToRemove;
+
+ if (!Config.RPathsToRemove.empty()) {
+ RPathsToRemove.insert(Config.RPathsToRemove.begin(),
+ Config.RPathsToRemove.end());
+ RemovePred = [&RPathsToRemove](const LoadCommand &LC) {
----------------
sameerarora101 wrote:
> sameerarora101 wrote:
> > sameerarora101 wrote:
> > > alexshap wrote:
> > > > jhenderson wrote:
> > > > > This whole block seems unnecessarily overcomplicated to me.
> > > > >
> > > > > 1) Make Config.RPathsToRemove a set and then you don't have to copy things from one set to another - you can just use the config variable directly.
> > > > > 2) There's no need for the `if (!Config.RPathsToRemove.empty())` check, since it is safe to do everything here whether it's empty or not (the insert disappears, but would be safe, and defining the lambda is harmless and trivial.
> > > > > 3) By getting rid of the "if empty" check, you can also get rid of the `return false` lambda.
> > > > > 4) `trim(0)` - I assume you mean `trim('\0')`? If so, use the char directly rather than an integer literal, since you are dealing with an array of chars. This makes it easier to follow. I am assuming here that `LC.Payload.size()` includes a trailing null byte you want to get rid of. Do you also mean to get rid of any leading null bytes too? If not, you might be better off using `rtrim`.
> > > > After some thinking it seems to me that having the original list of specified rpaths can be useful since it enables us to report the **first** option causing the issue. If we drop that error reporting then indeed it makes sense to simply use StringSet for storing RPATHs in CopyConfig and use that set directly.
> > > >
> > > > 2., 3. - I also think that !Config.RPathsToRemove.empty() is unnecessary
> > > >
> > > > If I am not mistaken there is a tiny bug in buildRPathLoadCommand function (I've just spotted it) - would be good to fix separately.
> > > >
> > > > RPathLC.cmdsize = alignTo(sizeof(MachO::rpath_command) + Path.size(), 8);
> > > >
> > > > should be replaced with
> > > >
> > > > RPathLC.cmdsize = alignTo(sizeof(MachO::rpath_command) + Path.size() + 1, 8);
> > > >
> > > > this is what LD64 does (see ld64/src/ld/HeaderAndLoadCommands.hpp) :
> > > >
> > > > template <typename A>
> > > > uint8_t* HeaderAndLoadCommandsAtom<A>::copyRPathLoadCommand(uint8_t* p, const char* path) const
> > > > {
> > > > uint32_t sz = alignedSize(sizeof(macho_rpath_command<P>) + strlen(path) + 1);
> > > > macho_rpath_command<P>* cmd = (macho_rpath_command<P>*)p;
> > > > cmd->set_cmd(LC_RPATH);
> > > > cmd->set_cmdsize(sz);
> > > > cmd->set_path_offset();
> > > > strcpy((char*)&p[sizeof(macho_rpath_command<P>)], path);
> > > > return p + sz;
> > > > }
> > > >
> > > >
> > > >
> > > >
> > > >
> > > 4 - for using `trim(0)`, I was looking at ` RPath == StringRef(reinterpret_cast<char *>(LC.Payload.data()),LC.Payload.size().trim(0))` at line 264 under `Config.RPathToAdd`. Is that not how I should be doing it? Thanks
> > @alexshap - ya, I can push a fix for it separately.
> pushed the fix at https://reviews.llvm.org/D81575
I assume something got messed up when you copied in that line, since you obviously don't trim a size value!
I would suggest that the older code should be improved (not in this patch though). When I read this code originally, I thought it was trying to trim 0 bytes or something before I confirmed that there is no such interface.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81527/new/
https://reviews.llvm.org/D81527
More information about the llvm-commits
mailing list