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

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Fri, Jul 7, 2017 at 9:41 AM, Renlin Li <renlin.li@foss.arm.com> wrote:
> 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.

An ELF testcase is missing.

-- 
H.J.