[PATCH] Don't relocate R_MIPS_JALR against dynamic symbols

[PATCH] Don't relocate R_MIPS_JALR against dynamic symbols

[Adam Nemet]

This was also found while regression testing the R_MIPS_JALR patch in GCC.

Current logic in mips_elf_calculate_relocation refuses to relocate a
R_MIPS_JALR if there is a lazy-stub defined for the symbol.  If there is no
lazy-stub we're happy to perform the relocation which can of course still be
against a dynamic symbol.

The change below makes the check stricter.

Regtested with ld on mips64octeon-linux.  Also the C-only bootstrap of the
R_MIPS_JALR GCC patch passes with these two BFD patches.

OK?

Adam


	* elfxx-mips.c (mips_elf_calculate_relocation): Don't relocate
	R_MIPS_JALR unless symbol is def_regular.

Index: elfxx-mips.c
===================================================================
RCS file: /cvs/src/src/bfd/elfxx-mips.c,v
retrieving revision 1.257
diff -F^\([(a-zA-Z0-9_]\|#define\) -u -p -u -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.257 elfxx-mips.c
--- elfxx-mips.c	5 Aug 2009 21:17:51 -0000	1.257
+++ elfxx-mips.c	22 Aug 2009 17:55:10 -0000
@@ -5489,8 +5489,9 @@ mips_elf_calculate_relocation (bfd *abfd
     case R_MIPS_JALR:
       /* This relocation is only a hint.  In some cases, we optimize
 	 it into a bal instruction.  But we don't try to optimize
-	 branches to the PLT; that will wind up wasting time.  */
-      if (h != NULL && h->root.plt.offset != (bfd_vma) -1)
+	 branches to symbols in shared object; those either go to the
+	 lazy stub or can only be resolved at run-time.  */
+      if (h != NULL && !h->root.def_regular)
 	return bfd_reloc_continue;
       value = symbol + addend;
       break;

Re: [PATCH] Don't relocate R_MIPS_JALR against dynamic symbols

[Richard Sandiford]

Adam Nemet <anemet@caviumnetworks.com> writes:
> Index: elfxx-mips.c
> ===================================================================
> RCS file: /cvs/src/src/bfd/elfxx-mips.c,v
> retrieving revision 1.257
> diff -F^\([(a-zA-Z0-9_]\|#define\) -u -p -u -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.257 elfxx-mips.c
> --- elfxx-mips.c	5 Aug 2009 21:17:51 -0000	1.257
> +++ elfxx-mips.c	22 Aug 2009 17:55:10 -0000
> @@ -5489,8 +5489,9 @@ mips_elf_calculate_relocation (bfd *abfd
>      case R_MIPS_JALR:
>        /* This relocation is only a hint.  In some cases, we optimize
>  	 it into a bal instruction.  But we don't try to optimize
> -	 branches to the PLT; that will wind up wasting time.  */
> -      if (h != NULL && h->root.plt.offset != (bfd_vma) -1)
> +	 branches to symbols in shared object; those either go to the
> +	 lazy stub or can only be resolved at run-time.  */
> +      if (h != NULL && !h->root.def_regular)
>  	return bfd_reloc_continue;
>        value = symbol + addend;
>        break;

I agree this is an improvement, but couldn't we still end up
using BAL when we shouldn't if:

  (a) the link is not -Bsymbolic
  (b) the function has a regular .o definition
  (c) no call references are lazily bound (either because the
      author decided to use %got* instead of %call*, or because
      -znow is in effect)

?  I think we want:

     if (h != NULL && !SYMBOL_CALLS_LOCAL (info, h))

instead.

Richard

Re: [PATCH] Don't relocate R_MIPS_JALR against dynamic symbols

[Adam Nemet]

Richard Sandiford writes:
> Adam Nemet <anemet@caviumnetworks.com> writes:
> > Index: elfxx-mips.c
> > ===================================================================
> > RCS file: /cvs/src/src/bfd/elfxx-mips.c,v
> > retrieving revision 1.257
> > diff -F^\([(a-zA-Z0-9_]\|#define\) -u -p -u -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.257 elfxx-mips.c
> > --- elfxx-mips.c	5 Aug 2009 21:17:51 -0000	1.257
> > +++ elfxx-mips.c	22 Aug 2009 17:55:10 -0000
> > @@ -5489,8 +5489,9 @@ mips_elf_calculate_relocation (bfd *abfd
> >      case R_MIPS_JALR:
> >        /* This relocation is only a hint.  In some cases, we optimize
> >  	 it into a bal instruction.  But we don't try to optimize
> > -	 branches to the PLT; that will wind up wasting time.  */
> > -      if (h != NULL && h->root.plt.offset != (bfd_vma) -1)
> > +	 branches to symbols in shared object; those either go to the
> > +	 lazy stub or can only be resolved at run-time.  */
> > +      if (h != NULL && !h->root.def_regular)
> >  	return bfd_reloc_continue;
> >        value = symbol + addend;
> >        break;
> 
> I agree this is an improvement, but couldn't we still end up
> using BAL when we shouldn't if:
> 
>   (a) the link is not -Bsymbolic
>   (b) the function has a regular .o definition
>   (c) no call references are lazily bound (either because the
>       author decided to use %got* instead of %call*, or because
>       -znow is in effect)

Right I found out about it too, the hard way:

  http://sourceware.org/ml/binutils/2009-08/msg00501.html

> ?  I think we want:
> 
>      if (h != NULL && !SYMBOL_CALLS_LOCAL (info, h))
> 
> instead.

I'll try that.  Should I check it in if it works (regtest, GCC bootstrap)?

Adam

Re: [PATCH] Don't relocate R_MIPS_JALR against dynamic symbols

[Richard Sandiford]

Adam Nemet <anemet@caviumnetworks.com> writes:
> Richard Sandiford writes:
>> Adam Nemet <anemet@caviumnetworks.com> writes:
>> > Index: elfxx-mips.c
>> > ===================================================================
>> > RCS file: /cvs/src/src/bfd/elfxx-mips.c,v
>> > retrieving revision 1.257
>> > diff -F^\([(a-zA-Z0-9_]\|#define\) -u -p -u -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.257 elfxx-mips.c
>> > --- elfxx-mips.c	5 Aug 2009 21:17:51 -0000	1.257
>> > +++ elfxx-mips.c	22 Aug 2009 17:55:10 -0000
>> > @@ -5489,8 +5489,9 @@ mips_elf_calculate_relocation (bfd *abfd
>> >      case R_MIPS_JALR:
>> >        /* This relocation is only a hint.  In some cases, we optimize
>> >  	 it into a bal instruction.  But we don't try to optimize
>> > -	 branches to the PLT; that will wind up wasting time.  */
>> > -      if (h != NULL && h->root.plt.offset != (bfd_vma) -1)
>> > +	 branches to symbols in shared object; those either go to the
>> > +	 lazy stub or can only be resolved at run-time.  */
>> > +      if (h != NULL && !h->root.def_regular)
>> >  	return bfd_reloc_continue;
>> >        value = symbol + addend;
>> >        break;
>> 
>> I agree this is an improvement, but couldn't we still end up
>> using BAL when we shouldn't if:
>> 
>>   (a) the link is not -Bsymbolic
>>   (b) the function has a regular .o definition
>>   (c) no call references are lazily bound (either because the
>>       author decided to use %got* instead of %call*, or because
>>       -znow is in effect)
>
> Right I found out about it too, the hard way:
>
>   http://sourceware.org/ml/binutils/2009-08/msg00501.html

Doh, sorry, hadn't got to that message by the time I replied.

>> ?  I think we want:
>> 
>>      if (h != NULL && !SYMBOL_CALLS_LOCAL (info, h))
>> 
>> instead.
>
> I'll try that.  Should I check it in if it works (regtest, GCC bootstrap)?

Yeah, please do.

Richard

Re: [PATCH] Don't relocate R_MIPS_JALR against dynamic symbols

[Adam Nemet]

Richard Sandiford writes:
> Adam Nemet <anemet@caviumnetworks.com> writes:
> > Richard Sandiford writes:
> >> ?  I think we want:
> >> 
> >>      if (h != NULL && !SYMBOL_CALLS_LOCAL (info, h))
> >> 
> >> instead.
> >
> > I'll try that.  Should I check it in if it works (regtest, GCC bootstrap)?
> 
> Yeah, please do.

This is what I've checked in after a GCC bootstrap with my R_MIPS_JALR
annotation changes and an ld regtest for mips64octeon-linux.

Sorry about the delay.  Seems like I still made it before the branch.

Adam

Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/bfd/ChangeLog,v
retrieving revision 1.4758
diff -F^\([(a-zA-Z0-9_]\|#define\) -u -p -r1.4758 ChangeLog
--- ChangeLog	3 Sep 2009 18:21:21 -0000	1.4758
+++ ChangeLog	3 Sep 2009 18:25:51 -0000
@@ -1,5 +1,10 @@
 2009-09-03  Adam Nemet  <anemet@caviumnetworks.com>
 
+	* elfxx-mips.c (mips_elf_calculate_relocation): Don't relocate
+	R_MIPS_JALR unless symbol resolves locally.
+
+2009-09-03  Adam Nemet  <anemet@caviumnetworks.com>
+
 	* elfxx-mips.c (_bfd_mips_elf_check_relocs): Don't set
 	has_static_relocs for R_MIPS_JALR.
 
Index: elfxx-mips.c
===================================================================
RCS file: /cvs/src/src/bfd/elfxx-mips.c,v
retrieving revision 1.259
diff -F^\([(a-zA-Z0-9_]\|#define\) -u -p -r1.259 elfxx-mips.c
--- elfxx-mips.c	3 Sep 2009 18:21:21 -0000	1.259
+++ elfxx-mips.c	3 Sep 2009 18:25:52 -0000
@@ -5489,8 +5489,8 @@ mips_elf_calculate_relocation (bfd *abfd
     case R_MIPS_JALR:
       /* This relocation is only a hint.  In some cases, we optimize
 	 it into a bal instruction.  But we don't try to optimize
-	 branches to the PLT; that will wind up wasting time.  */
-      if (h != NULL && h->root.plt.offset != (bfd_vma) -1)
+	 when the symbol does not resolve locally.  */
+      if (h != NULL && !SYMBOL_CALLS_LOCAL (info, &h->root))
 	return bfd_reloc_continue;
       value = symbol + addend;
       break;