This is an archive of the discontinued LLVM Phabricator instance.

[llvm-install-name-tool] Add -change option
ClosedPublic

Authored by sameerarora101 on Jun 25 2020, 4:52 PM.

Details

Summary

Implement -change option for install-name-tool. The behavior exactly
matches that of cctools. Depends on D82410.

Diff Detail

Event Timeline

sameerarora101 created this revision.Jun 25 2020, 4:52 PM
Herald added a project: Restricted Project. · View Herald Transcript
sameerarora101 marked 2 inline comments as done.Jun 25 2020, 5:02 PM
sameerarora101 added inline comments.
llvm/test/tools/llvm-objcopy/MachO/install-name-tool-change.test
2
NOTE: I have currently modeled -change option's behavior exactly as that of cctools. We thought it would be better to keep the same behavior in order to maintain consistency with the usage of the tools.

Rebasing (for harbormaster)

jhenderson added inline comments.Jun 26 2020, 12:38 AM
llvm/test/tools/llvm-objcopy/MachO/install-name-tool-change.test
68–69

To avoid any possible confusion, I'd recommend changing the output paths here to not match those in the input. The fact that they match suggests that's important otherwise.

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
274–296

I think it might make sense to move this logic inside the loop below - it probably doesn't make sense to loop over the load commands multiple times if you specify multiple options. The same might apply in relation the rpath stuff too.

What do you think?

sameerarora101 marked 3 inline comments as done.Jun 26 2020, 7:22 AM
sameerarora101 added inline comments.
llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
274–296

Yup, I moved the -id logic in the same loop as -change now.

However, I don't think we can do the same for rpath stuff because of all the error checking we need to perform. For eg., in case of -add_rpath, for each rpath to be added, we need to iterate through all LCs anyway so as to make sure we aren't duplicating rpaths. Similarly, in case of -delete_rpath we need to iterate through the whole list to throw an error in case we aren't able to find the rpath to be deleted.

But yeah, in case of -id and -change there is no such error checking that requires checking over all LCs, so I was able to put them in the same loop easily.

sameerarora101 marked an inline comment as done.

Putting -id and -change in the same loop

jhenderson accepted this revision.Jun 29 2020, 12:45 AM

LGTM. As mentioned inline, I think you could do a little extra refactoring to improve things further, but I'd leave that to a subsequent patch.

llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
274–296

I think it might be possible with some extra refactoring, but I'd recommend deferring that to a separate patch. What I'm thinking in semi-pseudo code terms is roughly:

std::unordered_set<StringRef> RPaths; // Pick your container of choice here. Also useful for the add rpath loop.
std::unordered_set<StirngRef> OriginalRPaths;
...
case MachO::LC_RPATH:
  StringRef Current = getLoadCommandPayloadString(LC);
  auto FoundArg = find_if(Config.RPathsToUpdate, IsFirstMatching);
  OriginalRPaths.insert(Current);
  if (!RPaths.insert(Current).second) // If already in RPaths...
    // report duplicate rpath error.
  if (FoundArg != Config.RPathsToUpdate.end())
    updateLoadCommandPayloadString<MachO::rpath_command>(Current, FoundArg->second);
...
// After loop.
for (const auto &RPair : Config.RPathsToUpdate) {
  if (OriginalRPaths.count(RPair.first) == 0)
    // Did not find rpath error.
}
296

To avoid confusion in the future, if this switch gets extended with more cases below, I'd be inclined to put a break here too.

This revision is now accepted and ready to land.Jun 29 2020, 12:45 AM
sameerarora101 marked 3 inline comments as done.Jun 29 2020, 7:16 AM
sameerarora101 added inline comments.
llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
274–296

I see now. Yup, this would be a nice change. I'll push a separate diff for this change. Thanks a lot 😊

sameerarora101 marked an inline comment as done.

rebasing over latest origin/master

sameerarora101 updated this revision to Diff 274122.EditedJun 29 2020, 7:41 AM

resolving clang-tidy

sameerarora101 marked an inline comment as done.Jun 29 2020, 3:06 PM
sameerarora101 added inline comments.
llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp
274–296

Ok, merged -rpath option inside the same loop as -id and -change here https://reviews.llvm.org/D82812. I refactored common logic too. In fact, it even simplified -add_rpath a little. Thanks a lot 😄 and please review!

This revision was automatically updated to reflect the committed changes.