[PATCH] D80099: [llvm-objcopy][MachO] Add support for removing Swift symbols

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap marked an inline comment as done.
alexshap added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:76-77
+    // This behavior is consistent with cctools' strip.
+    if (Config.StripSwiftSymbols && (Obj.Header.Flags & MachO::MH_DYLDLINK) &&
+        Obj.SwiftVersion && *Obj.SwiftVersion && N->isSwiftSymbol())
+      return true;
----------------
jhenderson wrote:
> Seems like you need test cases here for a) MH_DYLDLINK not being set and b) SwiftVersion being 0.
> 
> I personally would find it easier to read if the version check were explicitly compared against 0: `*Obj.SwitftVersion != 0`.
@jhenderson :

1) If you look at the man pages for cctools' strip then you will see that there are remarks there indicating that this behavior can change in the future. Regarding tests (what you mentioned in the comment above and below) - I didn't do that intentionally since it looks like they will have to be either removed or adjusted in the future, the current implementation is consistent with the latest version of cctools.
If you insist on covering those (very similar) cases - we can add them, though I think right now it seems to be ahead of time / not very useful.

2) I'm not aware of any official objcopy (from Apple) which would work for MachO (in particular, cctools contains strip and other things, but not objcopy), so right now it looks like there is no need to introduce such flag to llvm-objcopy. If somebody requests it - sure, we can consider adding it. However, since we are trying to make llvm-strip a drop-in replacement for strip (in this particular case - I'm referring to the tool 'strip' from Xcode (cctools)) - we have to implement this option.

3) regarding the comment about big-endian - if I'm not mistaken the PowerPC backend (for Apple platforms) was removed even from Clang a few years ago, so at the moment it's a bit unrealistic (if I'm not mistaken) that we'll have a big-endian MachO executable or dynamic library built from Swift sources. However, since people can use LLVM on a big-endian host machine (and if I am not mistaken we have such buildbots) - we need to take care of those conversions. If there is an issue those big-endian buildbots are supposed to catch it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80099