[PATCH]: Set little endian flag when linking SH arch tests for little endian multilibs

[PATCH]: Set little endian flag when linking SH arch tests for little endian multilibs

[Nick Clifton]

Hi Guys,

  I am applying the small patch below to make the SH specific linker
  tests set the appropriate endian flag when linking the arch tests.
  Without this, testing little endian SH multilibs was producing lots
  of unexpected failures and untested tests because the linker and
  assembler disagreed about endianness.

Cheers
  Nick

ld/testsuite/ChangeLog
2006-07-21  Nick Clifton  <nickc@redhat.com>

	* ld-sh/arch/arch.exp (test_arch): Set the endian flag to suit the
	multilib being tested.

Index: ld/testsuite/ld-sh/arch/arch.exp
===================================================================
RCS file: /cvs/src/src/ld/testsuite/ld-sh/arch/arch.exp,v
retrieving revision 1.4
diff -c -3 -p -r1.4 arch.exp
*** ld/testsuite/ld-sh/arch/arch.exp	12 May 2005 07:32:08 -0000	1.4
--- ld/testsuite/ld-sh/arch/arch.exp	21 Jul 2006 09:53:25 -0000
*************** proc test_arch { file1 file2 arch result
*** 76,84 ****
      set name2 [file tail $file2]
      set rootname2 [file rootname $name2]
  
      # This must use -r to prevent LD trying to relocate the (unrealistic) file
!     send_log "$LD -r -o ${rootname1}_${rootname2}.o $file1 $file2\n"
!     catch "exec $LD -r -o ${rootname1}_${rootname2}.o $file1 $file2" ld_output
      send_log $ld_output
  
      if {[string equal $ld_output ""] == 1} then {
--- 76,86 ----
      set name2 [file tail $file2]
      set rootname2 [file rootname $name2]
  
+     set flags [big_or_little_endian]
+     
      # This must use -r to prevent LD trying to relocate the (unrealistic) file
!     send_log "$LD $flags -r -o ${rootname1}_${rootname2}.o $file1 $file2\n"
!     catch "exec $LD $flags -r -o ${rootname1}_${rootname2}.o $file1 $file2" ld_output
      send_log $ld_output
  
      if {[string equal $ld_output ""] == 1} then {

Re: [PATCH]: Set little endian flag when linking SH arch tests for little endian multilibs

[Andrew STUBBS]

[-- Attachment #1: Type: text/plain, Size: 840 bytes --]

Nick Clifton wrote:
>   I am applying the small patch below to make the SH specific linker
>   tests set the appropriate endian flag when linking the arch tests.
>   Without this, testing little endian SH multilibs was producing lots
>   of unexpected failures and untested tests because the linker and
>   assembler disagreed about endianness.

As it happens I was just in the process of preparing that exact same 
patch (right down to the character - spooky).

I also had an issue with default_ld_assemble in lib/ld-lib.exp. It tries 
to apply linker endian flags to the assembler. I have attached the patch 
I was using. Did you not see this issue?

There is also an issue because not all SH architectures have a little 
endian variant, and therefore not all instructions and relocations are 
implemented in both endians.

Andrew Stubbs

[-- Attachment #2: testsuite-flags.patch --]
[-- Type: text/plain, Size: 2183 bytes --]

Index: src/ld/testsuite/lib/ld-lib.exp
===================================================================
--- src.orig/ld/testsuite/lib/ld-lib.exp	2006-07-05 12:28:54.000000000 +0100
+++ src/ld/testsuite/lib/ld-lib.exp	2006-07-05 14:24:10.000000000 +0100
@@ -276,6 +276,10 @@ proc default_ld_assemble { as source obj
     if ![info exists ASFLAGS] { set ASFLAGS "" }
 
     set flags [big_or_little_endian]
+    if {[istarget sh*-*-*] || [istarget pj*-*-*]} {
+	regsub -- {-EB} $flags {-big} flags
+	regsub -- {-EL} $flags {-little} flags
+    }
 
     verbose -log "$as $flags $ASFLAGS -o $object $source"
 
Index: src/ld/testsuite/ld-sh/arch/arch.exp
===================================================================
--- src.orig/ld/testsuite/ld-sh/arch/arch.exp	2005-05-12 08:32:08.000000000 +0100
+++ src/ld/testsuite/ld-sh/arch/arch.exp	2006-07-05 16:25:05.000000000 +0100
@@ -76,9 +76,11 @@ proc test_arch { file1 file2 arch result
     set name2 [file tail $file2]
     set rootname2 [file rootname $name2]
 
+    set flags [big_or_little_endian]
+
     # This must use -r to prevent LD trying to relocate the (unrealistic) file
-    send_log "$LD -r -o ${rootname1}_${rootname2}.o $file1 $file2\n"
-    catch "exec $LD -r -o ${rootname1}_${rootname2}.o $file1 $file2" ld_output
+    send_log "$LD $flags -r -o ${rootname1}_${rootname2}.o $file1 $file2\n"
+    catch "exec $LD $flags -r -o ${rootname1}_${rootname2}.o $file1 $file2" ld_output
     send_log $ld_output
 
     if {[string equal $ld_output ""] == 1} then {
@@ -117,9 +119,11 @@ proc test_arch_error { file1 file2 resul
     set name2 [file tail $file2]
     set rootname2 [file rootname $name2]
 
+    set flags [big_or_little_endian]
+
     # This must use -r to prevent LD trying to relocate the (unrealistic) file
-    send_log "$LD -r -o ${rootname1}_${rootname2}.o $file1 $file2\n"
-    catch "exec $LD -r -o ${rootname1}_${rootname2}.o $file1 $file2" ld_output
+    send_log "$LD $flags -r -o ${rootname1}_${rootname2}.o $file1 $file2\n"
+    catch "exec $LD $flags -r -o ${rootname1}_${rootname2}.o $file1 $file2" ld_output
     send_log $ld_output
 
     if {[string equal $ld_output ""] == 1} then {

Re: [PATCH]: Set little endian flag when linking SH arch tests for little endian multilibs

[Nick Clifton]

Hi Andrew,

> As it happens I was just in the process of preparing that exact same 
> patch (right down to the character - spooky).

[cue X-Files music...]

> I also had an issue with default_ld_assemble in lib/ld-lib.exp. It tries 
> to apply linker endian flags to the assembler. I have attached the patch 
> I was using. Did you not see this issue?

Yes I did.  My patches were only the start, but I ran out of time to 
track down all the other causes of little-endian SH linker test 
failures.  Fortunately it appears that you did not...

> There is also an issue because not all SH architectures have a little 
> endian variant, and therefore not all instructions and relocations are 
> implemented in both endians.

Yes, I suspected this, but again I did not have time to look into it.

> +++ src/ld/testsuite/lib/ld-lib.exp	2006-07-05 14:24:10.000000000 +0100
> @@ -276,6 +276,10 @@ proc default_ld_assemble { as source obj
>      if ![info exists ASFLAGS] { set ASFLAGS "" }
>  
>      set flags [big_or_little_endian]
> +    if {[istarget sh*-*-*] || [istarget pj*-*-*]} {
> +	regsub -- {-EB} $flags {-big} flags
> +	regsub -- {-EL} $flags {-little} flags
> +    }

I do not really like this.  I think that it would be better to avoid 
having target specific knowledge in ld-lib.exp, where possible anyway.

How about having a new board_info variable which, if defined, supplies 
the name of the little- or big- endian switch to use ?  eg (untested):

   proc big_or_little_endian {} {

     if [board_info [target_info name] exists multilib_flags] {
	set tmp_flags " [board_info [target_info name] multilib_flags]"

	foreach x $tmp_flags {
	    case $x in {
		{*big*endian eb EB -eb -EB -mb} {
+                   if [board_info [target_info name] exists 
bigendian_switch] {
+                     set flags [board_info [target_info name] 
bigendian_switch]
+                   } else {
		      set flags " -EB"
+                    }
		    return $flags
		}
		{*little*endian el EL -el -EL -ml} {
+                   if [board_info [target_info name] exists 
littleendian_switch] {
+                     set flags [board_info [target_info name] 
littleendian_switch]
+                   } else {
		      set flags " -EL"
+                   }
		    return $flags
		}
	    }
	}
     }

     set flags ""
     return $flags
   }

Then we can just define bigendian_switch and littleendian_switch in 
sh-hms.exp (or whatever dejagnu board file is being used).

> @@ -117,9 +119,11 @@ proc test_arch_error { file1 file2 resul
>      set name2 [file tail $file2]
>      set rootname2 [file rootname $name2]
>  
> +    set flags [big_or_little_endian]
> +
>      # This must use -r to prevent LD trying to relocate the (unrealistic) file
> -    send_log "$LD -r -o ${rootname1}_${rootname2}.o $file1 $file2\n"
> -    catch "exec $LD -r -o ${rootname1}_${rootname2}.o $file1 $file2" ld_output
> +    send_log "$LD $flags -r -o ${rootname1}_${rootname2}.o $file1 $file2\n"
> +    catch "exec $LD $flags -r -o ${rootname1}_${rootname2}.o $file1 $file2" ld_output
>      send_log $ld_output
>  
>      if {[string equal $ld_output ""] == 1} then {

For reasons I do not understand I found that if I included the above 
part of the patch to arch.exp I ended up with *more* SH linker testsuite 
failures (for little endian multilibs) than without it.  This was 
another thing that I was going to investigate when I had more time.  If 
you can show that it does reduce the number of testsuite failures 
however then I would be happy to accept it.

Cheers
   Nick

Re: [PATCH]: Set little endian flag when linking SH arch tests for little endian multilibs

[Andrew STUBBS]

Nick Clifton wrote:
> How about having a new board_info variable which, if defined, supplies 
> the name of the little- or big- endian switch to use ?  eg (untested):
[snip]
> Then we can just define bigendian_switch and littleendian_switch in 
> sh-hms.exp (or whatever dejagnu board file is being used).

The problem is that the sh linker continues to take -EB/-EL, but the sh 
assembler uses -big/-little. According to my examination of the code the 
pj assembler is the same.

Your solution looks good, but requires separate variables for assembler 
and linker, and then the the big_or_little_endian function needs to know 
which is needed.

BTW, Dejagnu 1.4.4 uses sh-sim, not sh-hms.

>> @@ -117,9 +119,11 @@ proc test_arch_error { file1 file2 resul
>>      set name2 [file tail $file2]
>>      set rootname2 [file rootname $name2]
>>  
>> +    set flags [big_or_little_endian]
>> +
>>      # This must use -r to prevent LD trying to relocate the 
>> (unrealistic) file
>> -    send_log "$LD -r -o ${rootname1}_${rootname2}.o $file1 $file2\n"
>> -    catch "exec $LD -r -o ${rootname1}_${rootname2}.o $file1 $file2" 
>> ld_output
>> +    send_log "$LD $flags -r -o ${rootname1}_${rootname2}.o $file1 
>> $file2\n"
>> +    catch "exec $LD $flags -r -o ${rootname1}_${rootname2}.o $file1 
>> $file2" ld_output
>>      send_log $ld_output
>>  
>>      if {[string equal $ld_output ""] == 1} then {
> 
> For reasons I do not understand I found that if I included the above 
> part of the patch to arch.exp I ended up with *more* SH linker testsuite 
> failures (for little endian multilibs) than without it.  This was 
> another thing that I was going to investigate when I had more time.  If 
> you can show that it does reduce the number of testsuite failures 
> however then I would be happy to accept it.

Hmmm, did you use both parts of the patch? You say that I have had time 
to test this, but actually the reason I had not submitted the patch was 
that I had not yet finished testing it. I suspect that a few more bits 
are needed. I also have a few more adjustments to the arch.exp script to 
submit. Also, this has got put on the back burner for the last few weeks 
so it isn't fresh in my mind any more.

I'll look into it. Not sure when though.

Andrew

Re: [PATCH]: Set little endian flag when linking SH arch tests for little endian multilibs

[Andrew STUBBS]

Andrew Stubbs wrote:
>> For reasons I do not understand I found that if I included the above 
>> part of the patch to arch.exp I ended up with *more* SH linker 
>> testsuite failures (for little endian multilibs) than without it.  
>> This was another thing that I was going to investigate when I had more 
>> time.  If you can show that it does reduce the number of testsuite 
>> failures however then I would be happy to accept it.
> 
> Hmmm, did you use both parts of the patch? You say that I have had time 
> to test this, but actually the reason I had not submitted the patch was 
> that I had not yet finished testing it. I suspect that a few more bits 
> are needed. I also have a few more adjustments to the arch.exp script to 
> submit. Also, this has got put on the back burner for the last few weeks 
> so it isn't fresh in my mind any more.
> 
> I'll look into it. Not sure when though.

Well, I did the tests with and without my two-part patch.

Without:

                 === ld Summary for sh-sim ===

# of expected passes            853
# of unexpected failures        15
# of expected failures          1
# of unresolved testcases       1
# of untested testcases         21

                 === ld Summary for sh-sim/-ml ===

# of expected passes            133
# of unexpected failures        357
# of expected failures          1
# of unresolved testcases       14
# of untested testcases         378

                 === ld Summary ===

# of expected passes            986
# of unexpected failures        372
# of expected failures          2
# of unresolved testcases       15
# of untested testcases         399

With:

                 === ld Summary for sh-sim ===

# of expected passes            853
# of unexpected failures        15
# of expected failures          1
# of unresolved testcases       1
# of untested testcases         21

                 === ld Summary for sh-sim/-ml ===

# of expected passes            797
# of unexpected failures        80
# of expected failures          1
# of untested testcases         21

                 === ld Summary ===

# of expected passes            1650
# of unexpected failures        95
# of expected failures          2
# of unresolved testcases       1
# of untested testcases         42

So that's quite considerably better. I have not looked at what causes 
the 65 additional failures in little endian, and I do not know when I 
will get chance to. I do know that the assembler tests also fail some 
little endian due to a lack of support for it in certain target 
variants. At least I now know why I never got around to submitting the 
patch.

The other testsuite patches I alluded to are, it turns out, related to 
changes in behaviour caused by another patch I have not yet submitted, 
so you don't need to worry about that just yet.

Andrew Stubbs

Re: [PATCH]: Set little endian flag when linking SH arch tests for little endian multilibs

[Nick Clifton]

Hi Andrew,

>>> For reasons I do not understand I found that if I included the above 
>>> part of the patch to arch.exp I ended up with *more* SH linker 
>>> testsuite failures (for little endian multilibs) than without it.  

>> Hmmm, did you use both parts of the patch?

Well I used my version of it, but essentially yes.

> Well, I did the tests with and without my two-part patch.
> 
> Without:
>                 === ld Summary for sh-sim/-ml ===
> 
> # of expected passes            133
> # of unexpected failures        357
> # of expected failures          1
> # of unresolved testcases       14
> # of untested testcases         378

> With:
>                 === ld Summary for sh-sim/-ml ===
> 
> # of expected passes            797
> # of unexpected failures        80
> # of expected failures          1
> # of untested testcases         21

> So that's quite considerably better. I have not looked at what causes 
> the 65 additional failures in little endian, 

This is where I thought that the second part of the patch was giving 
problems.  Without it I get only 33 unexpected failures for the "-ml" 
multilib... I suspect however that this was connected to the fact that 
some architecture variants do not support little endian and the tests do 
not current make allowances for this.

Anyway thanks very much for looking into this, and I am sure that at 
some date somebody will finish off the work and fix the remaining failures.

Cheers
   Nick