[PATCH] D50589: [llvm-objcopy] Implement -G/--keep-global-symbol(s).

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D50589#1199877, @rupprecht wrote:

> I basically ran --globalize-symbol a --keep-global-symbol b for a/b being local, weak, and global. (I added this as a separate test case).
>  For GNU, `b` ended up global in the output file. But `a` was kept local unless it was originally local; when `a` was local in the input, it ended up being global in the output.
>
> For reference, the GNU business logic is here: https://github.com/redox-os/binutils-gdb/blob/a9d73bb6dacfbbdc826fea178e3a47bf7a4154b1/binutils/objcopy.c#L1552
>  It seems that:
>
> - --globalize intentionally only applies to local symbols, so we're already broken w.r.t gnu compatability. (I don't know what the history of that is in llvm-objcopy -- I can fix that if you want). So running GNU objcopy --globalize Weak1 --keep-global-symbol Weak2 will result in Weak1 being local.
> - --globalize omits already global symbols (i.e. `(flags & BSF_LOCAL)`), but it only checks on the *original* binding type (i.e. it does not look at `sym->flags`). So --globalize G1 --keep-global-symbol G2 means that G1 will go from global->local (because it isn't included in --keep-global-symbol), but won't go back from local->global because the original binding was already global.
>
>   I'm not sure if the weak symbol binding is an issue; I can fix that in its own commit if you want. For the global symbol binding issue, IMHO, that's a bug, and we should do the functionality you suggested: --globalize foo should keep foo global, whether or not --keep-global-symbol covers it. OTOH, it's also an edge case that probably never comes up.


I can't remember whether we deliberately decided to deviate from GNU on the --globalize behaviour. To me, our behaviour makes sense: if a user deliberately requests to globalize a weak symbol, why shouldn't we? I could even see a use-case for it: a member, in an archive, with a weak symbol definition, won't normally be used, unless other symbols from the archive are needed, so --globalize could force its inclusion. I'd go back and dig out the change that introduced the behaviour and check if it was discussed previously, and if not, file a bug or send an email on the mailing list to discuss it further.

Overall, our goal has been to match GNU's behaviour except for where it doesn't make much sense. I'd say the second point is definitely dubious behaviour in GNU objcopy, but I'd like @jakehehrlich to confirm that he's happy with the deviation here. Otherwise, LGTM. Feel free to decide what you want to do about the CHECK patterns I mentioned inline, without needing further input from me.



================
Comment at: test/tools/llvm-objcopy/keep-global-symbols-mix-globalize.test:43
+
+# CHECK:       Symbol {
+# CHECK-NEXT:    Name:
----------------
I see what you're doing with this CHECK pattern, but I think it will false positive if we for some reason create additional symbols, although I guess I could be persuaded that this is not an issue to worry about.

I think you should probably also check the total symbol count. With that, I think this pattern becomes valid.


================
Comment at: test/tools/llvm-objcopy/keep-global-symbols.test:80
+      Section:     .text
+
+# CHECK:       Symbol {
----------------
See my earlier comment about checking the number of symbols.


Repository:
  rL LLVM

https://reviews.llvm.org/D50589