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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Please don't forget to update the CommandGuide doc for llvm-strip. Is this option deliberately not an llvm-objcopy option?



================
Comment at: llvm/test/tools/llvm-objcopy/MachO/remove-swift-symbols.test:6
+
+# NO-SWIFT-SYMBOLS: Symbols [
+# NO-SWIFT-SYMBOLS-NEXT:  Symbol {
----------------
Nit: it's fairly typical to ad addditional spaces to makes the first line here line up with the rest of the output:
```
# NO-SWIFT-SYMBOLS:      Symbols [
# NO-SWIFT-SYMBOLS-NEXT:   Symbol {
```

Same applies below.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/remove-swift-symbols.test:135
+
+## Verify that -T does not remove (public) swift symbols since the binary 
+## does not contain __objc_imageinfo.
----------------
Probably change "since" -> "when"


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:608
+  if (Config.StripSwiftSymbols)
+    return createStringError(llvm::errc::invalid_argument,
+                             "option not supported by llvm-objcopy for ELF");
----------------
I think there's a `std::errc::not_supported` you could use here.


================
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;
----------------
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`.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOReader.cpp:294-297
+      if (Sec->Sectname == "__objc_imageinfo" &&
+          (Sec->Segname == "__DATA" || Sec->Segname == "__DATA_CONST" ||
+           Sec->Segname == "__DATA_DIRTY") &&
+          Sec->Content.size() >= sizeof(ObjCImageInfo)) {
----------------
Seems like most of these clauses in this `if` are untested? I'd expect to see various test inputs with an __objc_imageinfo in each of these segments, plus one with it being in a different segment and one that is too small.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOReader.cpp:299
+        memcpy(&ImageInfo, Sec->Content.data(), sizeof(ObjCImageInfo));
+        if (MachOObj.isLittleEndian() != sys::IsLittleEndianHost) {
+          sys::swapByteOrder(ImageInfo.Version);
----------------
This also seems to be untested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80099