[RFA:] Fix ld/13990, ARM BFD_ASSERT with --shared and --gc-sections

[RFA:] Fix ld/13990, ARM BFD_ASSERT with --shared and --gc-sections

[Hans-Peter Nilsson]

This bug will strike for e.g. arm-linux-gnueabi when:

- There's a reference (call-reloc) to a symbol undefined at first
reference, preliminary requiring a PLT.

- The symbols' definition is found in the linked objects and is
hidden (or forced local).

- The referencing section (containing the call-reloc) is GC'd
(its global symbols are forced local by the version script and
found to be unused).

Then, the gc_sweep function tries to balance the books by -= 1
on the now-GC'd-away PLT (with refcount set to -1 from the
already forced-local symbol) which is guarded by a "BFD_ASSERT
(root_plt->refcount > 0)".  So the assert strikes.

But, the non-positive PLT refcount is supposed to be exactly
that -1 (and there's no fallout later on) and such negative
counts are handled elsewhere in elf32-arm.c.  The special PLT
and GOT accounting in elf32-arm.c doesn't allocate anything and
looking around in that file I found no other reason to add the
usual target-specific elf_backend_hide_symbol to handle the
hiding specially; just checking for a negative count seems
sufficient.

The assert is there to catch book-keeping errors, but as those
counts are balanced one item at a time, it's expected to see the
value 0 before seeing -1 on such errors: better add such an
BFD_ASSERT to not lose any error sentinels.  Also adding one on
other negative values for good measure.

So *this time* the BFD_ASSERT and ignoring it was benevolent,
and the output probably still works; I haven't tested besides
checking that the exact same file is output.

Tested arm-eabi, arm-linux-gnueabi, arm-unknown-nacl (to see
that the test is skipped; it pretty much had to be, because the
addresses are all different) and a bunch of other arm-targets
but which return early in arm-elf.exp and thus uninteresting.

Ok to commit?

ld/testsuite:

	PR ld/13990
	* ld-arm/arm-elf.exp: Run gc-hidden-1.
	* gc-hidden-1.d: New test-file.
	* gcdfn.s, hideall.ld, hidfn.s, main.s: New files.

bfd:
	PR ld/13990
	* elf32-arm.c (elf32_arm_gc_sweep_hook): Handle a forced-local
	symbol, where PLT refcount is set to -1.

diff --git a/ld/testsuite/ld-arm/arm-elf.exp b/ld/testsuite/ld-arm/arm-elf.exp
index 05dc7bb..b6eb3d3 100644
--- a/ld/testsuite/ld-arm/arm-elf.exp
+++ b/ld/testsuite/ld-arm/arm-elf.exp
@@ -759,3 +759,4 @@ run_dump_test "attr-merge-vfp-6r"
 run_dump_test "attr-merge-incompatible"
 run_dump_test "unresolved-1"
 run_dump_test "unresolved-1-dyn"
+run_dump_test "gc-hidden-1"
diff --git a/ld/testsuite/ld-arm/gc-hidden-1.d b/ld/testsuite/ld-arm/gc-hidden-1.d
new file mode 100644
index 0000000..80c7e9e
--- /dev/null
+++ b/ld/testsuite/ld-arm/gc-hidden-1.d
@@ -0,0 +1,25 @@
+#target: arm*-*-*eabi
+#source: main.s
+#source: gcdfn.s
+#source: hidfn.s
+#ld: --gc-sections --shared --version-script hideall.ld
+#objdump: -dRT
+
+# See PR ld/13990: a forced-local PLT reference to a
+# forced-local symbol is GC'ed, trigging a BFD_ASSERT.
+
+.*:     file format elf32-.*
+
+DYNAMIC SYMBOL TABLE:
+0+124 l    d  .text	0+              .text
+0+ g    DO \*ABS\*	0+  NS          NS
+
+Disassembly of section .text:
+
+0+124 <_start>:
+ 124:	e52de004 	push	{lr}		; \(str lr, \[sp, #-4\]!\)
+ 128:	eb000000 	bl	130 <hidfn>
+ 12c:	e8bd8000 	pop	{pc}
+
+0+130 <hidfn>:
+ 130:	e8bd8000 	pop	{pc}
diff --git a/ld/testsuite/ld-arm/gcdfn.s b/ld/testsuite/ld-arm/gcdfn.s
new file mode 100644
index 0000000..f2afae7
--- /dev/null
+++ b/ld/testsuite/ld-arm/gcdfn.s
@@ -0,0 +1,8 @@
+	.text
+	.globl gcdfn
+	.type gcdfn, %function
+gcdfn:
+	str	lr, [sp, #-4]!
+	bl	hidfn(PLT)
+	ldmfd	sp!, {pc}
+	.size gcdfn, . - gcdfn
diff --git a/ld/testsuite/ld-arm/hideall.ld b/ld/testsuite/ld-arm/hideall.ld
new file mode 100644
index 0000000..077d6b5
--- /dev/null
+++ b/ld/testsuite/ld-arm/hideall.ld
@@ -0,0 +1 @@
+NS { local: *; };
diff --git a/ld/testsuite/ld-arm/hidfn.s b/ld/testsuite/ld-arm/hidfn.s
new file mode 100644
index 0000000..a66b558
--- /dev/null
+++ b/ld/testsuite/ld-arm/hidfn.s
@@ -0,0 +1,7 @@
+	.text
+	.globl hidfn
+	.hidden	hidfn
+	.type hidfn, %function
+hidfn:
+	ldmfd	sp!, {pc}
+	.size hidfn, . - hidfn
diff --git a/ld/testsuite/ld-arm/main.s b/ld/testsuite/ld-arm/main.s
new file mode 100644
index 0000000..046d19d
--- /dev/null
+++ b/ld/testsuite/ld-arm/main.s
@@ -0,0 +1,8 @@
+	.text
+	.globl _start
+	.type _start, %function
+_start:
+	str	lr, [sp, #-4]!
+	bl	hidfn(PLT)
+	ldmfd	sp!, {pc}
+	.size _start, . - _start

diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
index f5b5c4d..5907974 100644
--- a/bfd/elf32-arm.c
+++ b/bfd/elf32-arm.c
@@ -12256,18 +12256,34 @@ elf32_arm_gc_sweep_hook (bfd *                     abfd,
       if (may_need_local_target_p
 	  && elf32_arm_get_plt_info (abfd, eh, r_symndx, &root_plt, &arm_plt))
 	{
-	  BFD_ASSERT (root_plt->refcount > 0);
-	  root_plt->refcount -= 1;
+	  /* If PLT refcount book-keeping is wrong and too low, we'll
+	     see a zero value (going to -1) for the root PLT reference
+	     count.  */
+	  if (root_plt->refcount >= 0)
+	    {
+	      BFD_ASSERT (root_plt->refcount != 0);
+	      root_plt->refcount -= 1;
+
+	      /* No use incrementing these members as all their uses are
+		 dominated by tests on the PLT refcount being
+		 non-negative.  */
+	      if (r_type == R_ARM_THM_CALL)
+		arm_plt->maybe_thumb_refcount--;
+
+	      if (r_type == R_ARM_THM_JUMP24
+		  || r_type == R_ARM_THM_JUMP19)
+		arm_plt->thumb_refcount--;
+	    }
+	  else
+	    /* A value of -1 means the symbol has become local, forced
+	       or seeing a hidden definition.  Any other negative value
+	       is an error.  */
+	    BFD_ASSERT (root_plt->refcount == -1);
 
+	  /* Use of this member (controlling the IPLT type), is not
+	     fully dominated by PLT refcount tests so always update it.  */
 	  if (!call_reloc_p)
 	    arm_plt->noncall_refcount--;
-
-	  if (r_type == R_ARM_THM_CALL)
-	    arm_plt->maybe_thumb_refcount--;
-
-	  if (r_type == R_ARM_THM_JUMP24
-	      || r_type == R_ARM_THM_JUMP19)
-	    arm_plt->thumb_refcount--;
 	}
 
       if (may_become_dynamic_p)

brgds, H-P

Re: [RFA:] Fix ld/13990, ARM BFD_ASSERT with --shared and --gc-sections

[Hans-Peter Nilsson]

> From: Hans-Peter Nilsson <hp@axis.com>
> Date: Thu, 19 Apr 2012 01:07:25 +0200

> ld/testsuite:
> 
> 	PR ld/13990
> 	* ld-arm/arm-elf.exp: Run gc-hidden-1.
> 	* gc-hidden-1.d: New test-file.
> 	* gcdfn.s, hideall.ld, hidfn.s, main.s: New files.

Bah.  Stupid git.  I mean:

	PR ld/13990
	* ld-arm/arm-elf.exp: Run gc-hidden-1.
	* ld-arm/gc-hidden-1.d: New test-file.
	* ld-arm/gcdfn.s, ld-arm/hideall.ld, ld-arm/hidfn.s,
	ld-arm/main.s: New files.

brgds, H-P

Re: [RFA:] Fix ld/13990, ARM BFD_ASSERT with --shared and --gc-sections

[Richard Sandiford]

I'll leave the proper review to an ARM maintainer, but:

The refcount change itself looks good -- hope it goes in.  However...

Hans-Peter Nilsson <hans-peter.nilsson@axis.com> writes:
> diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
> index f5b5c4d..5907974 100644
> --- a/bfd/elf32-arm.c
> +++ b/bfd/elf32-arm.c
> @@ -12256,18 +12256,34 @@ elf32_arm_gc_sweep_hook (bfd *                     abfd,
>        if (may_need_local_target_p
>  	  && elf32_arm_get_plt_info (abfd, eh, r_symndx, &root_plt, &arm_plt))
>  	{
> -	  BFD_ASSERT (root_plt->refcount > 0);
> -	  root_plt->refcount -= 1;
> +	  /* If PLT refcount book-keeping is wrong and too low, we'll
> +	     see a zero value (going to -1) for the root PLT reference
> +	     count.  */
> +	  if (root_plt->refcount >= 0)
> +	    {
> +	      BFD_ASSERT (root_plt->refcount != 0);
> +	      root_plt->refcount -= 1;
> +
> +	      /* No use incrementing these members as all their uses are
> +		 dominated by tests on the PLT refcount being
> +		 non-negative.  */
> +	      if (r_type == R_ARM_THM_CALL)
> +		arm_plt->maybe_thumb_refcount--;
> +
> +	      if (r_type == R_ARM_THM_JUMP24
> +		  || r_type == R_ARM_THM_JUMP19)
> +		arm_plt->thumb_refcount--;

...while I take your point, I think it would be better to keep
these last two as they are.  The fact that...

> +	    }
> +	  else
> +	    /* A value of -1 means the symbol has become local, forced
> +	       or seeing a hidden definition.  Any other negative value
> +	       is an error.  */
> +	    BFD_ASSERT (root_plt->refcount == -1);
>  
> +	  /* Use of this member (controlling the IPLT type), is not
> +	     fully dominated by PLT refcount tests so always update it.  */
>  	  if (!call_reloc_p)
>  	    arm_plt->noncall_refcount--;
> -
> -	  if (r_type == R_ARM_THM_CALL)
> -	    arm_plt->maybe_thumb_refcount--;
> -
> -	  if (r_type == R_ARM_THM_JUMP24
> -	      || r_type == R_ARM_THM_JUMP19)
> -	    arm_plt->thumb_refcount--;

...noncall_refcount is useful for IPLTs shows that the others may
become useful in future too.  Whoever uses them in future may not
know that this GC code exists.

This block is really supposed to be undoing the counting in
check_relocs.  Information for removed relocs shouldn't be left lying
around to trip up later code.  Your refcount fix sticks to that
principle because for refcount < 0 the precise count has already,
deliberately, been dropped.  The other counts are still meaningful
though, whether or not they are used for refcount < 0 at present.

Thanks,
Richard

Re: [RFA:] Fix ld/13990, ARM BFD_ASSERT with --shared and --gc-sections

[Hans-Peter Nilsson]

> From: Richard Sandiford <rdsandiford@googlemail.com>
> CC: "binutils@sourceware.org" <binutils@sourceware.org>
> Date: Thu, 19 Apr 2012 22:38:17 +0200

> Information for removed relocs shouldn't be left lying
> around to trip up later code.  Your refcount fix sticks to that
> principle because for refcount < 0 the precise count has already,
> deliberately, been dropped.  The other counts are still meaningful
> though, whether or not they are used for refcount < 0 at present.

Yah...  I'll call that a thinko; their *update* increments are
independent even though their current meaning is tied to the
existence of a non-IPLT PLT.

Here's an update, instead of a ping, tested arm-eabi and
arm-linux-gnueabi.  ld/testsuite as in previous message.

Ok to commit?

bfd:
	PR ld/13990
	* elf32-arm.c (elf32_arm_gc_sweep_hook): Handle a forced-local
	symbol, where PLT refcount is set to -1.


diff --git a/bfd/elf32-arm.c b/bfd/elf32-arm.c
index f5b5c4d..e2fb62d 100644
--- a/bfd/elf32-arm.c
+++ b/bfd/elf32-arm.c
@@ -12256,8 +12256,19 @@ elf32_arm_gc_sweep_hook (bfd *                     abfd,
       if (may_need_local_target_p
 	  && elf32_arm_get_plt_info (abfd, eh, r_symndx, &root_plt, &arm_plt))
 	{
-	  BFD_ASSERT (root_plt->refcount > 0);
-	  root_plt->refcount -= 1;
+	  /* If PLT refcount book-keeping is wrong and too low, we'll
+	     see a zero value (going to -1) for the root PLT reference
+	     count.  */
+	  if (root_plt->refcount >= 0)
+	    {
+	      BFD_ASSERT (root_plt->refcount != 0);
+	      root_plt->refcount -= 1;
+	    }
+	  else
+	    /* A value of -1 means the symbol has become local, forced
+	       or seeing a hidden definition.  Any other negative value
+	       is an error.  */
+	    BFD_ASSERT (root_plt->refcount == -1);
 
 	  if (!call_reloc_p)
 	    arm_plt->noncall_refcount--;

brgds, H-P

Re: [RFA:] Fix ld/13990, ARM BFD_ASSERT with --shared and --gc-sections

[Alan Modra]

On Tue, Apr 24, 2012 at 03:07:58AM +0200, Hans-Peter Nilsson wrote:
> 	PR ld/13990
> 	* elf32-arm.c (elf32_arm_gc_sweep_hook): Handle a forced-local
> 	symbol, where PLT refcount is set to -1.

OK.

-- 
Alan Modra
Australia Development Lab, IBM