This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]