[PATCH][SH] simulator mis-executing pre-decrement

[PATCH][SH] simulator mis-executing pre-decrement

[Andrew STUBBS]

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

Hi,

The attached patch fixes a problem with pre-decrement store instructions 
on the SH simulator.

The problem can be demonstrated with the following program:

#include <stdio.h>

int
main()
{
	int i[2] = {0,0};
	int *p = &i[1];

	asm ("mov.l %0,@-%0\n" : "+r" (p));

	if ((int*)i[0] == &i[1])
	  printf ("PASS: Value Correct.\n");
	else
	  printf ("FAIL: Incorrect value written.\n");

	return 0;
}

The issue is that, when both operands use the same register, the *value* 
to be stored is decremented, but only the *address* of the store should 
have been decremented. I.e. in the example, it is supposed to store "p" 
at "p - 4", but actually stores "p - 4" at "p - 4".

Note that GCC has a similar error which means that the two work together 
(probably not an accident), but the binaries may not work on real 
silicon. I think a patch for this problem will be submitted to GCC 
sometime soon.

:ADDPATCH sh sim:

Unfortunately, I can't test it on the latest GDB because CVS isn't 
accessible (corporate IT issue), but I'm fairly sure it works in the 
sources I do have. For the same reason, I'll have to ask somebody else 
to do the commit for me.

Thanks

Andrew Stubbs

[-- Attachment #2: sim.patch --]
[-- Type: text/plain, Size: 1447 bytes --]

2007-02-22  Andrew Stubbs  <andrew.stubbs@st.com>

	* gencode.c (tab): Correct pre-decrement instructions when m == n.

Index: src/sim/sh/gencode.c
===================================================================
--- src.orig/sim/sh/gencode.c	2005-06-17 04:13:07.000000000 +0100
+++ src/sim/sh/gencode.c	2007-02-23 12:04:35.000000000 +0000
@@ -970,9 +970,11 @@ op tab[] =
     "WBAT (R[n] + R0, R[m]);",
   },
   { "n", "nm", "mov.b <REG_M>,@-<REG_N>", "0010nnnnmmmm0100",
+    /* Allow for the case where m == n.  */
+    "int t = R[m];",
     "MA (1);",
     "R[n] -= 1;",
-    "WBAT (R[n], R[m]);",
+    "WBAT (R[n], t);",
   },
   { "n", "n0", "mov.b R0,@<REG_N>+", "0100nnnn10001011",
     "MA (1);",
@@ -1035,9 +1037,11 @@ op tab[] =
     "WLAT (R0 + R[n], R[m]);",
   },
   { "n", "nm", "mov.l <REG_M>,@-<REG_N>", "0010nnnnmmmm0110",
+    /* Allow for the case where m == n.  */
+    "int t = R[m];",
     "MA (1) ;",
     "R[n] -= 4;",
-    "WLAT (R[n], R[m]);",
+    "WLAT (R[n], t);",
   },
   { "n", "n0", "mov.l R0,@<REG_N>+", "0100nnnn10101011",
     "MA (1) ;",
@@ -1099,9 +1103,11 @@ op tab[] =
     "WWAT (R0 + R[n], R[m]);",
   },
   { "n", "mn", "mov.w <REG_M>,@-<REG_N>", "0010nnnnmmmm0101",
+    /* Allow for the case where m == n.  */
+    "int t = R[m];",
     "MA (1);",
     "R[n] -= 2;",
-    "WWAT (R[n], R[m]);",
+    "WWAT (R[n], t);",
   },
   { "n", "0n", "mov.w R0,@<REG_N>+", "0100nnnn10011011",
     "MA (1);",

Re: [PATCH][SH] simulator mis-executing pre-decrement

[Andrew STUBBS]

Ping. No word on this for a week.

Just in case everyone is waiting for the SH maintainer, there is no 
specific maintainer for this simulator, so a global maintainer will be 
required.

Thanks

Andrew STUBBS wrote:
> Hi,
> 
> The attached patch fixes a problem with pre-decrement store instructions 
> on the SH simulator.
> 
> The problem can be demonstrated with the following program:
> 
> #include <stdio.h>
> 
> int
> main()
> {
>     int i[2] = {0,0};
>     int *p = &i[1];
> 
>     asm ("mov.l %0,@-%0\n" : "+r" (p));
> 
>     if ((int*)i[0] == &i[1])
>       printf ("PASS: Value Correct.\n");
>     else
>       printf ("FAIL: Incorrect value written.\n");
> 
>     return 0;
> }
> 
> The issue is that, when both operands use the same register, the *value* 
> to be stored is decremented, but only the *address* of the store should 
> have been decremented. I.e. in the example, it is supposed to store "p" 
> at "p - 4", but actually stores "p - 4" at "p - 4".
> 
> Note that GCC has a similar error which means that the two work together 
> (probably not an accident), but the binaries may not work on real 
> silicon. I think a patch for this problem will be submitted to GCC 
> sometime soon.
> 
> :ADDPATCH sh sim:
> 
> Unfortunately, I can't test it on the latest GDB because CVS isn't 
> accessible (corporate IT issue), but I'm fairly sure it works in the 
> sources I do have. For the same reason, I'll have to ask somebody else 
> to do the commit for me.
> 
> Thanks
> 
> Andrew Stubbs
> 

Re: [PATCH][SH] simulator mis-executing pre-decrement

[Daniel Jacobowitz]

On Fri, Mar 02, 2007 at 11:43:30AM +0000, Andrew STUBBS wrote:
> Ping. No word on this for a week.
> 
> Just in case everyone is waiting for the SH maintainer, there is no 
> specific maintainer for this simulator, so a global maintainer will be 
> required.

It's OK.  You need me to check it in, right?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH][SH] simulator mis-executing pre-decrement

[Andrew STUBBS]

Daniel Jacobowitz wrote:
> On Fri, Mar 02, 2007 at 11:43:30AM +0000, Andrew STUBBS wrote:
>> Ping. No word on this for a week.
>>
>> Just in case everyone is waiting for the SH maintainer, there is no 
>> specific maintainer for this simulator, so a global maintainer will be 
>> required.
> 
> It's OK.  You need me to check it in, right?

Until our external SSH issue is worked out (the problem is somewhere in 
western Europe), yes please.

Thanks

Andrew

Re: [PATCH][SH] simulator mis-executing pre-decrement

[Daniel Jacobowitz]

On Fri, Mar 02, 2007 at 12:04:19PM +0000, Andrew STUBBS wrote:
> Daniel Jacobowitz wrote:
> >On Fri, Mar 02, 2007 at 11:43:30AM +0000, Andrew STUBBS wrote:
> >>Ping. No word on this for a week.
> >>
> >>Just in case everyone is waiting for the SH maintainer, there is no 
> >>specific maintainer for this simulator, so a global maintainer will be 
> >>required.
> >
> >It's OK.  You need me to check it in, right?
> 
> Until our external SSH issue is worked out (the problem is somewhere in 
> western Europe), yes please.

CHecked in.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH][SH] simulator mis-executing pre-decrement

[Andrew STUBBS]

Daniel Jacobowitz wrote:
> CHecked in.

Thank you very much.