PR20989, sparc GOT sequence optimization

PR20989, sparc GOT sequence optimization

[Alan Modra]

This patch stops sparc ld doing the wrong thing when optimizing loads
from the GOT to a GOT pointer relative sequence.  Tested by code
inspection and cross-build of binutils+testsuite as I don't have a
sparc system handy to do native regression testing.  OK?

It would be easy to edit these code sequences to avoid the GOT load
for undefined weak hidden visibility syms by loading a zero inline,
but I'll leave that to the maintainers.  Also, you might like to check
why the testcase in the PR didn't signal a relocation overflow..

Oh, and I strongly suspect the same bug exists in gold.

	PR ld/20989
	* elfxx-sparc.c (gdop_relative_offset_ok): New function.
	(_bfd_sparc_elf_relocate_section): Use it to validate GOT
	indirect to GOT pointer relative code edit.

diff --git a/bfd/elfxx-sparc.c b/bfd/elfxx-sparc.c
index d7b2688..ca43c41 100644
--- a/bfd/elfxx-sparc.c
+++ b/bfd/elfxx-sparc.c
@@ -2927,6 +2927,33 @@ gdopoff (struct bfd_link_info *info, bfd_vma address)
   return address - got_base;
 }
 
+/* Return whether H is local and its ADDRESS is within 4G of
+   _GLOBAL_OFFSET_TABLE_ and thus the offset may be calculated by a
+   sethi, xor sequence.  */
+
+static bfd_boolean
+gdop_relative_offset_ok (struct bfd_link_info *info,
+			 struct elf_link_hash_entry *h,
+			 bfd_vma address ATTRIBUTE_UNUSED)
+{
+  if (!SYMBOL_REFERENCES_LOCAL (info, h))
+    return FALSE;
+  /* If H is undefined, ADDRESS will be zero.  We can't allow a
+     relative offset to "zero" when producing PIEs or shared libs.
+     Note that to get here with an undefined symbol it must also be
+     hidden or internal visibility.  */
+  if (bfd_link_pic (info)
+      && h != NULL
+      && (h->root.type == bfd_link_hash_undefweak
+	  || h->root.type == bfd_link_hash_undefined))
+    return FALSE;
+#ifdef BFD64
+  return gdopoff (info, address) + ((bfd_vma) 1 << 32) < (bfd_vma) 2 << 32;
+#else
+  return TRUE;
+#endif
+}
+
 /* Relocate a SPARC ELF section.  */
 
 bfd_boolean
