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

[Sameer Arora via Phabricator via llvm-commits]

sameerarora101 marked 4 inline comments as done.
sameerarora101 added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:25
 
+static StringRef getLoadCommandPayload(const LoadCommand &LC) {
+  assert((LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_RPATH ||
----------------
jhenderson wrote:
> sameerarora101 wrote:
> > I haven't added an assert for LC_REEXPORT_DYLIB , LC_LOAD_UPWARD_DYLIB or LC_LAZY_LOAD_DYLIB as llvm-objcopy doesn't support them yet. I can add a TODO comment here if needed?
> TODO comments for missing functionality is usually reasonable, though I wonder whether we should just have an unsupported error somewhere for such cases.
so there is currently an unsupported error thrown by `MachO/MachOLayoutBuilder.cpp` for such cases:

```
...
 case MachO::LC_SOURCE_VERSION:
      // Nothing to update.
      break;
    default:
      // Abort if it's unsupported in order to prevent corrupting the object.
      return createStringError(llvm::errc::not_supported,
                               "unsupported load command (cmd=0x%x)", cmd);
    }
...
```
But yeah I can add a TODO above as well. Thanks!



================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:134-142
+  assert((LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_RPATH ||
+          LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_ID_DYLIB ||
+          LC.MachOLoadCommand.load_command_data.cmd == MachO::LC_LOAD_DYLIB ||
+          LC.MachOLoadCommand.load_command_data.cmd ==
+              MachO::LC_LOAD_WEAK_DYLIB) &&
+         "LC_RPATH or \
+          LC_ID_DYLIB or \
----------------
jhenderson wrote:
> Since this assert is common between the two functions, it might be worth pulling it into a simple helper. Something like "isLoadCommandWithPayload" and just call it instead:
> 
> ```
> assert(isLoadCommandWithPayload(LC && "unsupported load command encountered");
> ```
nice, thanks 😊 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82051