Re: [BFD][PR21703]Override the new defined symbol with the old normal symbol when --allow-multiple-definition is provided

[Renlin Li <renlin dot li at foss dot arm dot com>]

Hi Alan,

On 06/07/17 06:43, Alan Modra wrote:
> On Tue, Jul 04, 2017 at 08:50:50PM +0100, Renlin Li wrote:
> > --- a/bfd/elflink.c
> > +++ b/bfd/elflink.c
> > @@ -1569,6 +1569,15 @@ _bfd_elf_merge_symbol (bfd *abfd,
> >         *size_change_ok = TRUE;
> >       }
> >
> > +  /* The old symbol definition is overriding the new one if the symbol is a
> > +     normal symbol.  */
> > +  if (olddef && !olddyn && !oldweak
> > +      && newdef && info->allow_multiple_definition)
> > +    {
> > +      *override = TRUE;
> > +      return TRUE;
> > +    }
> > +
> >     /* If we are looking at a dynamic object, and we have found a
> >        definition, we need to see if the symbol was already defined by
> >        some other object.  If so, we want to use the existing
>
> This doesn't look correct to me.  override is really meant to handle
> cases involving dynamic symbols.  I think skip should be set instead.
>
> It might be easier to get the condition correct, and better for
> maintenance, if you write
>
>    if (<multiple definition detected>)
>      {
>        if (!info->allow_multiple_definition)
> 	into->callbacks->multiple_definition (info, h, abfd, sec, *pvalue);
>        *skip = TRUE;
>        return TRUE;
>      }
>
> ie. handle multiple definitions completely in _bfd_elf_merge_symbol.
> I haven't written in the condition because I'll probably get it wrong
> without testing.  :)

I was hesitating to use OVERRIDE flags at that time. SKIP seems more clean.
To do multiple definition handling here is a good idea.
There are similar code in _bfd_elf_merge_symbol which handles the multiple common symbol case.

I didn't include the condition to check allow_multiple_definition in the patch as it's 
done in multiple_definition function.

The condition to check multiple definition of normal symbol is quite strict (somehow looks 
not so smart).
Both symbols definitions are neither weak nor dynamic. And the symbol version case is 
excluded as well.

I did the following check.
1, define the first symbol as weak.
2, create shared object to make the first symbol a dynamic symbol.

It all compiles Okay without using allow_multiple_definition option. And the arm version 
of foo() overrides the thumb version.
And for the original case links only when allow_multiple_definition is given, the old 
definition overrides the new definition.

ld cross and native checked Okay for arm environment. glibc builds Okay with the patched 
linker.

Any other suggestions?

Regards,
Renlin

bfd/changelog:

2017-07-07  renlin li  <renlin.li@arm.com>

    PR ld/21703
    * elflink.c (_bfd_elf_merge_symbol): Handle multiple symbol definition case.

diff --git a/bfd/elflink.c b/bfd/elflink.c
index 471e8ad..e4ab670 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -1547,6 +1547,17 @@ _bfd_elf_merge_symbol (bfd *abfd,
       sec = *psec;
     }
 
+  /* There are multiple definitions of a normal symbol.  */
+  if (olddef && !olddyn && !oldweak && newdef && !newdyn && !newweak
+      && !*matched)
+    {
+      /* Handle a multiple definition.  */
+      (*info->callbacks->multiple_definition) (info, &h->root,
+					       abfd, sec, sym->st_value);
+      *skip = TRUE;
+      return TRUE;
+    }
+
   /* If both the old and the new symbols look like common symbols in a
      dynamic object, set the size of the symbol to the larger of the
      two.  */