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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

I currently view this change as padding the walls for a somewhat improper use of llvm-objcopy. Sort of like how javascript, PHP, or other such languages see a funny operation at runtime and try and pick the most sensible thing to keep your program running when there should probably be an error. In this case I actually view it as worse because this is an operation you might actually want to perform unlike adding 5 and "10 bananas" to get 15 or whatever crazy thing PHP does.

In general I'm fine with sweeping options like --localize-hidden padding the walls like this if it makes sense. I'm unsure one way or the other if it makes sense here however. I'm less ok with `--localize-symbol` padding the walls like this since the user should be allowed to do what they want when the specifier is so specific. Any use of `--localize-symbol` that causes an error like this is an error on the part of the user IMO. I'm sympathetic to needing to add this padding anyway because sometimes forking is not an option but the code you're pulling from still has the bug. So you can probably still convince me we should add this, especially to `--localize-hidden`. Also there isn't really a way to say "localize hidden symbols but ignore common symbols" right now so this fix might actually be the correct thing to do always. I just want to understand the failure mode better before adding what I currently feel is a blind workaround/foam padding for the user.

I'd also feel better if we added a --localize-hidden-common because then we cover the full use cases. Alternatively we can branch from GNU objcopy again and add `--localize-hidden-gnu` and *also* add a `-gnu` option that would make all `--*-gnu` options the semantics when using the non-gnu counter part name. I think we need such an option for `--strip-all-gnu` to be useful anyway because otherwise you can't really do a proper drop in replacement.



================
Comment at: test/tools/llvm-objcopy/localize.test:44
+    - Name:     GlobalCommon
+      Type:     STT_OBJECT
+      Index:    SHN_COMMON
----------------
Slight oddity that doesn't really matter. If Index is SHN_COMMON then the Type should always by STT_COMMON. In relocatable files if the type STT_COMMON then the index must be SHN_COMMON. In fully linked executables it needs to be something specific but the type should still be STT_COMMON. You should never see a symbol with index SHN_COMMON without STT_COMMON. 


================
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 &&
----------------
jhenderson wrote:
> 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.
This isn't the right way to check for common symbols. You should check the type not shndx.


Repository:
  rL LLVM

https://reviews.llvm.org/D53782