Minor fix for H8 simulator

Minor fix for H8 simulator

[Jeff Law]

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


I haven't regularly contributed to gdb/sim for a long time. So please be 
gentle if I goof on process stuff.

The upstream GCC tester has  showed spurious execution failures on the 
H8 target for the H8/SX multilibs. I suspected memory corruption or an 
uninitialized variable early as the same binary would sometimes work and 
sometimes it got the wrong result. Worse yet, the point where the test 
determined it was getting the wrong result would change.

Because it only happened on the H8/SX variant I was able to zero in on 
the "mova" support and the "short form" of those instructions in particular.

As the code stands it checks if code->op3.type == 0 to try and identify 
cases where op3 wasn't filled in and thus we've got the short form of 
the mova instruction.

But for the short-form of those instructions we never set any of the 
"op3" data structure. We get whatever was lying around -- it's usually 
zero and thus things usually work, but if the stale data was nonzero, 
then we'd fail to recognize the instruction as a short-form and fail to 
set up the various fields appropriately.

I initially initialized the op3.type field to zero, but didn't like that 
because it was inconsistent with how other operands were initialized. 
Bringing consistency meant using -1 as the initializer value and 
adjusting the check for short form mova appropriately.

I've had this in the upstream GCC tester for perhaps a year at this 
point and haven't seen any of the intermittent failures again.


OK to push to master (assuming I even still have permissions to do so)?

Thanks,
Jeff

