This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [BFD][PR21703]Override the new defined symbol with the old normal symbol when --allow-multiple-definition is provided
- From: "H.J. Lu" <hjl dot tools at gmail dot com>
- To: Renlin Li <renlin dot li at foss dot arm dot com>
- Cc: Alan Modra <amodra at gmail dot com>, "binutils at sourceware dot org" <binutils at sourceware dot org>
- Date: Fri, 7 Jul 2017 10:01:08 -0700
- Subject: Re: [BFD][PR21703]Override the new defined symbol with the old normal symbol when --allow-multiple-definition is provided
- Authentication-results: sourceware.org; auth=none
- References: <595BF19A.6040000@foss.arm.com> <20170706054331.GD14520@bubble.grove.modra.org> <595FB9A0.9080508@foss.arm.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.