Re: [patch] sim/erc32/ max simulation time extended by using 64bit ints

[Joel Sherrill <joel dot sherrill at oarcorp dot com>]

Attached is V4.  Comments interspersed.

--joel

On 04/23/2010 03:28 PM, Doug Evans wrote:
> On Fri, Apr 23, 2010 at 6:25 AM, Tiemen Schut<T.Schut@sron.nl>  wrote:
>    
> > Hey all,
> >
> > This patch solves the problem that the sparc instruction simulator (SIS) would hang after a few minutes of simulation time (time depending on speed of host pc), because of the use of 32 bit counters internally.
> >
> > This patch doesn't change anything to simulator behaviour, except that it allows for longer simulation times.
> >
> > There may be a problem with the use of 64 bit integers, but that was also there before this patch.
> >
> > Thanks,
> >
> > Tiemen Schut
> >      
> Hi.
>
> The patch is ok with me, with a few changes.
> I'd leave it a week to see if anyone else has something to say.
>
>    
I am trying to help address these.
> 1)
>
> -#define	VAL(x)	strtol(x,(char **)NULL,0)
> +#define	VAL(x)	strtoull(x,(char **)NULL,0)
>
> I realize VAL is only used once in interf.c but it's also defined in
> other files as well.
> While one could consolidate them, having the macro at all is probably
> less preferable to just calling strtoul{,l} directly.
> I would just remove it from interf.c and call strtoull directly.
>    
I have fixed this in interf.c.  But didn't touch the other files.
Is this OK?

I can do a second patch after this is merged if you want VAL killed.
> 2)
>
> @@ -338,10 +338,10 @@
>
>   int
>   sim_fetch_register(sd, regno, buf, length)
> -     SIM_DESC sd;
> -    int             regno;
> -    unsigned char  *buf;
> -     int length;
> +    SIM_DESC sd;
> +    int regno;
> +    unsigned char *buf;
> +    int length;
>   {
>       get_regi(&sregs, regno, buf);
>       return -1;
> @@ -349,10 +349,10 @@
>
>   int
>   sim_write(sd, mem, buf, length)
> -     SIM_DESC sd;
> -    SIM_ADDR             mem;
> +    SIM_DESC sd;
> +    SIM_ADDR mem;
>       const unsigned char  *buf;
> -    int             length;
> +    int length;
>   {
>       return (sis_memory_write(mem, buf, length));
>   }
>
> Generally, formatting changes/fixes should be separate.  I noticed a
> few, can you remove them?
>    
The ones you are citing above are my fault.  I used a
very new version of gcc to compile this when checking
his patch and it complained.  I got near the code and
fixed it. Sorry.
> 3)
>
> +#include "stdint.h"
>
> That should be<stdint.h>
>    
Fixed.
> 4)
>
> -    uint32          ildreg;	/* Destination of last load instruction */
> +    uint64          ildreg;	/* Destination of last load instruction */
>
> No point in making this uint64, leave it as uint32.
>
>    
Fixed.
> 5)
>
> +	* sis.c, func.c, sis.h, interf.c: Increase max simulation time
> +	by using uint64 for relevant counters.
>
> I realize sim/erc32/ChangeLog doesn't always follow the GNU
> conventions for ChangeLog entries - it's software obtained from
> elsewhere.  It's ok with me to leave as is, but I defer to someone
> else with an opinion.
>
>    
What specifically are you noticing here?

I looked around and didn't spot anything too offensive.

FWIW Doug I think you have a script which is helping with
your ChangeLog entries and it has (or had) a bug.  Look
at this one from sim/ChangeLog:

2010-02-11  Doug Evans <dje@sebabeach.org>

    * cris/cpuv10.h, * cris/cpuv32.h, * cris/cris-desc.c,
    * cris/cris-desc.h, * cris/decodev10.c, * cris/decodev32.c,
    * cris/modelv10.c, * cris/modelv32.c, * cris/semcrisv10f-switch.c,
    * cris/semcrisv32f-switch.c: Regenerate.

I don't think there should be a ", *" between the files. :-D

My helper script has its own oddities. LOL
> 6) I'm assuming this change has been well tested.
>    
I've used it for RTEMS testing.

--joel
RTEMS

Attachment: sis-V4.diff
Description: Text document