[PATCH] D82812: [llvm-install-name-tool] Merge rpath with id/change

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:275
 
+  std::set<std::string> OriginalRPaths;
+  auto FindPair =
----------------
sameerarora101 wrote:
> NOTE: Cannot store `StringRef` in OriginlRPaths (i.e cannot have `set<StringRef> OriginalRPaths`). This is because the StringRef `CurrentRPath` is a reference to the string stored inside LC and thus when we change the payload (via `-rpath` option) `CurrentRPath` is also updated to hold a reference to the new string. Hence I need to store the actual strings inside the set.
You probably don't want a `std::set` here. Have you reviewed [[ https://llvm.org/docs/ProgrammersManual.html#set-like-containers-std-set-smallset-setvector-etc | the Programmer's Manual ]] on what set type to use?


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:295
 
+    case MachO::LC_RPATH: {
+      StringRef CurrentRPath = getPayloadString(LC);
----------------
Ktwu wrote:
> nit: are these braces intentional? It's inconsistent with the other cases.
I think the braces are required because of the declaration of `CurrentRPath` in this case statement.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:302
+                                                             FoundArg->second);
+    } break;
+
----------------
I think the `break` should be inside the braces. It looks weird where it is.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:340-347
+  for (StringRef RPath : Config.RPathToAdd) {
+    if (OriginalRPaths.count(RPath.str()) != 0)
       return createStringError(errc::invalid_argument,
-                               "rpath " + New +
+                               "rpath " + RPath +
                                    " would create a duplicate load command");
-
-    auto OldIt = FindRPathLC(Old);
-    if (OldIt == Obj.LoadCommands.end())
-      return createStringError(errc::invalid_argument,
-                               "no LC_RPATH load command with path: " + Old);
-
-    updateLoadCommandPayloadString<MachO::rpath_command>(*OldIt, New);
-  }
-
-  for (StringRef RPath : Config.RPathToAdd) {
-    for (LoadCommand &LC : Obj.LoadCommands) {
-      if (LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_RPATH &&
-          RPath == getPayloadString(LC)) {
-        return createStringError(errc::invalid_argument,
-                                 "rpath " + RPath +
-                                     " would create a duplicate load command");
-      }
-    }
+    OriginalRPaths.insert(RPath.str());
     Obj.addLoadCommand(buildRPathLoadCommand(RPath));
----------------
I'd be tempted to move this code above the `AddSection` code so that all rpath related stuff is together.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82812