diff mbox

[RFC] bfd/elf32-arm.c: Set st_value to zero for undefined symbols.

Message ID 1389868125-7349-1-git-send-email-will.newton@linaro.org
State Accepted
Headers show

Commit Message

Will Newton Jan. 16, 2014, 10:28 a.m. UTC
I'm not sure what the implications of this patch are, so I thought I would
post it here in the hope that someone can explain what might break.

The issue comes from a report that Qt does not work correctly on ARM when
built with -Bsymbolic:

http://lists.linaro.org/pipermail/linaro-toolchain/2014-January/003942.html

Unless point_equality_needed is set then set st_value to be zero
for undefined symbols.

bfd/ChangeLog:

2014-01-10  Will Newton  <will.newton@linaro.org>

	* elf32-arm.c (elf32_arm_check_relocs): Set
	pointer_equality_needed for absolute references within
	executable links.
	(elf32_arm_finish_dynamic_symbol): Set st_value to zero
	unless pointer_equality_needed is set.

ld/testsuite/ChangeLog:

2014-01-10  Will Newton  <will.newton@linaro.org>

	* ld-arm/ifunc-14.rd: Update symbol values.
---
 bfd/elf32-arm.c                 | 7 ++++++-
 ld/testsuite/ld-arm/ifunc-14.rd | 4 ++--
 2 files changed, 8 insertions(+), 3 deletions(-)

Comments

Will Newton Jan. 23, 2014, 3:22 p.m. UTC | #1
On 16 January 2014 10:28, Will Newton <will.newton@linaro.org> wrote:
> I'm not sure what the implications of this patch are, so I thought I would
> post it here in the hope that someone can explain what might break.
>
> The issue comes from a report that Qt does not work correctly on ARM when
> built with -Bsymbolic:
>
> http://lists.linaro.org/pipermail/linaro-toolchain/2014-January/003942.html
>
> Unless point_equality_needed is set then set st_value to be zero
> for undefined symbols.
>
> bfd/ChangeLog:
>
> 2014-01-10  Will Newton  <will.newton@linaro.org>
>
>         * elf32-arm.c (elf32_arm_check_relocs): Set
>         pointer_equality_needed for absolute references within
>         executable links.
>         (elf32_arm_finish_dynamic_symbol): Set st_value to zero
>         unless pointer_equality_needed is set.
>
> ld/testsuite/ChangeLog:
>
> 2014-01-10  Will Newton  <will.newton@linaro.org>
>
>         * ld-arm/ifunc-14.rd: Update symbol values.
> ---
>  bfd/elf32-arm.c                 | 7 ++++++-
>  ld/testsuite/ld-arm/ifunc-14.rd | 4 ++--
>  2 files changed, 8 insertions(+), 3 deletions(-)

Ping?
Will Newton Jan. 24, 2014, 10:11 a.m. UTC | #2
On 24 January 2014 04:17, Alan Modra <amodra@gmail.com> wrote:
> On Thu, Jan 23, 2014 at 03:22:21PM +0000, Will Newton wrote:
>> On 16 January 2014 10:28, Will Newton <will.newton@linaro.org> wrote:
>> > I'm not sure what the implications of this patch are, so I thought I would
>> > post it here in the hope that someone can explain what might break.
>
> You're probably being a bit too honest here. :)
>
> The patch looks OK to me as far as it goes, but I count myself
> unqualified to ack it.  I don't know enough about ARM to say whether
> you've set pointer_equality_needed for all the relocs types you
> should.  It needs to be set whenever you might be taking the address
> of a function in non-PIC code or data.

Thanks, I wasn't sure of the semantics of pointer_equality_needed.
I'll investigate and post an updated patch.
Will Newton Jan. 27, 2014, noon UTC | #3
On 24 January 2014 10:11, Will Newton <will.newton@linaro.org> wrote:
> On 24 January 2014 04:17, Alan Modra <amodra@gmail.com> wrote:
>> On Thu, Jan 23, 2014 at 03:22:21PM +0000, Will Newton wrote:
>>> On 16 January 2014 10:28, Will Newton <will.newton@linaro.org> wrote:
>>> > I'm not sure what the implications of this patch are, so I thought I would
>>> > post it here in the hope that someone can explain what might break.
>>
>> You're probably being a bit too honest here. :)
>>
>> The patch looks OK to me as far as it goes, but I count myself
>> unqualified to ack it.  I don't know enough about ARM to say whether
>> you've set pointer_equality_needed for all the relocs types you
>> should.  It needs to be set whenever you might be taking the address
>> of a function in non-PIC code or data.
>
> Thanks, I wasn't sure of the semantics of pointer_equality_needed.
> I'll investigate and post an updated patch.

After investigating the relocs involved I believe the original patch
is correct from that point of view. Richard, have you got any opinion
on the patch?

Thanks,
diff mbox

Patch

diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
index 7216244..a999b64 100644
--- a/bfd/elf32-arm.c
+++ b/bfd/elf32-arm.c
@@ -12681,6 +12681,11 @@  elf32_arm_check_relocs (bfd *abfd, struct bfd_link_info *info,
 	    /* Fall through.  */
 	  case R_ARM_ABS32:
 	  case R_ARM_ABS32_NOI:
+	    if (h != NULL && info->executable)
+	      {
+		h->pointer_equality_needed = 1;
+	      }
+	    /* Fall through.  */
 	  case R_ARM_REL32:
 	  case R_ARM_REL32_NOI:
 	  case R_ARM_MOVW_PREL_NC:
@@ -14046,7 +14051,7 @@  elf32_arm_finish_dynamic_symbol (bfd * output_bfd,
 	     Otherwise, the PLT entry would provide a definition for
 	     the symbol even if the symbol wasn't defined anywhere,
 	     and so the symbol would never be NULL.  */
-	  if (!h->ref_regular_nonweak)
+	  if (!h->ref_regular_nonweak || !h->pointer_equality_needed)
 	    sym->st_value = 0;
 	}
       else if (eh->is_iplt && eh->plt.noncall_refcount != 0)
diff --git a/ld/testsuite/ld-arm/ifunc-14.rd b/ld/testsuite/ld-arm/ifunc-14.rd
index 59ea29b..9c44092 100644
--- a/ld/testsuite/ld-arm/ifunc-14.rd
+++ b/ld/testsuite/ld-arm/ifunc-14.rd
@@ -8,5 +8,5 @@  Relocation section '\.rel\.dyn' at offset 0x8000 contains 4 entries:
 
 Relocation section '\.rel\.plt' at offset 0x8020 contains 2 entries:
  Offset     Info    Type            Sym\.Value  Sym\. Name
-0001100c  ......16 R_ARM_JUMP_SLOT   00009014   f2t
-00011010  ......16 R_ARM_JUMP_SLOT   00009020   f2
+0001100c  ......16 R_ARM_JUMP_SLOT   00000000   f2t
+00011010  ......16 R_ARM_JUMP_SLOT   00000000   f2