Re: [gold patch] PR 16504: Fix internal error caused by conflicting default version definitions

["Ian Lance Taylor via binutils" <binutils at sourceware dot org>]

On Mon, Apr 23, 2018 at 3:25 PM,  <ccoutant@gmail.com> wrote:
>
> This patch fixes a couple of problems with symbol versioning in gold,
> but I'd like to get an extra pair eyes on it before I check it in,
> since the expected behavior in some of these cases is (to my mind)
> quite subtle.
>
> The problem that affects this PR directly is when we have a
> default version definition (set by a version script) for a
> symbol, foo@@V2, in an LTO object, along with an external reference
> to a different version, foo@V1, of that same symbol. We then link
> that object against a shared library that provides a default
> definition of foo@@V1. When we see the definition of foo@@V1 in the
> shared library, we already have foo@@V2 in the symbol table, and we
> resolve (incorrectly) foo@@V1 to foo@@V2. We don't actually see the
> external reference to foo@V2 until the replacement file is provided,
> and then try to resolve it to foo@@V1. At that point, we hit an
> internal error in Symbol::override_version() because of
> the conflicting version name.
>
> I've fixed this by making sure that we enter foo@@V2 from the
> shared library as a new (but not default) symbol, so that when
> we later see the external reference to foo@V2, it resolves to
> that symbol instead of foo@@V1. I only do this if the new symbol
> is coming from a shared library, and if the original symbol
> already has a (non-empty) version. If we see a conflicting
> default version from a regular object, we give a warning --
> or should it be an error? -- Gnu ld simply prints a multiple
> definition error for that case.
>
> During my investigation of this bug, I also found that gold is
> setting the VERSYM_HIDDEN bit in cases where Gnu ld is not. I've
> changed gold so that it sets the HIDDEN bit only for definitions,
> and not for UNDEFs.
>
> The test case provided with the PR leaves me with a nervous
> feeling that neither IFUNCs nor .symver directives are a good
> idea when using LTO. For .symver at least, the compiler doesn't
> see the real symbol name (it's just an asm pass-thru), so it
> could be missing some important linkage information. It could
> theoretically figure out how the IFUNCs work by looking at the
> resolve function, but I'm not sure it does.

I'm not going to claim that I've thought of all the implications, but
everything you write here looks entirely reasonable to me, and the
patch looks good.  Thanks.

Ian