[PATCH] D82051: [llvm-install-name-tool] Add -rpath option
Sameer Arora via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 24 07:31:46 PDT 2020
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
More information about the llvm-commits
mailing list