[PATCH] D82051: [llvm-install-name-tool] Add -rpath option

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:120
+template <typename T>
+static void updateLoadCommandName(LoadCommand *LC, StringRef NewName) {
+  uint32_t NewCmdsize = alignTo(sizeof(T) + NewName.size() + 1, 8);
----------------
sameerarora101 wrote:
> alexshap wrote:
> > Different load commands have very different payloads, so i think this function is a bit misleading.
> > What about this:
> > ```
> > static void updateRPathLoadCommand(LoadCommand &LC, StringRef NewRPath) {
> >     assert( ....  ); // make sure that LC is indeed LC_RPATH;
> >     ...
> > }
> > ```
> So it turns out the way to update LC name follows the same logic (as above) for all the options `-rpath`, `-id`, and `-change` for `llvm-install-name-tool`. Here is how I was thinking about using it :
> 
> `-rpath` : `updateLoadCommandName<MachO::rpath_command>(&LC, NewRPath);`,
> 
> `-id` : `updateLoadCommandName<MachO::dylib_command>(&LC, NewID);`
> 
> `-change` : `updateLoadCommandName<MachO::dylib_command>(&LC, NewInstallName);`
> 
> This allowed me to refactor the common logic of updating the name payload of any load command (instead of making it specific for rpath). Would this not be ok?
alright, let's give it a try.

A few nits: 
# since in this function LC can't be null, it would be better to pass a reference (from the readability perspective)
# maybe there is a better name for this function ? it actually updates the payload, 
so I'd either use something like updateLoadCommand or updateLoadCommandPayload.
# This code requires `assert`  (that the type of the load command is one of those which you listed above)
# T should be replaced with something more specific & informative, e.g. LCType






================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:25
 
+StringRef getRPath(const LoadCommand &LC) {
+  assert(LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_RPATH &&
----------------
nit: static

P.S. but, please, take a look at my comment below (line 278), let's try to understand the broader picture.
It doesn't mean that something is wrong now but it's necessary to think about the best way to organize this code
(to improve readability and avoid code duplication in the future).


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:278
+
+    auto Match = [&Obj](StringRef RPath) {
+      return find_if(Obj.LoadCommands, [=](const LoadCommand &LC) {
----------------
I'm not sure that Match is the best possible name here (it's more like findLoadCommand, but this requires more thinking), 
but, more importantly,  it's necessary to find out how the case of dylib_command differs from rpath_command.
Based on this information one will be able to refactor this code / make an informed decision - should it be generalized or not.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82051