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

[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:72
+    if (Config.DiscardMode == DiscardType::All)
+      return !static_cast<bool>(N->n_type & MachO::N_EXT);
+    return false;
----------------
MaskRay wrote:
> 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);`
yes, there is an equivalent (e.g. in cctools strip) for the option -X, -X is an alias to --discard-locals,
it's also documented here https://www.unix.com/man-page/osx/1/strip/, that option is not implemented in llvm-objcopy for MachO yet, but it's not hard to add (in a separate diff).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75104