[Qemu-devel] [SPARC] Branch condition problems

[Qemu-devel] [SPARC] Branch condition problems

[Aurelien Jarno]

Hi all,

I have noticed that the branches have some problem on the sparc target
in very rare conditions. This happens when a store double instruction
(std) is used in the delay slot, as in the following test:

  tst  %g0
  bne  9b5d8
  std  %o2, [ %o1 ]

Inserting a nop between bne and std "fixes" the problem.

tst %g0 sets the zero flag, so that the branch should never be taked. It
happens however that it is sometimes taken. This seems to be due to the
fact that T2 holds the result of the condition, and std replace T2 with
another value. flush_T2() is called before altering T2, but it does not
seems to work.

I am currently stuck at that point, I hope somebody who has better 
understanding of the branch code on Sparc could fix that.

Thanks,
Aurelien

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

RE: [Qemu-devel] [SPARC] Branch condition problems

[Blue Swirl]

>I have noticed that the branches have some problem on the sparc target
>in very rare conditions. This happens when a store double instruction
>(std) is used in the delay slot, as in the following test:
>
>   tst  %g0
>   bne  9b5d8
>   std  %o2, [ %o1 ]
>
>Inserting a nop between bne and std "fixes" the problem.
>
>tst %g0 sets the zero flag, so that the branch should never be taked. It
>happens however that it is sometimes taken. This seems to be due to the
>fact that T2 holds the result of the condition, and std replace T2 with
>another value. flush_T2() is called before altering T2, but it does not
>seems to work.
>
>I am currently stuck at that point, I hope somebody who has better
>understanding of the branch code on Sparc could fix that.

Nice analysis, thanks. Flush_T2 is probably a misnomer. One solution could 
be adding a new field to CPU structure for std's (and stda's) use, so that 
T2 does not need to be used.

_________________________________________________________________
Interest Rates near 39yr lows! $430,000 Mortgage for $1,399/mo - Calculate 
new payment 
http://www.lowermybills.com/lre/index.jsp?sourceid=lmb-9632-18466&moid=7581

Re: [Qemu-devel] [SPARC] Branch condition problems

[Aurelien Jarno]

On Wed, Mar 21, 2007 at 07:42:20PM +0100, Blue Swirl wrote:
> >I have noticed that the branches have some problem on the sparc target
> >in very rare conditions. This happens when a store double instruction
> >(std) is used in the delay slot, as in the following test:
> >
> >  tst  %g0
> >  bne  9b5d8
> >  std  %o2, [ %o1 ]
> >
> >Inserting a nop between bne and std "fixes" the problem.
> >
> >tst %g0 sets the zero flag, so that the branch should never be taked. It
> >happens however that it is sometimes taken. This seems to be due to the
> >fact that T2 holds the result of the condition, and std replace T2 with
> >another value. flush_T2() is called before altering T2, but it does not
> >seems to work.
> >
> >I am currently stuck at that point, I hope somebody who has better
> >understanding of the branch code on Sparc could fix that.
> 
> Nice analysis, thanks. Flush_T2 is probably a misnomer. One solution could 

You basically says that flush_T2() should not be used in this case,
right? 

> be adding a new field to CPU structure for std's (and stda's) use, so that 
> T2 does not need to be used.

>From my tests, it seems that std in a delayed branch slot occurs a
hundred of time during a boot, so not a lot. Adding a new field to the
CPU structure would probably decrease the performances (except on
hosts with a lot of registers). Therefore I am proposing something like
that (currently for std only):

--- target-sparc/translate.c.orig	2007-03-21 02:59:49.000000000 +0100
+++ target-sparc/translate.c	2007-03-21 20:23:21.000000000 +0100
@@ -2454,9 +2454,11 @@
 		    gen_op_ldst(sth);
 		    break;
 		case 0x7:
-                    flush_T2(dc);
-		    gen_movl_reg_T2(rd + 1);
-		    gen_op_ldst(std);
+		    gen_op_ldst(st);
+		    gen_op_movl_T1_im(4);
+		    gen_op_add_T1_T0();
+		    gen_movl_reg_T1(rd + 1);
+		    gen_op_ldst(st);
 		    break;
 #if !defined(CONFIG_USER_ONLY) || defined(TARGET_SPARC64)
 		case 0x14:

