diff mbox

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

Message ID 529461B1.8080200@linaro.org
State Accepted
Headers show

Commit Message

Will Newton Nov. 26, 2013, 8:54 a.m. UTC 
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?

Comments

Marcus Shawcroft Nov. 26, 2013, 10:34 a.m. UTC | #1 
On 26 November 2013 08:54, Will Newton <will.newton@linaro.org> wrote:

> -      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;
> +           }
> +         else
> +           {
> +             asection *plt;
> +
> +             if (!h->pointer_equality_needed)
> +               abort ();

Is abort() the best option here, can't we spit out a diagnostic and return?

/Marcus
Richard Earnshaw Nov. 26, 2013, 10:38 a.m. UTC | #2 
On 26/11/13 10:34, Marcus Shawcroft wrote:
> On 26 November 2013 08:54, Will Newton <will.newton@linaro.org> wrote:
> 
>> -      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;
>> +           }
>> +         else
>> +           {
>> +             asection *plt;
>> +
>> +             if (!h->pointer_equality_needed)
>> +               abort ();
> 
> Is abort() the best option here, can't we spit out a diagnostic and return?
> 
> /Marcus
> 

In general, abort is OK here if it's due to internal inconsistencies in
the linker.   It's not OK if it's due to inconsistencies in the input
object files.

R.
Will Newton Nov. 26, 2013, 10:39 a.m. UTC | #3 
On 26 November 2013 10:34, Marcus Shawcroft <marcus.shawcroft@gmail.com> wrote:
> On 26 November 2013 08:54, Will Newton <will.newton@linaro.org> wrote:
>
>> -      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;
>> +           }
>> +         else
>> +           {
>> +             asection *plt;
>> +
>> +             if (!h->pointer_equality_needed)
>> +               abort ();
>
> Is abort() the best option here, can't we spit out a diagnostic and return?

I believe it is really an assert as _bfd_elf_allocate_ifunc_dyn_relocs
ensures this condition does not occur. The code is copied from
i386/x86_64 for better or worse - I am never quite sure whether it is
better to be the same or different in these cases. ;-)
Yufeng Zhang Nov. 26, 2013, 11:06 a.m. UTC | #4 
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.

  +      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

> +		 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?


Thanks,
Yufeng


> diff --git a/ld/testsuite/ld-aarch64/aarch64-elf.exp b/ld/testsuite/ld-aarch64/aarch64-elf.exp
> index a6b3ea2..692bf34 100644
> --- a/ld/testsuite/ld-aarch64/aarch64-elf.exp
> +++ b/ld/testsuite/ld-aarch64/aarch64-elf.exp
> @@ -156,3 +156,4 @@ run_dump_test "ifunc-19a"
>   run_dump_test "ifunc-19b"
>   run_dump_test "ifunc-20"
>   run_dump_test "ifunc-21"
> +run_dump_test "ifunc-22"
> diff --git a/ld/testsuite/ld-aarch64/ifunc-22.d b/ld/testsuite/ld-aarch64/ifunc-22.d
> new file mode 100644
> index 0000000..f28b039
> --- /dev/null
> +++ b/ld/testsuite/ld-aarch64/ifunc-22.d
> @@ -0,0 +1,11 @@
> +#source: ifunc-22.s
> +#objdump: -s -j .got
> +#ld: -static
> +#target: aarch64*-*-*
> +
> +# Ensure GOT is populated correctly in static link
> +
> +.*:     file format elf64-(little|big)aarch64
> +
> +Contents of section \.got:
> + 4100f0 00000000 00000000 d0004000 00000000  ..........@.....
> diff --git a/ld/testsuite/ld-aarch64/ifunc-22.s b/ld/testsuite/ld-aarch64/ifunc-22.s
> new file mode 100644
> index 0000000..69a87bb
> --- /dev/null
> +++ b/ld/testsuite/ld-aarch64/ifunc-22.s
> @@ -0,0 +1,14 @@
> +	.text
> +	.type ifunc, @gnu_indirect_function
> +	.global ifunc
> +ifunc:
> +	ret
> +	.size	ifunc, .-ifunc
> +	.type _start, @function
> +	.globl _start
> +_start:
> +        adrp    x0, :got:ifunc
> +        ldr     x0, [x0, #:got_lo12:ifunc]
> +	.size	_start, .-_start
> +	.data
> +	.xword	ifunc
> -- 1.8.1.4
>
Will Newton Nov. 26, 2013, 11:16 a.m. UTC | #5 
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! 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.

>  +      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.
Yufeng Zhang Nov. 26, 2013, 11:51 a.m. UTC | #6 
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.
>
diff mbox

Patch

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;
+	    }
+	  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
+		 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));
diff --git a/ld/testsuite/ld-aarch64/aarch64-elf.exp b/ld/testsuite/ld-aarch64/aarch64-elf.exp
index a6b3ea2..692bf34 100644
--- a/ld/testsuite/ld-aarch64/aarch64-elf.exp
+++ b/ld/testsuite/ld-aarch64/aarch64-elf.exp
@@ -156,3 +156,4 @@  run_dump_test "ifunc-19a"
 run_dump_test "ifunc-19b"
 run_dump_test "ifunc-20"
 run_dump_test "ifunc-21"
+run_dump_test "ifunc-22"
diff --git a/ld/testsuite/ld-aarch64/ifunc-22.d b/ld/testsuite/ld-aarch64/ifunc-22.d
new file mode 100644
index 0000000..f28b039
--- /dev/null
+++ b/ld/testsuite/ld-aarch64/ifunc-22.d
@@ -0,0 +1,11 @@ 
+#source: ifunc-22.s
+#objdump: -s -j .got
+#ld: -static
+#target: aarch64*-*-*
+
+# Ensure GOT is populated correctly in static link
+
+.*:     file format elf64-(little|big)aarch64
+
+Contents of section \.got:
+ 4100f0 00000000 00000000 d0004000 00000000  ..........@.....
diff --git a/ld/testsuite/ld-aarch64/ifunc-22.s b/ld/testsuite/ld-aarch64/ifunc-22.s
new file mode 100644
index 0000000..69a87bb
--- /dev/null
+++ b/ld/testsuite/ld-aarch64/ifunc-22.s
@@ -0,0 +1,14 @@ 
+	.text
+	.type ifunc, @gnu_indirect_function
+	.global ifunc
+ifunc:
+	ret
+	.size	ifunc, .-ifunc
+	.type _start, @function
+	.globl _start
+_start:
+        adrp    x0, :got:ifunc
+        ldr     x0, [x0, #:got_lo12:ifunc]
+	.size	_start, .-_start
+	.data
+	.xword	ifunc