@@ -3168,7 +3195,7 @@ _bfd_sparc_elf_relocate_section (bfd *output_bfd,
 	{
 	case R_SPARC_GOTDATA_OP_HIX22:
 	case R_SPARC_GOTDATA_OP_LOX10:
-	  if (SYMBOL_REFERENCES_LOCAL (info, h))
+	  if (gdop_relative_offset_ok (info, h, relocation))
 	    {
 	      r_type = (r_type == R_SPARC_GOTDATA_OP_HIX22
 			? R_SPARC_GOTDATA_HIX22
@@ -3178,7 +3205,7 @@ _bfd_sparc_elf_relocate_section (bfd *output_bfd,
 	  break;
 
 	case R_SPARC_GOTDATA_OP:
-	  if (SYMBOL_REFERENCES_LOCAL (info, h))
+	  if (gdop_relative_offset_ok (info, h, relocation))
 	    {
 	      bfd_vma insn = bfd_get_32 (input_bfd, contents + rel->r_offset);
 

-- 
Alan Modra
Australia Development Lab, IBM

Re: PR20989, sparc GOT sequence optimization

[Jose E. Marchesi]

Hi Alan.

    This patch stops sparc ld doing the wrong thing when optimizing loads
    from the GOT to a GOT pointer relative sequence.  Tested by code
    inspection and cross-build of binutils+testsuite as I don't have a
    sparc system handy to do native regression testing.  OK?

    It would be easy to edit these code sequences to avoid the GOT load
    for undefined weak hidden visibility syms by loading a zero inline,
    but I'll leave that to the maintainers.  Also, you might like to check
    why the testcase in the PR didn't signal a relocation overflow..
    
    Oh, and I strongly suspect the same bug exists in gold.
    
    	PR ld/20989
    	* elfxx-sparc.c (gdop_relative_offset_ok): New function.
    	(_bfd_sparc_elf_relocate_section): Use it to validate GOT
    	indirect to GOT pointer relative code edit.
    
    diff --git a/bfd/elfxx-sparc.c b/bfd/elfxx-sparc.c
    index d7b2688..ca43c41 100644
    --- a/bfd/elfxx-sparc.c
    +++ b/bfd/elfxx-sparc.c
    @@ -2927,6 +2927,33 @@ gdopoff (struct bfd_link_info *info, bfd_vma address)
       return address - got_base;
     }
     
    +/* Return whether H is local and its ADDRESS is within 4G of
    +   _GLOBAL_OFFSET_TABLE_ and thus the offset may be calculated by a
    +   sethi, xor sequence.  */

Generally speaking the address must be within {+-}2G of
_GLOBAL_OFFSET_TABLE_, i.e. there can be negative offsets.

I did an attempt recently to support negative offsets in GNU only to
find that we need some additional GCC support, but I will revisit it
soon.

Otherwise the fix looks good to me.  I tested it in sparc64-linux-gnu
and sparcv9-linux-gnu targets.  No regressions.

Thanks!

Re: PR20989, sparc GOT sequence optimization

[Rainer Orth]

Hi Jose, Alan,

> Otherwise the fix looks good to me.  I tested it in sparc64-linux-gnu
> and sparcv9-linux-gnu targets.  No regressions.

I've run a sparc-sun-solaris2.12 gcc mainline bootstrap last night,
using gas and gld 2.27 with your patch applied.  Bootstrap is restored
and test results are at the same level as the gas/ld combination, with
the (unchanged) exception of PR ld/20179 related failures.

Thanks.
	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: PR20989, sparc GOT sequence optimization

[Alan Modra]

On Mon, Jan 02, 2017 at 09:53:44AM +0100, Jose E. Marchesi wrote:
>     +/* Return whether H is local and its ADDRESS is within 4G of
>     +   _GLOBAL_OFFSET_TABLE_ and thus the offset may be calculated by a
>     +   sethi, xor sequence.  */
> 
> Generally speaking the address must be within {+-}2G of
> _GLOBAL_OFFSET_TABLE_, i.e. there can be negative offsets.

I allowed +/-4G, and believe that is the range of sethi, xor.

	sethi	%hi(-1), %l7      # 0x0000 0000 ffff fc00
	xor	%l7, -0x400, %l6  # 0xffff ffff 0000 0000

	xor	%l7, 0x3ff, %l5   # 0x0000 0000 ffff ffff

-- 
Alan Modra
Australia Development Lab, IBM

Re: PR20989, sparc GOT sequence optimization

[Jose E. Marchesi]

    >     +/* Return whether H is local and its ADDRESS is within 4G of
    >     +   _GLOBAL_OFFSET_TABLE_ and thus the offset may be calculated by a
    >     +   sethi, xor sequence.  */
    > 
    > Generally speaking the address must be within {+-}2G of
    > _GLOBAL_OFFSET_TABLE_, i.e. there can be negative offsets.
    
    I allowed +/-4G, and believe that is the range of sethi, xor.
    
    	sethi	%hi(-1), %l7      # 0x0000 0000 ffff fc00
    	xor	%l7, -0x400, %l6  # 0xffff ffff 0000 0000
    
    	xor	%l7, 0x3ff, %l5   # 0x0000 0000 ffff ffff

I stand corrected.  In 64-bits the range is indeed a signed 33-bit
constant.