[PATCH 2/2] bfd/elfnn-aarch64.c: Handle static links with ifunc correctly.

[Yufeng Zhang]

On 11/26/13 11:16, Will Newton wrote:
> On 26 November 2013 11:06, Yufeng Zhang<Yufeng.Zhang@arm.com>  wrote:
>> On 11/26/13 08:54, Will Newton wrote:
>>>
>>> The code for handling GOT references to ifunc symbols in static links
>>> was missing.
>>>
>>> bfd/ChangeLog:
>>>
>>> 2013-11-25  Will Newton<will.newton@linaro.org>
>>>
>>>          * elfnn-aarch64.c (elfNN_aarch64_finish_dynamic_symbol):
>>>          Handle STT_GNU_IFUNC symbols correctly in static links.
>>>
>>> 2013-11-25  Will Newton<will.newton@linaro.org>
>>>
>>>          * ld-aarch64/aarch64-elf.exp: Add ifunc-22.
>>>          * ld-aarch64/ifunc-22.d: New file.
>>>          * ld-aarch64/ifunc-22.s: Likewise.
>>> ---
>>>    bfd/elfnn-aarch64.c                     | 30
>>> +++++++++++++++++++++++++++++-
>>>    ld/testsuite/ld-aarch64/aarch64-elf.exp |  1 +
>>>    ld/testsuite/ld-aarch64/ifunc-22.d      | 11 +++++++++++
>>>    ld/testsuite/ld-aarch64/ifunc-22.s      | 14 ++++++++++++++
>>>    4 files changed, 55 insertions(+), 1 deletion(-)
>>>    create mode 100644 ld/testsuite/ld-aarch64/ifunc-22.d
>>>    create mode 100644 ld/testsuite/ld-aarch64/ifunc-22.s
>>>
>>> OK for trunk and binutils_2_24-branch?
>>>
>>> diff --git a/bfd/elfnn-aarch64.c b/bfd/elfnn-aarch64.c
>>> index 7cce6f4..1467f5d 100644
>>> --- a/bfd/elfnn-aarch64.c
>>> +++ b/bfd/elfnn-aarch64.c
>>> @@ -6824,7 +6824,34 @@ elfNN_aarch64_finish_dynamic_symbol (bfd
>>> *output_bfd,
>>>                         + htab->root.sgot->output_offset
>>>                         + (h->got.offset&   ~(bfd_vma) 1));
>>>
>>> -      if (info->shared&&   SYMBOL_REFERENCES_LOCAL (info, h))
>>>
>>> +      if (h->def_regular
>>> +&&   h->type == STT_GNU_IFUNC)
>>> +       {
>>> +         if (info->shared)
>>> +           {
>>> +             /* Generate R_AARCH64_GLOB_DAT.  */
>>> +             goto do_glob_dat;
>>> +           }
>>
>>
>> Can the control flow be optimized so that the outer if condition also checks
>> !info->shared?  I wonder whether the goto statement be avoided.
>
> It would involve altering the else if condition somewhat so I am
> inclined to leave as is.
>
> I am aware that the bfd copy and paste model of code reuse is rather
> unsatisfactory, but sometimes I think it is better to have the same
> bugs as everyone else rather than different ones!

Now I realized where the code is from.  Not ideal but I see your point.

> And I also I have
> dreams that one day some kindly bfd hacker will come along and pull
> some of this stuff out into common code and at that point the
> similarity between ports will make that job easier.

I have a similar dream ;)


Yufeng



>
>>   +      if (h->def_regular
>>   +&&   h->type == STT_GNU_IFUNC
>>   +&&   !info->shared )
>>
>>
>>
>>> +         else
>>> +           {
>>> +             asection *plt;
>>> +
>>> +             if (!h->pointer_equality_needed)
>>> +               abort ();
>>> +
>>> +             /* For non-shared object, we can't use .got.plt, which
>>> +                contains the real function addres if we need pointer
>>
>>
>> addres/address
>
> Thanks, fixed.
>
>>> +                equality.  We load the GOT entry with the PLT entry.  */
>>> +             plt = htab->root.splt ? htab->root.splt : htab->root.iplt;
>>> +             bfd_put_NN (output_bfd, (plt->output_section->vma
>>> +                                      + plt->output_offset
>>> +                                      + h->plt.offset),
>>> +                         htab->root.sgot->contents
>>> +                         + (h->got.offset&   ~(bfd_vma) 1));
>>>
>>> +             return TRUE;
>>> +           }
>>> +       }
>>> +      else if (info->shared&&   SYMBOL_REFERENCES_LOCAL (info, h))
>>>
>>>          {
>>>            if (!h->def_regular)
>>>              return FALSE;
>>> @@ -6838,6 +6865,7 @@ elfNN_aarch64_finish_dynamic_symbol (bfd
>>> *output_bfd,
>>>          else
>>>          {
>>>            BFD_ASSERT ((h->got.offset&   1) == 0);
>>>
>>> +do_glob_dat:
>>>            bfd_put_NN (output_bfd, (bfd_vma) 0,
>>>                        htab->root.sgot->contents + h->got.offset);
>>>            rela.r_info = ELFNN_R_INFO (h->dynindx, AARCH64_R (GLOB_DAT));
>>
>>
>> Is do_glob_dat placed deliberately after the assertion?
>
> I don't know. But I share your concern about this code, I'll move the
> label up above the assert as I don't see a case where the assert could
> fail but the code below remain valid.
>