[PATCH] D81527: [llvm-install-name-tool] Add `delete_rpath` option
Sameer Arora via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 10 07:37:38 PDT 2020
sameerarora101 marked 5 inline comments as done.
sameerarora101 added inline comments.
================
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) {
----------------
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
================
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:
> 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.
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