What do you thing?

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

Re: [Qemu-devel] [SPARC] Branch condition problems

[Blue Swirl]

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

>From my tests, it seems that std in a delayed branch slot occurs a
>hundred of time during a boot, so not a lot. Adding a new field to the
>CPU structure would probably decrease the performances (except on
>hosts with a lot of registers). Therefore I am proposing something like
>that (currently for std only):

Can you test if this works instead?

_________________________________________________________________
Express yourself instantly with MSN Messenger! Download today it's FREE! 
http://messenger.msn.click-url.com/go/onm00200471ave/direct/01/

[-- Attachment #2: std.diff.bz2 --]
[-- Type: application/x-bzip, Size: 546 bytes --]

Re: [Qemu-devel] [SPARC] Branch condition problems

[Aurelien Jarno]

Blue Swirl a écrit :
>>From my tests, it seems that std in a delayed branch slot occurs a
>> hundred of time during a boot, so not a lot. Adding a new field to the
>> CPU structure would probably decrease the performances (except on
>> hosts with a lot of registers). Therefore I am proposing something like
>> that (currently for std only):
> 
> Can you test if this works instead?
> 

The first hunk works well, and is indeed better than my patch. The
second hunk applies, but does not compile.


-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

Re: [Qemu-devel] [SPARC] Branch condition problems

[Aurelien Jarno]

On Wed, Mar 21, 2007 at 09:34:46PM +0100, Aurelien Jarno wrote:
> Blue Swirl a écrit :
> >>From my tests, it seems that std in a delayed branch slot occurs a
> >> hundred of time during a boot, so not a lot. Adding a new field to the
> >> CPU structure would probably decrease the performances (except on
> >> hosts with a lot of registers). Therefore I am proposing something like
> >> that (currently for std only):
> > 
> > Can you test if this works instead?
> > 
> 
> The first hunk works well, and is indeed better than my patch. The
> second hunk applies, but does not compile.
> 

The patch below (that includes the first hunk of your patch) also fixes
stda. There is probably a smarter way to do that for the second hunk,
but that imply more changes.

Index: target-sparc/translate.c
===================================================================
RCS file: /sources/qemu/qemu/target-sparc/translate.c,v
retrieving revision 1.34
diff -u -d -p -r1.34 translate.c
--- target-sparc/translate.c	23 Oct 2006 21:37:34 -0000	1.34
+++ target-sparc/translate.c	21 Mar 2007 21:23:18 -0000
@@ -2454,8 +2454,8 @@ static void disas_sparc_insn(DisasContex
 		    gen_op_ldst(sth);
 		    break;
 		case 0x7:
-                    flush_T2(dc);
-		    gen_movl_reg_T2(rd + 1);
+                    gen_op_ldst(st);
+                    gen_movl_reg_T1(rd + 1);
 		    gen_op_ldst(std);
 		    break;
 #if !defined(CONFIG_USER_ONLY) || defined(TARGET_SPARC64)
@@ -2485,9 +2485,11 @@ static void disas_sparc_insn(DisasContex
 		    if (!supervisor(dc))
 			goto priv_insn;
 #endif
-                    flush_T2(dc);
-		    gen_movl_reg_T2(rd + 1);
-		    gen_op_stda(insn, 0, 8, 0);
+		    gen_op_stda(insn, 0, 4, 0);
+		    gen_op_movl_T1_im(4);
+		    gen_op_add_T1_T0();
+		    gen_movl_reg_T1(rd + 1);
+		    gen_op_stda(insn, 0, 4, 0);
                     break;
 #endif
 #ifdef TARGET_SPARC64

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

Re: [Qemu-devel] [SPARC] Branch condition problems

[Blue Swirl]

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

Hi,

I can't reproduce the problem with attached test program. Also the generated 
ops look just fine:

IN:
0x00010074:  sethi  %hi(0x20000), %g3
0x00010078:  or  %g3, 0xa0, %g3 ! 0x200a0
0x0001007c:  mov  -1, %g1
0x00010080:  mov  -1, %g2
0x00010084:  tst  %g0
0x00010088:  bne  0x10094
0x0001008c:  std  %g1, [ %g3 ]

OP:
0x0000: movl_T0_im 0x20000
0x0001: movl_g3_T0
0x0002: movl_T0_g3
0x0003: movl_T1_sim 0xa0
0x0004: or_T1_T0
0x0005: movl_g3_T0
0x0006: movl_T1_sim 0xffffffff
0x0007: movl_g1_T1
0x0008: movl_T1_sim 0xffffffff
0x0009: movl_g2_T1
0x000a: movl_T0_im 0x0
0x000b: movl_T1_im 0x0
0x000c: or_T1_T0
0x000d: logic_T0_cc
Set T2:
0x000e: eval_bne
First part of std:
0x000f: movl_T0_g3
0x0010: movl_T1_g1
flush_T2 here:
0x0011: jz_T2_label 0x0
0x0012: movl_npc_im 0x10094
0x0013: jmp_label 0x1
0x0014: movl_npc_im 0x10090
Now reuse T2, rest of std:
0x0015: movl_T2_g2
0x0016: std_raw
0x0017: next_insn
0x0018: movl_T0_0
0x0019: exit_tb
0x001a: end

--------------
IN:
Branch not taken (OK)
0x00010090:  nop
0x00010094:  mov  1, %g1        ! 0x1
0x00010098:  ta  0x10

OP:
0x0000: movl_T1_sim 0x1
0x0001: movl_g1_T1
0x0002: movl_T0_im 0x0
0x0003: movl_T1_sim 0x10
0x0004: add_T1_T0
0x0005: jmp_im 0x10098
0x0006: movl_npc_im 0x1009c
0x0007: trap_T0
0x0008: next_insn
0x0009: movl_T0_0
0x000a: exit_tb
0x000b: end

Can you describe how to reproduce the bug?

_________________________________________________________________
Express yourself instantly with MSN Messenger! Download today it's FREE! 
http://messenger.msn.click-url.com/go/onm00200471ave/direct/01/

[-- Attachment #2: br_test.S --]
[-- Type: application/octet-stream, Size: 290 bytes --]

.globl _start
_start:
        set     _loc, %g3
        set     -1, %g1
        set     -1, %g2
        tst     %g0
/*
        ba,a    1f
currpc: .skip 3952
1:
*/
        bne     2f
         std     %g1, [%g3]
        nop
2:      mov     1, %g1
        ta      0x10
.data
_loc:   .word 0,0

Re: [Qemu-devel] [SPARC] Branch condition problems

[Aurelien Jarno]

Blue Swirl a écrit :
> Hi,
> 
> I can't reproduce the problem with attached test program. Also the generated 
> ops look just fine:
> 

[snip]

> 
> Can you describe how to reproduce the bug?
> 

Well I also have difficulties to reproduce it with a simple program. I
have detected it while trying to build binutils on a Debian unstable
distribution running in QEMU.

The problem occurs when running the configure script of libiberty as a
normal user using fakeroot, using bash as /bin/sh. The last character of
some of the $LINENO variables is wrongly computed. It is computed using
the modulo function from gcc (moddi3) called from fmtumax() called from
itos(). In some very rare conditions (I don't know all of them) a branch
is falsely taken which result in a wrong sign. When added to 0x30, it
results in a wrong character. Some of them like ) or ( are triggering an
error message. I have been able to track the bug up to the branch
instruction followed by a std instruction by directly editing the
assembly code of bash to cancel some tests.

Note that you need to use fakeroot to see the problem, and also that
using gdb workaround the bug. Rebuilding bash with -O0 or -O1
workarounds the problem. Using -g helps to get the function names along
with the assembly code when using objdump

To reproduce it, the best way is to use a Debian unstable distribution
running in QEMU, at run (I can provide an image if you need one):

  apt-get install binutils
  cd binutils-2.17-3/libiberty
  fakeroot ./configure

You will see that some tests fail with a strange error message from
bash. This error is fixed by the patch I posted or by the one you posted.

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net