[-- Attachment #2: 0006-h8simfix.patch --]
[-- Type: text/plain, Size: 753 bytes --]

	* h8300/compile.c (decode): Initialize op3.type to -1.
	(step_once): Adjust test of op3.type.

diff --git a/sim/h8300/compile.c b/sim/h8300/compile.c
index c1c61d8211..af9137d6d6 100644
--- a/sim/h8300/compile.c
+++ b/sim/h8300/compile.c
@@ -568,6 +568,7 @@ decode (SIM_DESC sd, int addr, unsigned char *data, decoded_inst *dst)
 
   dst->dst.type = -1;
   dst->src.type = -1;
+  dst->op3.type = -1;
 
   /* Find the exact opcode/arg combo.  */
   for (q = h8_opcodes; q->name; q++)
@@ -1940,7 +1941,7 @@ step_once (SIM_DESC sd, SIM_CPU *cpu)
 		of the same register.
 	  */
 
-	  if (code->op3.type == 0)
+	  if (code->op3.type == -1)
 	    {
 	      /* Short form: src == INDEXB/INDEXW, dst == op3 == 0.
 		 We get to compose dst and op3 as follows:

Re: Minor fix for H8 simulator

[Mike Frysinger]

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

On 11 Nov 2021 09:50, Jeff Law via Gdb-patches wrote:
> The upstream GCC tester has  showed spurious execution failures on the 
> H8 target for the H8/SX multilibs. I suspected memory corruption or an 
> uninitialized variable early as the same binary would sometimes work and 
> sometimes it got the wrong result. Worse yet, the point where the test 
> determined it was getting the wrong result would change.
> 
> Because it only happened on the H8/SX variant I was able to zero in on 
> the "mova" support and the "short form" of those instructions in particular.
> 
> As the code stands it checks if code->op3.type == 0 to try and identify 
> cases where op3 wasn't filled in and thus we've got the short form of 
> the mova instruction.
> 
> But for the short-form of those instructions we never set any of the 
> "op3" data structure. We get whatever was lying around -- it's usually 
> zero and thus things usually work, but if the stale data was nonzero, 
> then we'd fail to recognize the instruction as a short-form and fail to 
> set up the various fields appropriately.
> 
> I initially initialized the op3.type field to zero, but didn't like that 
> because it was inconsistent with how other operands were initialized. 
> Bringing consistency meant using -1 as the initializer value and 
> adjusting the check for short form mova appropriately.
> 
> I've had this in the upstream GCC tester for perhaps a year at this 
> point and haven't seen any of the intermittent failures again.

can you update the unittests too ?

and while we have your eyes on H8, can you perhaps peek at the open reports ?
https://sourceware.org/bugzilla/buglist.cgi?bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&component=sim&&short_desc=h8300&short_desc_type=allwordssubstr
-mike

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Minor fix for H8 simulator

[Jeff Law]

On 11/11/2021 10:55 AM, Mike Frysinger wrote:
> On 11 Nov 2021 09:50, Jeff Law via Gdb-patches wrote:
>> The upstream GCC tester has  showed spurious execution failures on the
>> H8 target for the H8/SX multilibs. I suspected memory corruption or an
>> uninitialized variable early as the same binary would sometimes work and
>> sometimes it got the wrong result. Worse yet, the point where the test
>> determined it was getting the wrong result would change.
>>
>> Because it only happened on the H8/SX variant I was able to zero in on
>> the "mova" support and the "short form" of those instructions in particular.
>>
>> As the code stands it checks if code->op3.type == 0 to try and identify
>> cases where op3 wasn't filled in and thus we've got the short form of
>> the mova instruction.
>>
>> But for the short-form of those instructions we never set any of the
>> "op3" data structure. We get whatever was lying around -- it's usually
>> zero and thus things usually work, but if the stale data was nonzero,
>> then we'd fail to recognize the instruction as a short-form and fail to
>> set up the various fields appropriately.
>>
>> I initially initialized the op3.type field to zero, but didn't like that
>> because it was inconsistent with how other operands were initialized.
>> Bringing consistency meant using -1 as the initializer value and
>> adjusting the check for short form mova appropriately.
>>
>> I've had this in the upstream GCC tester for perhaps a year at this
>> point and haven't seen any of the intermittent failures again.
> can you update the unittests too ?
IIRC it was dependent upon what instructions had recently executed as 
those impacted the state of code->op3.type -- my analysis is from over a 
year ago and had been sitting in my todo box for a while.  Even just 
reproducing was fairly painful.

>
> and while we have your eyes on H8, can you perhaps peek at the open reports ?
> https://sourceware.org/bugzilla/buglist.cgi?bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&component=sim&&short_desc=h8300&short_desc_type=allwordssubstr
I can take a look at them as I'm familiar with the H8/S and earlier 
ISAs.  I've got virtually no experience with the SX variants.

Jeff

Re: Minor fix for H8 simulator

[Mike Frysinger]

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

On 11 Nov 2021 11:06, Jeff Law via Gdb-patches wrote:
> On 11/11/2021 10:55 AM, Mike Frysinger wrote:
> > On 11 Nov 2021 09:50, Jeff Law via Gdb-patches wrote:
> >> The upstream GCC tester has  showed spurious execution failures on the
> >> H8 target for the H8/SX multilibs. I suspected memory corruption or an
> >> uninitialized variable early as the same binary would sometimes work and
> >> sometimes it got the wrong result. Worse yet, the point where the test
> >> determined it was getting the wrong result would change.
> >>
> >> Because it only happened on the H8/SX variant I was able to zero in on
> >> the "mova" support and the "short form" of those instructions in particular.
> >>
> >> As the code stands it checks if code->op3.type == 0 to try and identify
> >> cases where op3 wasn't filled in and thus we've got the short form of
> >> the mova instruction.
> >>
> >> But for the short-form of those instructions we never set any of the
> >> "op3" data structure. We get whatever was lying around -- it's usually
> >> zero and thus things usually work, but if the stale data was nonzero,
> >> then we'd fail to recognize the instruction as a short-form and fail to
> >> set up the various fields appropriately.
> >>
> >> I initially initialized the op3.type field to zero, but didn't like that
> >> because it was inconsistent with how other operands were initialized.
> >> Bringing consistency meant using -1 as the initializer value and
> >> adjusting the check for short form mova appropriately.
> >>
> >> I've had this in the upstream GCC tester for perhaps a year at this
> >> point and haven't seen any of the intermittent failures again.
> >
> > can you update the unittests too ?
>
> IIRC it was dependent upon what instructions had recently executed as 
> those impacted the state of code->op3.type -- my analysis is from over a 
> year ago and had been sitting in my todo box for a while.  Even just 
> reproducing was fairly painful.

if reconstructing the failure is too hard now, and you think the existing
unittests at least cover this insn in this mode at some level, then OK.
feel free to merge.
-mike

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]