[PATCH]: gold sparc unaligned...

[PATCH]: gold sparc unaligned...

[David Miller]

With gcc-4.4, or I guess any gcc emitting .cfi_personality directives,
we get a SIGBUS on sparc in gold during the testsuite.

The reason is that the dwarf2 emitter in gas is emitting R_SPARC_32
relocs on unaligned offsets.

This never gets noticed with the BFD linker because the sparc BFD
backend looks for unaligned relocation offsets and handles them
like R_SPARC_UA* relocs when it finds such unaligned relocs.

The PowerPC BFD linker backends do something similar.

I could hack the sparc GAS target code to change which reloc
it emits, as needed, but who knows how many objects out there
already have this unaligned R_SPARC_32 reloc and gold should
be able to link against existing objects.

BTW, when playing around with fixing this in GAS I noticed that the
gold testsuite doesn't use GAS from the build tree.

Ok to commit?

gold/

2010-02-08  David S. Miller  <davem@davemloft.net>

	* sparc.cc (Target_sparc::Relocate::relocate): If relocation offset is
	unaligned for R_SPARC_16, R_SPARC_32, or R_SPARC_64, use the unaligned
	relocation helper function.

diff --git a/gold/sparc.cc b/gold/sparc.cc
index c5ce06a..fee9554 100644
--- a/gold/sparc.cc
+++ b/gold/sparc.cc
@@ -2447,14 +2447,22 @@ Target_sparc<size, big_endian>::Relocate::relocate(
       break;
 
     case elfcpp::R_SPARC_16:
-      Relocate_functions<size, big_endian>::rela16(view, object,
-						   psymval, addend);
+      if (rela.get_r_offset() & 0x1)
+	Reloc::ua16(view, object, psymval, addend);
+      else
+	Relocate_functions<size, big_endian>::rela16(view, object,
+						     psymval, addend);
       break;
 
     case elfcpp::R_SPARC_32:
       if (!parameters->options().output_is_position_independent())
-	Relocate_functions<size, big_endian>::rela32(view, object,
-						     psymval, addend);
+	{
+	  if (rela.get_r_offset() & 0x3)
+	    Reloc::ua32(view, object, psymval, addend);
+	  else
+	    Relocate_functions<size, big_endian>::rela32(view, object,
+							 psymval, addend);
+	}
       break;
 
     case elfcpp::R_SPARC_DISP8:
@@ -2563,8 +2571,13 @@ Target_sparc<size, big_endian>::Relocate::relocate(
 
     case elfcpp::R_SPARC_64:
       if (!parameters->options().output_is_position_independent())
-	      Relocate_functions<size, big_endian>::rela64(view, object,
-							   psymval, addend);
+	{
+	  if (rela.get_r_offset() & 0x7)
+	    Reloc::ua64(view, object, psymval, addend);
+	  else
+	    Relocate_functions<size, big_endian>::rela64(view, object,
+							 psymval, addend);
+	}
       break;
 
     case elfcpp::R_SPARC_OLO10:

Re: [PATCH]: gold sparc unaligned...

[Ian Lance Taylor]

David Miller <davem@davemloft.net> writes:

> BTW, when playing around with fixing this in GAS I noticed that the
> gold testsuite doesn't use GAS from the build tree.

When running the assembler directly it ought to use the one from the
build tree.  It's true that when running the compiler it does not pass
a -B option to use the assembler in the source tree.  Do you think it
should?


> 2010-02-08  David S. Miller  <davem@davemloft.net>
>
> 	* sparc.cc (Target_sparc::Relocate::relocate): If relocation offset is
> 	unaligned for R_SPARC_16, R_SPARC_32, or R_SPARC_64, use the unaligned
> 	relocation helper function.

This is OK if you add a comment at each site explaining why we are
doing things in this less efficient way.

I'm actually surprised this doesn't work since I thought the Solaris
linker did enforce the alignment requirement.  Or is it that it works
with the Solaris assembler but not gas?  In which case this patch is
still OK, I guess, but we should also fix gas.

Thanks.

Ian

Re: [PATCH]: gold sparc unaligned...

[David Miller]

From: Ian Lance Taylor <iant@google.com>
Date: Mon, 08 Feb 2010 15:49:57 -0800

> David Miller <davem@davemloft.net> writes:
> 
>> BTW, when playing around with fixing this in GAS I noticed that the
>> gold testsuite doesn't use GAS from the build tree.
> 
> When running the assembler directly it ought to use the one from the
> build tree.  It's true that when running the compiler it does not pass
> a -B option to use the assembler in the source tree.  Do you think it
> should?

Passing in -B to get the in-tree assembler would be a good way
to find regressions when doing development, I think.

>> 2010-02-08  David S. Miller  <davem@davemloft.net>
>>
>> 	* sparc.cc (Target_sparc::Relocate::relocate): If relocation offset is
>> 	unaligned for R_SPARC_16, R_SPARC_32, or R_SPARC_64, use the unaligned
>> 	relocation helper function.
> 
> This is OK if you add a comment at each site explaining why we are
> doing things in this less efficient way.

Ok, will do.

> I'm actually surprised this doesn't work since I thought the Solaris
> linker did enforce the alignment requirement.  Or is it that it works
> with the Solaris assembler but not gas?  In which case this patch is
> still OK, I guess, but we should also fix gas.

I really have no idea.  I doubt many people test with GAS but not GNU
ld on Solaris.

Re: [PATCH]: gold sparc unaligned...

[Eric Botcazou]

> I really have no idea.  I doubt many people test with GAS but not GNU
> ld on Solaris.

We do it all the time, the Solaris assembler is not very well maintained and 
the GNU linker doesn't have all the features of the Sun linker on Solaris 10.

So it would be better to do something like:

2006-11-27  Eric Botcazou  <ebotcazou@adacore.com>

	* config/tc-sparc.c (tc_gen_reloc): Turn aligned relocs into
	their unaligned counterparts in debugging sections.

-- 
Eric Botcazou

Re: [PATCH]: gold sparc unaligned...

[David Miller]

From: Eric Botcazou <ebotcazou@adacore.com>
Date: Tue, 9 Feb 2010 17:14:11 +0100

>> I really have no idea.  I doubt many people test with GAS but not GNU
>> ld on Solaris.
> 
> We do it all the time, the Solaris assembler is not very well maintained and 
> the GNU linker doesn't have all the features of the Sun linker on Solaris 10.

I'm steadily trying to close those gaps.  I actually enjoy hacking on
these tools so just tell me what the most important stuff is. :-)

What assembler features are missing?  I thought I had added the most
important ones already.

After I finish GOTDATA_OP optimization support (will post the patch
for that today) the only missing linker features should be the fortran
data compression hacks.  Are there any others you really need?

> So it would be better to do something like:
> 
> 2006-11-27  Eric Botcazou  <ebotcazou@adacore.com>
> 
> 	* config/tc-sparc.c (tc_gen_reloc): Turn aligned relocs into
> 	their unaligned counterparts in debugging sections.

This is a good idea, but overkill.  We have access to the
relocation information, so we can see whether the offset
is going to be unaligned or not.

I cooked up the following patch yesterday while looking
into these issues.  Want me to commit it?

gas/

2010-02-09  David S. Miller  <davem@davemloft.net>

	* config/tc-sparc.c (cons_fix_new_sparc): Use unaligned reloc
	variant if relocation offset is unaligned.

diff --git a/gas/config/tc-sparc.c b/gas/config/tc-sparc.c
index 26ecc86..d65c71a 100644
--- a/gas/config/tc-sparc.c
+++ b/gas/config/tc-sparc.c
@@ -4443,15 +4443,41 @@ cons_fix_new_sparc (fragS *frag,
 		    expressionS *exp)
 {
   bfd_reloc_code_real_type r;
+  int unaligned = 0;
 
-  r = (nbytes == 1 ? BFD_RELOC_8 :
-       (nbytes == 2 ? BFD_RELOC_16 :
-	(nbytes == 4 ? BFD_RELOC_32 : BFD_RELOC_64)));
+  switch (nbytes)
+    {
+    case 1:
+      r = BFD_RELOC_8;
+      break;
+
+    case 2:
+      r = BFD_RELOC_16;
+      unaligned = (where & 1);
+      break;
+
+    case 4:
+      r = BFD_RELOC_32;
+      unaligned = (where & 3);
+      break;
+
+    case 8:
+      r = BFD_RELOC_64;
+      unaligned = (where & 7);
+      break;
+
+    default:
+      abort ();
+    }
 
   if (target_little_endian_data
       && nbytes == 4
       && now_seg->flags & SEC_ALLOC)
-    r = BFD_RELOC_SPARC_REV32;
+    {
+      if (unaligned)
+	abort ();
+      r = BFD_RELOC_SPARC_REV32;
+    }
 
   if (sparc_cons_special_reloc)
     {
@@ -4477,7 +4503,8 @@ cons_fix_new_sparc (fragS *frag,
 	  case 8: r = BFD_RELOC_SPARC_TLS_DTPOFF64; break;
 	  }
     }
-  else if (sparc_no_align_cons)
+  else if (sparc_no_align_cons
+	   || unaligned)
     {
       switch (nbytes)
 	{

Re: [PATCH]: gold sparc unaligned...

[Eric Botcazou]

> After I finish GOTDATA_OP optimization support (will post the patch
> for that today) the only missing linker features should be the fortran
> data compression hacks.  Are there any others you really need?

IIRC shared libraries on Solaris 10 contain special symbols that the GNU 
linker cannot handle.  There is probably a bugzilla ticket about it.

> I cooked up the following patch yesterday while looking
> into these issues.  Want me to commit it?

Looks overkill too. :-)  There is at the end of config/tc-sparc.c:

void
sparc_cfi_emit_pcrel_expr (expressionS *exp, unsigned int nbytes)
{
  sparc_cons_special_reloc = "disp";
  sparc_no_align_cons = 1;
  emit_expr (exp, nbytes);
  sparc_no_align_cons = 0;
  sparc_cons_special_reloc = NULL;
}

so why doesn't gas generate the SPARC_UA* variants directly?

-- 
Eric Botcazou

Re: [PATCH]: gold sparc unaligned...

[David Miller]

From: Eric Botcazou <ebotcazou@adacore.com>
Date: Tue, 9 Feb 2010 18:55:46 +0100

> Looks overkill too. :-)  There is at the end of config/tc-sparc.c:
>
> void
> sparc_cfi_emit_pcrel_expr (expressionS *exp, unsigned int nbytes)
> {
>   sparc_cons_special_reloc = "disp";
>   sparc_no_align_cons = 1;
>   emit_expr (exp, nbytes);
>   sparc_no_align_cons = 0;
>   sparc_cons_special_reloc = NULL;
> }
> 
> so why doesn't gas generate the SPARC_UA* variants directly?

That special hook is only invoked by the dwarf2 CFI emitter
for pcrel cases.  It isn't going to be used for the expressions
generated by .cfi_personality

Re: [PATCH]: gold sparc unaligned...

[David Miller]

From: Eric Botcazou <ebotcazou@adacore.com>
Date: Tue, 9 Feb 2010 18:55:46 +0100

>> After I finish GOTDATA_OP optimization support (will post the patch
>> for that today) the only missing linker features should be the fortran
>> data compression hacks.  Are there any others you really need?
> 
> IIRC shared libraries on Solaris 10 contain special symbols that the GNU 
> linker cannot handle.  There is probably a bugzilla ticket about it.

Looks like some symbol versioning issues.

PR/1021 and PR/1031, which you partially fixed.

Re: [PATCH]: gold sparc unaligned...

[Eric Botcazou]

> That special hook is only invoked by the dwarf2 CFI emitter
> for pcrel cases.  It isn't going to be used for the expressions
> generated by .cfi_personality

You're right.  In fact, I've found out why there is no SIGBUS on Solaris with 
recent GNU as and Solaris ld.  Despite:

/* Define 0/1 if your assembler supports CFI directives. */
#define HAVE_GAS_CFI_DIRECTIVE 1
#define HAVE_GAS_CFI_PERSONALITY_DIRECTIVE 1

in the auto-host file of GCC, the CFI directives are still disabled in this 
configuration because of:

/* Decide whether to emit frame unwind via assembler directives.  */

int
dwarf2out_do_cfi_asm (void)
{
[...]
  /* Make sure the personality encoding is one the assembler can support.
     In particular, aligned addresses can't be handled.  */
  enc = ASM_PREFERRED_EH_DATA_FORMAT (/*code=*/2,/*global=*/1);
  if ((enc & 0x70) != 0 && (enc & 0x70) != DW_EH_PE_pcrel)
    return false;

and config/sparc/sol2.h has:

/* Select a format to encode pointers in exception handling data.  CODE
   is 0 for data, 1 for code labels, 2 for function pointers.  GLOBAL is
   true if the symbol may be affected by dynamic relocations.

   Some Solaris dynamic linkers don't handle unaligned section relative
   relocs properly, so force them to be aligned.  */
#ifndef HAVE_AS_SPARC_UA_PCREL
#define ASM_PREFERRED_EH_DATA_FORMAT(CODE,GLOBAL)		\
  ((flag_pic || GLOBAL) ? DW_EH_PE_aligned : DW_EH_PE_absptr)
#endif

and HAVE_AS_SPARC_UA_PCREL isn't defined.

-- 
Eric Botcazou

Re: [PATCH]: gold sparc unaligned...

[David Miller]

From: Eric Botcazou <ebotcazou@adacore.com>
Date: Thu, 11 Feb 2010 22:13:13 +0100

> and config/sparc/sol2.h has:
> 
> /* Select a format to encode pointers in exception handling data.  CODE
>    is 0 for data, 1 for code labels, 2 for function pointers.  GLOBAL is
>    true if the symbol may be affected by dynamic relocations.
> 
>    Some Solaris dynamic linkers don't handle unaligned section relative
>    relocs properly, so force them to be aligned.  */
> #ifndef HAVE_AS_SPARC_UA_PCREL
> #define ASM_PREFERRED_EH_DATA_FORMAT(CODE,GLOBAL)		\
>   ((flag_pic || GLOBAL) ? DW_EH_PE_aligned : DW_EH_PE_absptr)
> #endif
> 
> and HAVE_AS_SPARC_UA_PCREL isn't defined.

Dwarf2 is such a mess on sparc because of all of these unaligned
relocation issues.

Thanks for reporting all of this detective work Eric.