From: Joel Sherrill <joel.sherrill@oarcorp.com>
To: Doug Evans <dje@google.com>
Cc: Tiemen Schut <T.Schut@sron.nl>,
"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: Re: [patch] sim/erc32/ max simulation time extended by using 64bit ints
Date: Tue, 04 May 2010 21:16:00 -0000 [thread overview]
Message-ID: <4BE08E95.5040500@oarcorp.com> (raw)
In-Reply-To: <m2ze394668d1004231328h55952dfpdea88e2fd17b4c2d@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3691 bytes --]
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 #2: sis-V4.diff --]
[-- Type: text/plain, Size: 10187 bytes --]
Index: sim/erc32/ChangeLog
===================================================================
RCS file: /cvs/src/src/sim/erc32/ChangeLog,v
retrieving revision 1.31
diff -u -r1.31 ChangeLog
--- sim/erc32/ChangeLog 14 Apr 2010 07:38:04 -0000 1.31
+++ sim/erc32/ChangeLog 4 May 2010 21:14:55 -0000
@@ -1,3 +1,9 @@
+2010-04-20 Tiemen Schut <T.Schut@sron.nl>
+
+ * erc32.c, sis.c, func.c, sis.h, interf.c: Increase max simulation
+ time by using uint64 for relevant counters. Change prototype
+ of sis_memory_write() to const unsigned char *.
+
2010-04-14 Mike Frysinger <vapier@gentoo.org>
* interp.c (sim_write): Add const to buf arg.
Index: sim/erc32/erc32.c
===================================================================
RCS file: /cvs/src/src/sim/erc32/erc32.c,v
retrieving revision 1.2
diff -u -r1.2 erc32.c
--- sim/erc32/erc32.c 11 Nov 2008 22:20:54 -0000 1.2
+++ sim/erc32/erc32.c 4 May 2010 21:14:55 -0000
@@ -1860,9 +1860,9 @@
int
sis_memory_write(addr, data, length)
- uint32 addr;
- char *data;
- uint32 length;
+ uint32 addr;
+ const unsigned char *data;
+ uint32 length;
{
char *mem;
Index: sim/erc32/func.c
===================================================================
RCS file: /cvs/src/src/sim/erc32/func.c,v
retrieving revision 1.4
diff -u -r1.4 func.c
--- sim/erc32/func.c 8 Jul 2005 08:04:54 -0000 1.4
+++ sim/erc32/func.c 4 May 2010 21:14:55 -0000
@@ -421,7 +421,7 @@
}
} else if (strncmp(cmd1, "cont", clen) == 0) {
if ((cmd1 = strtok(NULL, " \t\n\r")) == NULL) {
- stat = run_sim(sregs, -1, 0);
+ stat = run_sim(sregs, UINT64_MAX, 0);
} else {
stat = run_sim(sregs, VAL(cmd1), 0);
}
@@ -472,7 +472,7 @@
if ((cmd2 = strtok(NULL, " \t\n\r")) != NULL) {
stat = run_sim(sregs, VAL(cmd2), 0);
} else {
- stat = run_sim(sregs, -1, 0);
+ stat = run_sim(sregs, UINT64_MAX, 0);
}
daddr = sregs->pc;
sim_halt();
@@ -544,7 +544,7 @@
reset_all();
reset_stat(sregs);
if ((cmd1 = strtok(NULL, " \t\n\r")) == NULL) {
- stat = run_sim(sregs, -1, 0);
+ stat = run_sim(sregs, UINT64_MAX, 0);
} else {
stat = run_sim(sregs, VAL(cmd1), 0);
}
@@ -560,7 +560,7 @@
sim_halt();
} else if (strncmp(cmd1, "tcont", clen) == 0) {
sregs->tlimit = limcalc(sregs->freq);
- stat = run_sim(sregs, -1, 0);
+ stat = run_sim(sregs, UINT64_MAX, 0);
daddr = sregs->pc;
sim_halt();
} else if (strncmp(cmd1, "tgo", clen) == 0) {
@@ -573,7 +573,7 @@
sregs->pc = len & ~3;
sregs->npc = sregs->pc + 4;
printf("resuming at 0x%08x\n",sregs->pc);
- stat = run_sim(sregs, -1, 0);
+ stat = run_sim(sregs, UINT64_MAX, 0);
daddr = sregs->pc;
sim_halt();
} else if (strncmp(cmd1, "tlimit", clen) == 0) {
@@ -583,7 +583,7 @@
sregs->tlimit / sregs->freq / 1000);
} else if (strncmp(cmd1, "tra", clen) == 0) {
if ((cmd1 = strtok(NULL, " \t\n\r")) == NULL) {
- stat = run_sim(sregs, -1, 1);
+ stat = run_sim(sregs, UINT64_MAX, 1);
} else {
stat = run_sim(sregs, VAL(cmd1), 1);
}
@@ -595,7 +595,7 @@
reset_all();
reset_stat(sregs);
sregs->tlimit = limcalc(sregs->freq);
- stat = run_sim(sregs, -1, 0);
+ stat = run_sim(sregs, UINT64_MAX, 0);
daddr = sregs->pc;
sim_halt();
} else
@@ -833,7 +833,7 @@
event(cfunc, arg, delta)
void (*cfunc) ();
int32 arg;
- uint32 delta;
+ uint64 delta;
{
struct evcell *ev1, *evins;
@@ -900,7 +900,8 @@
struct evcell *evrem;
void (*cfunc) ();
- uint32 arg, endtime;
+ uint32 arg;
+ uint64 endtime;
#ifdef STAT
sregs->fholdt += sregs->fhold;
@@ -938,7 +939,8 @@
{
struct evcell *evrem;
void (*cfunc) ();
- int32 arg, endtime;
+ int32 arg;
+ uint64 endtime;
if (ebase.eq.nxt == NULL)
printf("Warning: event queue empty - power-down mode not entered\n");
Index: sim/erc32/interf.c
===================================================================
RCS file: /cvs/src/src/sim/erc32/interf.c,v
retrieving revision 1.7
diff -u -r1.7 interf.c
--- sim/erc32/interf.c 14 Apr 2010 07:38:04 -0000 1.7
+++ sim/erc32/interf.c 4 May 2010 21:14:55 -0000
@@ -37,8 +37,6 @@
#define PSR_CWP 0x7
-#define VAL(x) strtol(x,(char **)NULL,0)
-
extern struct disassemble_info dinfo;
extern struct pstate sregs;
extern struct estate ebase;
@@ -69,7 +67,7 @@
int
run_sim(sregs, icount, dis)
struct pstate *sregs;
- unsigned int icount;
+ uint64 icount;
int dis;
{
int mexc, irq;
@@ -234,7 +232,7 @@
} else
if (strcmp(argv[stat], "-freq") == 0) {
if ((stat + 1) < argc) {
- freq = VAL(argv[++stat]);
+ freq = strtol(argv[++stat], (char **)NULL, 0);
}
} else {
(*sim_callback->printf_filtered) (sim_callback,
@@ -338,10 +336,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 +347,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));
}
@@ -461,7 +459,7 @@
void
sim_resume(SIM_DESC sd, int step, int siggnal)
{
- simstat = run_sim(&sregs, -1, 0);
+ simstat = run_sim(&sregs, UINT64_MAX, 0);
if (sis_gdb_break) flush_windows ();
}
Index: sim/erc32/sis.c
===================================================================
RCS file: /cvs/src/src/sim/erc32/sis.c,v
retrieving revision 1.3
diff -u -r1.3 sis.c
--- sim/erc32/sis.c 9 Jun 2008 22:55:27 -0000 1.3
+++ sim/erc32/sis.c 4 May 2010 21:14:55 -0000
@@ -82,7 +82,7 @@
int
run_sim(sregs, icount, dis)
struct pstate *sregs;
- unsigned int icount;
+ uint64 icount;
int dis;
{
int irq, mexc, deb, asi;
Index: sim/erc32/sis.h
===================================================================
RCS file: /cvs/src/src/sim/erc32/sis.h,v
retrieving revision 1.2
diff -u -r1.2 sis.h
--- sim/erc32/sis.h 9 Jun 2002 15:45:46 -0000 1.2
+++ sim/erc32/sis.h 4 May 2010 21:14:55 -0000
@@ -23,6 +23,7 @@
#include "ansidecl.h"
#include "gdb/callback.h"
#include "gdb/remote-sim.h"
+#include <stdint.h>
#include "end.h"
@@ -52,8 +53,8 @@
typedef double float64; /* 64-bit float */
/* FIXME: what about host compilers that don't support 64-bit ints? */
-typedef unsigned long long uint64; /* 64-bit unsigned int */
-typedef long long int64; /* 64-bit signed int */
+typedef uint64_t uint64; /* 64-bit unsigned int */
+typedef int64_t int64; /* 64-bit signed int */
struct pstate {
@@ -108,22 +109,22 @@
float32 freq; /* Simulated processor frequency */
- uint32 tottime;
- uint32 ninst;
- uint32 fholdt;
- uint32 holdt;
- uint32 icntt;
- uint32 finst;
- uint32 simstart;
- uint32 starttime;
- uint32 tlimit; /* Simulation time limit */
- uint32 pwdtime; /* Cycles in power-down mode */
- uint32 nstore; /* Number of load instructions */
- uint32 nload; /* Number of store instructions */
- uint32 nannul; /* Number of annuled instructions */
- uint32 nbranch; /* Number of branch instructions */
+ uint64 tottime;
+ uint64 ninst;
+ uint64 fholdt;
+ uint64 holdt;
+ uint64 icntt;
+ uint64 finst;
+ uint64 simstart;
+ uint64 starttime;
+ uint64 tlimit; /* Simulation time limit */
+ uint64 pwdtime; /* Cycles in power-down mode */
+ uint64 nstore; /* Number of load instructions */
+ uint64 nload; /* Number of store instructions */
+ uint64 nannul; /* Number of annuled instructions */
+ uint64 nbranch; /* Number of branch instructions */
uint32 ildreg; /* Destination of last load instruction */
- uint32 ildtime; /* Last time point for load dependency */
+ uint64 ildtime; /* Last time point for load dependency */
int rett_err; /* IU in jmpl/restore error state (Rev.0) */
int jmpltime;
@@ -132,14 +133,14 @@
struct evcell {
void (*cfunc) ();
int32 arg;
- uint32 time;
+ uint64 time;
struct evcell *nxt;
};
struct estate {
struct evcell eq;
struct evcell *freeq;
- uint32 simtime;
+ uint64 simtime;
};
struct irqcell {
@@ -168,8 +169,8 @@
int32 sz, int32 *ws));
extern int memory_write PARAMS ((int32 asi, uint32 addr, uint32 *data,
int32 sz, int32 *ws));
-extern int sis_memory_write PARAMS ((uint32 addr, char *data,
- uint32 length));
+extern int sis_memory_write PARAMS ((uint32 addr,
+ const unsigned char *data, uint32 length));
extern int sis_memory_read PARAMS ((uint32 addr, char *data,
uint32 length));
@@ -186,7 +187,7 @@
struct disassemble_info;
extern void dis_mem PARAMS ((uint32 addr, uint32 len,
struct disassemble_info *info));
-extern void event PARAMS ((void (*cfunc) (), int32 arg, uint32 delta));
+extern void event PARAMS ((void (*cfunc) (), int32 arg, uint64 delta));
extern void set_int PARAMS ((int32 level, void (*callback) (), int32 arg));
extern void advance_time PARAMS ((struct pstate *sregs));
extern uint32 now PARAMS ((void));
@@ -205,7 +206,7 @@
/* interf.c */
extern int run_sim PARAMS ((struct pstate *sregs,
- unsigned int icount, int dis));
+ uint64 icount, int dis));
/* float.c */
extern int get_accex PARAMS ((void));
next prev parent reply other threads:[~2010-05-04 21:16 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-04-23 13:25 Tiemen Schut
2010-04-23 20:28 ` Doug Evans
2010-05-04 21:16 ` Joel Sherrill [this message]
2010-05-07 17:48 ` Doug Evans
2010-05-17 2:08 ` Joel Brobecker
2010-05-17 3:23 ` Joel Sherrill
2010-05-17 16:37 ` Doug Evans
2010-05-17 16:59 ` Joel Brobecker
2010-05-20 23:13 ` Joel Brobecker
2010-05-21 13:47 ` Joel Sherrill
2010-05-21 15:06 ` Joel Brobecker
2010-05-21 15:15 ` Joel Sherrill
2010-05-21 17:13 ` Doug Evans
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4BE08E95.5040500@oarcorp.com \
--to=joel.sherrill@oarcorp.com \
--cc=T.Schut@sron.nl \
--cc=dje@google.com \
--cc=gdb-patches@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox