[PATCH] D46217: [llvm-objcopy] Add --weaken option

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:339
+      if (Config.Weaken && Sym.Binding == STB_GLOBAL &&
+          Sym.NameIndex != STN_UNDEF)
+        Sym.Binding = STB_WEAK;
----------------
paulsemel wrote:
> paulsemel wrote:
> > jakehehrlich wrote:
> > > paulsemel wrote:
> > > > paulsemel wrote:
> > > > > jakehehrlich wrote:
> > > > > > Why is this NameIndex check here?
> > > > > This check tells me whether the symbol is defined or not. If not, it means that it's a relocation.
> > > > > In this case (as GNU objdump does to), I'm not marking the symbol.
> > > > Sorry, I meant GNU objcopy..
> > > Well at the very least this isn't the right way to handle the situation you describe. I wasn't aware of this as it doesn't seem to be documented behavior (*highly* common with GNU objcopy). I'm going to ask around on what the behavior *should* be for the intended use cases to see what should happen. It sounds like we may want other 'weaken' options to do the same thing.
> > > 
> > > 1) NameIndex is the index in the string table so it has nothing todo with weather a symbol is defined or not. It also isn't valid or meaningful until *after* finalization.
> > > 2) There isn't a good notion of "defined" in llvm-objcopy right now. There's 'defined in section X' but that dosn't cover absolute symbols and the like so there are some edge cases to check there.
> > > 3) Did we miss this behavior in the other 'weaken' options? It seems strange to me that this case would behave differently from the other weaken cases.
> > > 
> > > I've asked Roland McGrath whats supposed to be happening here but I don't suspect he'll get back to me until Monday. Sorry about that! I'll also do some black box testing of GNU objcopy to see what its doing but it isn't the case that we always want to copy it.
> > Well, you're definitely right ! I was looking for the section index, and I switched from `getShndx()` to `NameIndex` and I still can't explain why I did this..
> > But yes, it might neither respect the exact behavior of GNU objcopy, as you pointed out. I will wait for the answer of Roland McGrath then !
> > Anyway, about your third question, I am going to do some experiments to check whether I missed a behavior (which in my opinion is highly probable), and I'll come back to you !
> Alright, the behavior  is the same for the `--weaken-symbol` option. Anyway, my thoughts are that we might not need to change the behavior of this option.
> 
> Indeed, as the user explicitly want to weaken a symbol, I think we might weaken it in anyway; or at least, we might throw some sort of warnings (GNU objcopy silently does nothing..)
> 
> For this `--weaken` option, I would also go for this behavior (i.e. weaken every global symbols), as it is the behavior explained in the `objcopy` man page. Anyway, maybe we are missing something, and Roland McGrath will tell us why they made this choice of keeping undefined symbols global.
> 
> What do you think about it ?
FWIW, checking `getShndx() == SHN_UNDEF` is the correct way to test for undefined symbols currently. This deals with absolute symbols etc happily.

On a related note, what is GNU objcopy's behaviour for `--weaken` etc and global absolute and common symbols?


Repository:
  rL LLVM

https://reviews.llvm.org/D46217