[PATCH] D75104: [tools][llvm-objcopy] Enable --discard-all for MachO

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/discard-all.test:4
+# RUN: llvm-objcopy --discard-all %t %t2
+# Verify that llvm-objcopy has not modified the input.
+# RUN: cmp %t %t1
----------------
`## ` for comments.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:72
+    if (Config.DiscardMode == DiscardType::All)
+      return !static_cast<bool>(N->n_type & MachO::N_EXT);
+    return false;
----------------
alexshap wrote:
> MaskRay wrote:
> > Is this binutils's behavior? I cannot find a reference to `N_EXT`.
> I was looking at how other tools in LLVM check if a macho symbol is global
> https://llvm.org/doxygen/MachOObjectFile_8cpp_source.html#l01854 
> + checked that on a simple examples cctools's strip -x does the same as llvm-strip. 
I think it is slightly better to use

```lang=cpp
if (Config.DiscardMode == DiscardType::All && !(N->n_type & MachO::N_EXT))
  return true;
```

Can you check if `objcopy --discard-locals` works?

If there are other checks in the future, we won't have to update `return !static_cast<bool>(N->n_type & MachO::N_EXT);`


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:179
       Config.StripSections || Config.StripUnneeded ||
-      Config.DiscardMode != DiscardType::None || !Config.SymbolsToAdd.empty() ||
-      Config.EntryExpr) {
+      !Config.SymbolsToAdd.empty() || Config.EntryExpr) {
     return createStringError(llvm::errc::invalid_argument,
----------------
Since `--discard-locals` has not been implemented, `--discard-locals` should still be rejected.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75104