[PATCH] D53782: [llvm-objcopy] Don't apply --localize flags to common symbols

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I can certainly understand your reasoning, but this could also adversely impact a desired use-case of hiding a symbol in an object or library from another object, which is presumably the purpose of --localize-hidden, and is what is discussed in https://chromium.googlesource.com/external/dynamorio/+/master/core/CMakeLists.txt#541:

> We need to do extra work to hide symbols in the static library build.
>  First we do a partial link with ld -r, which makes a single libdynamorio.o
>  object file.  Then we use objcopy --localize-hidden to hide all
>  non-exported symbols.

Let's say that there's a common symbol `foo` which is defined in test.o, and is added to a static library to be shipped by a vendor. They might want this symbol to not appear in their interface. Using --localize-hidden would be one way of doing this, but only if the existing llvm-objcopy behaviour is maintained.

Basically, I'm on the fence on this, but I wanted to clarify the argument against it from my point of view.



================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:259
     Obj.SymbolTable->updateSymbols([&](Symbol &Sym) {
-      if ((Config.LocalizeHidden &&
-           (Sym.Visibility == STV_HIDDEN || Sym.Visibility == STV_INTERNAL)) ||
-          (!Config.SymbolsToLocalize.empty() &&
-           is_contained(Config.SymbolsToLocalize, Sym.Name)))
+      if (Sym.getShndx() != SHN_COMMON &&
+          ((Config.LocalizeHidden &&
----------------
MaskRay wrote:
> MaskRay wrote:
> > GNU objcopy -L does not apply on `SHN_UNDEF` symbols.
>       if (Sym.getShndx() != SHN_UNDEF && Sym.getShndx() != SHN_COMMON &&
>           (Sym.Binding == STB_GLOBAL || Sym.Binding == STB_WEAK) &&
>           (is_contained(Config.SymbolsToLocalize, Sym.Name) ||
>            (Config.LocalizeHidden &&
>             (Sym.Visibility == STV_HIDDEN || Sym.Visibility == STV_INTERNAL))))
> 
> 
> May be a better emulation of GNU objcopy's behavior. But I don't know how much compatibility we want to achieve...
Reasonably observations these, but I think that they're a separate issue, and should not be done as part of this patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D53782