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

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added subscribers: compnerd, mtrent.
smeenai added a comment.

The logic looks good and matches up with cctools strip.

The man page does say:

  -T     The intent of this flag is to remove Swift symbols.  It removes the symbols whose names begin with `_$S' or `_$s' only when it finds an __objc_imageinfo section with and it has a non-zero swift version.  The future the implementation of this  flag
         may change to match the intent.

so we should keep an eye out for future behavioral changes. Also CC @mtrent if he has any input on that.



================
Comment at: llvm/test/tools/llvm-objcopy/MachO/remove-swift-symbols.test:1
+## This test is based on a trimmed down version of the binary built as follows:
+## a.swift:
----------------
Can you also add a test for Swift symbols *not* being stripped when a Swift version isn't present? It would also be great to test `_$S` symbols in addition to `_$s` ones.


================
Comment at: llvm/tools/llvm-objcopy/COFF/COFFObjcopy.cpp:255
+      Config.StripNonAlloc || Config.StripSections ||
+      Config.StripSwiftSymbols || Config.Weaken ||
       Config.DecompressDebugSections ||
----------------
This is okay for now, but note that Swift supports both COFF and ELF, so theoretically this flag could apply there as well. I don't know what the equivalent symbols would look like on COFF or ELF though ... @compnerd?


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:607
                         const Reader &Reader, ElfType OutputElfType) {
-
+  if (Config.StripSwiftSymbols)
+    return createStringError(llvm::errc::invalid_argument,
----------------
Same coment here.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:77
+    if (Config.StripSwiftSymbols &&
+        (Obj.Header.Flags & MachO::MH_DYLDLINK == MachO::MH_DYLDLINK) &&
+        Obj.SwiftVersion && *Obj.SwiftVersion && N->isSwiftSymbol())
----------------
`&` has stupidly low precedence in C/C++ (because `&&` didn't exist once upon a time), so this is gonna parse as

```
(Obj.Header.Flags & (MachO::MH_DYLDLINK == MachO::MH_DYLDLINK))
```

You'll need the parens around `(Obj.Header.Flags & MachO::MH_DYLDLINK)` instead.

Moreover, since `MH_DYLDLINK` is a single bit, can't you just do

```
(Obj.Header.Flags & MachO::MH_DYLDLINK)
```

or, if you wanna be really explicit

```
(Obj.Header.Flags & MachO::MH_DYLDLINK) != 0
```

?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80099