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 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 > 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 * 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