new sim: Visium

new sim: Visium

[Joel Brobecker]

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

Hi Mike,

Attached is a commit which adds a simulator for the visium architecture.
The code was originally written by a customer of ours, and although
we reworked it very extensively, there are still major chunks which
we did not touch.  This can explain some of the idiosyncrasies in
the implementation, such as the "check" macro, for instance. I will
also admit that some of the code could have been written more elegantly
(IMO), but I didn't feel the need to rewrite it, since, in practice,
the code is stable and rewriting would not have any user-visible
benefit.

What's still left to do on our end is create a testsuite, which
we haven't gotten around to doing yet. But we will! :-)

Any comments on this port, while we work on the testsuite?

sim/ChangeLog:

        * configure.tgt: Add visium target handling.
        * configure: Regenerate.

sim/visium/ChangeLog:

        * Makefile.in, aclocal.m4, config.in, configure, configure.ac,
        sim-fpu.c, sim-fpu.h, sim-main.h, sim_calls.c, visium-config.c,
        visium-config.h, visium-core.c, visium-core.h, visium-defs.h,
        visium-dev-ancillary.c, visium-dev-ancillary.h,
        visium-dev-cmdline.c, visium-dev-cmdline.h, visium-dev-fpu.c,
        visium-dev-fpu.h, visium-dev-ram.c, visium-dev-ram.h,
        visium-dev-rom.c, visium-dev-rom.h, visium-dev-syscall.c,
        visium-dev-syscall.h, visium-dev.c, visium-dev.h,
        visium-processor.c, visium-processor.h, visium-program.c,
        visium-program.h, visium-ranges.c, visium-ranges.h,
        visium-storage.c, visium-storage.h, visium-trace.c,
        visium-trace.h: New file.

Thanks!
-- 
Joel

[-- Attachment #2: 0001-Add-Visium-simulator-support.patch.xz --]
[-- Type: application/x-xz, Size: 138272 bytes --]

Re: new sim: Visium

[Mike Frysinger]

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

On 06 Jan 2016 08:47, Joel Brobecker wrote:
> What's still left to do on our end is create a testsuite, which
> we haven't gotten around to doing yet. But we will! :-)

yeah, i don't think i want to allow any more ports w/out tests.
it makes it too hard for me to adjust code w/out breaking things.

> +++ b/sim/visium/configure.ac
>
> +dnl This file is part of GDB.

i think all these lines should be changed:
dnl This file is part of the GNU Simulators.

comes up in a bunch of files in this patch

> +SIM_AC_COMMON

please add at least:
SIM_AC_OPTION_WARNINGS

and then fix all the warnings :)

> +# BFD conditionally uses zlib, so we must link it in if libbfd does, by
> +# using the same condition.
> +AM_ZLIB
> +
> +# BFD uses libdl when when plugins enabled.
> +AC_PLUGINS

you should not need either of these.  they should be handled for you
already.

> +++ b/sim/visium/sim-fpu.c
>
> +/* Note: Although the origin of this file has not been researched,
> +   we know this is not the master copy of this code, and therefore
> +   we try to do as few modifications as possible, in order to facilitate
> +   possible coordination with that original, if it is every found.
> +   This explains why no apparent effort is made to improve this file's
> +   style to better match our usual standards.  */

erm, the origin is pretty clear -- it was duplicated from sim/common/sim-fpu.c.
this needs to be rectified.

> +++ b/sim/visium/sim-main.h
>
> +  int32_t regs[VISIUM_GENERAL_REGS];

do you want all the regs to be signed ?  usually ports have them be
unsigned as it makes coding simpler.

> +/* A small macro to return the sim_cpu from the sim descriptor.
> +   We only support one CPU at the moment, so the CPU index is
> +   always 0.  But perhaps we'll need to support SMP on this architecture,
> +   one day, in which case this macro will be useful to help supporting
> +   that (easy to find all locations, and perhaps CPU selection could
> +   be automated inside this macro itself).  */
> +#define VISIUM_STATE_CPU(sd) (STATE_CPU (sd, 0))

usually you shouldn't need this.  if you have a reference to the state
but not a cpu, it tends to indicate the API isn't correctly passing down
the cpu as an argument.  so those funcs should be adjusted instead.

> +++ b/sim/visium/sim_calls.c
>
> +sim_open (SIM_OPEN_KIND kind, struct host_callback_struct *callback,
> +	  struct bfd *abfd, char **argv)

looks like this forgets to call:
	sim_config
	sim_post_argv_init

> +void
> +sim_close (SIM_DESC sd, int quitting)
> +{
> +  sim_cpu *cpu = VISIUM_STATE_CPU (sd);
> +
> +  visium_core_destroy (sd);
> +  visium_config_delete (sd);
> +
> +  if (sd->trace_data != NULL)
> +    visium_trace_close (sd);
> +}

you should define SIM_CLOSE_HOOK in sim-main.h instead and use
the common sim-close.c file.  then delete this.

> +sim_load (SIM_DESC sd, const char *prog, bfd *abfd, int from_tty)
> +sim_read (SIM_DESC sd, SIM_ADDR mem, unsigned char *buf, int length)
> +sim_write (SIM_DESC sd, SIM_ADDR mem, const unsigned char *buf, int length)

this looks hairy and will require a good bit of unwinding.  you shouldn't
be defining your own sim_read/sim_write anymore.  if you want memory, you
should be using the common memory functions to attach it.  if you want to
simulate hardware, you should be using the WITH_HW framework.

> +sim_fetch_register (SIM_DESC sd, int regno, unsigned char *buf, int length)
> +sim_store_register (SIM_DESC sd, int regno, unsigned char *buf, int length)

don't define these yourself.  see commit e1211e55062594679697d2175b7ea77d
as an example of converting to the new callbacks.

> +sim_stop_reason (SIM_DESC sd, enum sim_stop *reason, int *sigrc)
> +sim_stop (SIM_DESC sd)
> +sim_resume (SIM_DESC sd, int step, int siggnal)

you should be using the common core for these.  instead, define
sim_engine_run (see bfin/interp.c as a simple example), and instead
of setting sd->stop_reason, call sim_engine_halt to halt simulation.
that call will not return as it uses setjmp/longjmp.  it tends to
make design of your sim simpler.

> +++ b/sim/visium/visium-defs.h
>
> +/* Display an error message.  */
> +#define err(msg, args ...) \
> +  do { \
> +    fprintf (stderr, "[ERROR] (%s:%d: errno: %s) " msg "\n", \
> +	     __FILE__, __LINE__, visium_get_errno_msg (), ## args); \
> +  } while (0)

you should be using sim_io_eprintf instead of fprintf

> +#define info(msg, args ...) \
> +  do { \
> +    fprintf (stdout, "[INFO] (%s:%d) " msg "\n", __FILE__, __LINE__, \
> +	     ## args); \
> +  } while (0)
> +
> +/* Display a message.  */
> +#define msg(msg, args ...) \
> +  do { \
> +    fprintf (stdout, msg "\n", ## args); \
> +    fflush (stdout); \
> +  } while (0)

these should use sim_io_printf instead of fprintf

> +/* All as "checks" except not requesting the simulation to stop.  */
> +#define check(val, msg, args ...)            \
> +  do { \
> +    if (!(val)) \
> +      { \
> +	err (msg, ## args); \
> +	errno = 0; \
> +	goto error; \
> +      } \
> +  } while (0)
> +
> +/* Check that Addr is not NULL.  */
> +#define check_memory(addr) check (addr, "Out of memory")
> +
> +/* Unconditionally display an error message and jump to an error
> +   handler.  */
> +#define fatal(msg, args ...) \
> +  do { \
> +    err (msg, ## args); \
> +    errno = 0; \
> +    goto error; \
> +  } while (0)

we have sim_io_error for throwing an error and exiting

> --- /dev/null
> +++ b/sim/visium/visium-dev-syscall.c
>
> +/* Handle a subset of system calls via a modified newlib C library.
> +   List of supported functions:
> +    - open()
> +    - close()
> +    - read()
> +    - write()
> +    - lseek()
> +    - rename()
> +    - unlink()
> +    - stat()
> +    - fstat()
> +    - gettimeofday()
> +    - isatty()
> +    - system()
> + */

you shouldn't have to implement all this yourself.  we have sim-syscall.c
to deal with these.  all your sim needs to do is handle the trap and then
unpack all the regs before passing them to sim_syscall().

> +++ b/sim/visium/visium-ranges.c
> +++ b/sim/visium/visium-ranges.h

glancing at the API, i think the common sim-arange module provides the
same logic already.

> +++ b/sim/visium/visium-trace.c
> +++ b/sim/visium/visium-trace.h

i glanced through the trace logic ... it doesn't seem like it's hardware
specific (like you've got a hardware module that is handling this).  since
it's all software based, you should throw away the visium trace logic and
switch to the common sim-trace module.  the sim-trace.h header includes a
lot of macros to quickly instrument your code.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: new sim: Visium

[Joel Brobecker]

Hi Mike,

Thanks for the quick review. A few questions below, before
I get cracking on it again...

> > +++ b/sim/visium/configure.ac
> >
> > +dnl This file is part of GDB.
> 
> i think all these lines should be changed:
> dnl This file is part of the GNU Simulators.
> 
> comes up in a bunch of files in this patch

Easy to do.

> > +SIM_AC_COMMON
> 
> please add at least:
> SIM_AC_OPTION_WARNINGS
> 
> and then fix all the warnings :)

I didn't know about SIM_AC_OPTION_WARNINGS. FTR, during development/
cleanup, I modified the visium Makefile to add all the compilation
warnings that we use for GDB, expect pointer signedness, IIRC, which
was creating a lot more noise than what I felt had the time to handle.
Dealing with those warnings was a very useful exercise because it
found a couple of bugs, and allowed a fair amount of cleanup.
I'll followup with pointer signedness in another of your comments...

> > +++ b/sim/visium/sim-fpu.c
> >
> > +/* Note: Although the origin of this file has not been researched,
> > +   we know this is not the master copy of this code, and therefore
> > +   we try to do as few modifications as possible, in order to facilitate
> > +   possible coordination with that original, if it is every found.
> > +   This explains why no apparent effort is made to improve this file's
> > +   style to better match our usual standards.  */
> 
> erm, the origin is pretty clear -- it was duplicated from
> sim/common/sim-fpu.c.  this needs to be rectified.

Does "this" mean the comment, or moving visium to the common sim-fpu?
I see that many of the small differences are comments and formatting,
so I will work towards normalizing. But there seems to be an important
difference in:

     const sim_fpu sim_fpu_qnan = {
    -  sim_fpu_class_qnan, 0, 0, 0
    +  sim_fpu_class_qnan, 1, 1152921367167893504, 1986400654

I am not sure what to do for that one...

I was hoping we could start with visium having its own copy for now,
with the understanding that we should find a solution to avoid it
in the future.

> > +++ b/sim/visium/sim-main.h
> >
> > +  int32_t regs[VISIUM_GENERAL_REGS];
> 
> do you want all the regs to be signed ?  usually ports have them be
> unsigned as it makes coding simpler.

That's the pointer-signedness warnings I was talking about above.
Good eyes! I think you are right; and I actually tried to change it,
but it had ripple effects, and I didn't want to get distracted by
that, while I was working on numerous other cleanups.

I will work on it now...

> > +/* A small macro to return the sim_cpu from the sim descriptor.
> > +   We only support one CPU at the moment, so the CPU index is
> > +   always 0.  But perhaps we'll need to support SMP on this architecture,
> > +   one day, in which case this macro will be useful to help supporting
> > +   that (easy to find all locations, and perhaps CPU selection could
> > +   be automated inside this macro itself).  */
> > +#define VISIUM_STATE_CPU(sd) (STATE_CPU (sd, 0))
> 
> usually you shouldn't need this.  if you have a reference to the state
> but not a cpu, it tends to indicate the API isn't correctly passing down
> the cpu as an argument.  so those funcs should be adjusted instead.

I tried to differentiate between the data which is CPU-specific
(eg. registers) and the data which is shared between all CPUs
(eg. devices). The former was part of the sim_cpu structure, while
on the other hand, the latter was placed inside the sim_desc.
Because you nearly always need access to stuff like the memory
device, I was naturally pushed towards passing the sim_desc rather
than the sim_cpu. To pass the sim_cpu instead, I think I would have
to move a lot of the stuff in struct sim_state to the sim_cpu,
which feels wrong to me.

> > +sim_load (SIM_DESC sd, const char *prog, bfd *abfd, int from_tty)
> > +sim_read (SIM_DESC sd, SIM_ADDR mem, unsigned char *buf, int length)
> > +sim_write (SIM_DESC sd, SIM_ADDR mem, const unsigned char *buf, int length)
> 
> this looks hairy and will require a good bit of unwinding.  you shouldn't
> be defining your own sim_read/sim_write anymore.  if you want memory, you
> should be using the common memory functions to attach it.  if you want to
> simulate hardware, you should be using the WITH_HW framework.

For the read/write functions, we have a feature read-before-write
feature which I don't think the current sim provides. There is
also a pre-initialization feature of the RAM to a certain value
to make execution more reliable when the program reads undefined
memory.  What would you suggest we do?

I will look at the WITH_HW framework.

> > +++ b/sim/visium/visium-trace.c
> > +++ b/sim/visium/visium-trace.h
> 
> i glanced through the trace logic ... it doesn't seem like it's hardware
> specific (like you've got a hardware module that is handling this).  since
> it's all software based, you should throw away the visium trace logic and
> switch to the common sim-trace module.  the sim-trace.h header includes a
> lot of macros to quickly instrument your code.

The traces have to have the the format that visium-trace generates.
This is because the format is then exploited by other tools which
expect that format, and so we cannot change that. I don't think
the sim-trace module allows us to generate the data in the format
we need, does it? If that's not the case, then we have two options:
1.  leave the visum-trace module as it; 2. yank it out. I don't think
that (1) will make global maintenance of the sim project harder, but
if you think (2) is best, then we'll keep this as an AdaCore-only
piece of code.

Thanks again for the super-fast review!
-- 
Joel

Re: new sim: Visium

[Mike Frysinger]

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

On 07 Jan 2016 07:35, Joel Brobecker wrote:
> > > +SIM_AC_COMMON
> > 
> > please add at least:
> > SIM_AC_OPTION_WARNINGS
> > 
> > and then fix all the warnings :)
> 
> I didn't know about SIM_AC_OPTION_WARNINGS. FTR, during development/
> cleanup, I modified the visium Makefile to add all the compilation
> warnings that we use for GDB, expect pointer signedness, IIRC, which
> was creating a lot more noise than what I felt had the time to handle.
> Dealing with those warnings was a very useful exercise because it
> found a couple of bugs, and allowed a fair amount of cleanup.
> I'll followup with pointer signedness in another of your comments...

SIM_AC_OPTION_WARNINGS is based on the gdb flags.  i don't think we add
anything in the sim that isn't in gdb.  it sometimes gets out of date,
but then someone just resyncs them ;).

> > > +++ b/sim/visium/sim-fpu.c
> > >
> > > +/* Note: Although the origin of this file has not been researched,
> > > +   we know this is not the master copy of this code, and therefore
> > > +   we try to do as few modifications as possible, in order to facilitate
> > > +   possible coordination with that original, if it is every found.
> > > +   This explains why no apparent effort is made to improve this file's
> > > +   style to better match our usual standards.  */
> > 
> > erm, the origin is pretty clear -- it was duplicated from
> > sim/common/sim-fpu.c.  this needs to be rectified.
> 
> Does "this" mean the comment, or moving visium to the common sim-fpu?

moving to the common code

> I see that many of the small differences are comments and formatting,
> so I will work towards normalizing. But there seems to be an important
> difference in:
> 
>      const sim_fpu sim_fpu_qnan = {
>     -  sim_fpu_class_qnan, 0, 0, 0
>     +  sim_fpu_class_qnan, 1, 1152921367167893504, 1986400654
> 
> I am not sure what to do for that one...
> 
> I was hoping we could start with visium having its own copy for now,
> with the understanding that we should find a solution to avoid it
> in the future.

is this the only difference ?  iiuc, it's not unheard of for arches (either
in the hardware or the ABI) to define different values for NaN.  as such,
letting a target override this makes sense.

maybe for now introduce a define like:
#ifndef SIM_FPU_QNAN_VALUE
# define SIM_FPU_QNAN_VALUE 0, 0, 0
#endif
const sim_fpu sim_fpu_qnan = {
  sim_fpu_class_qnan, SIM_FPU_QNAN_VALUE
};

and then in your sim-main.h do:
#define SIM_FPU_QNAN_VALUE 1, UNSIGNED64(0xfffffe000000000), 0x7666118e

while you're at it, use hex values to make it more readable :)

> > > +/* A small macro to return the sim_cpu from the sim descriptor.
> > > +   We only support one CPU at the moment, so the CPU index is
> > > +   always 0.  But perhaps we'll need to support SMP on this architecture,
> > > +   one day, in which case this macro will be useful to help supporting
> > > +   that (easy to find all locations, and perhaps CPU selection could
> > > +   be automated inside this macro itself).  */
> > > +#define VISIUM_STATE_CPU(sd) (STATE_CPU (sd, 0))
> > 
> > usually you shouldn't need this.  if you have a reference to the state
> > but not a cpu, it tends to indicate the API isn't correctly passing down
> > the cpu as an argument.  so those funcs should be adjusted instead.
> 
> I tried to differentiate between the data which is CPU-specific
> (eg. registers) and the data which is shared between all CPUs
> (eg. devices). The former was part of the sim_cpu structure, while
> on the other hand, the latter was placed inside the sim_desc.
> Because you nearly always need access to stuff like the memory
> device, I was naturally pushed towards passing the sim_desc rather
> than the sim_cpu. To pass the sim_cpu instead, I think I would have
> to move a lot of the stuff in struct sim_state to the sim_cpu,
> which feels wrong to me.

you can get to the state from the cpu:
  SIM_DESC sd = CPU_STATE (cpu);

so i don't think you need to do any structure shuffling

> > > +sim_load (SIM_DESC sd, const char *prog, bfd *abfd, int from_tty)
> > > +sim_read (SIM_DESC sd, SIM_ADDR mem, unsigned char *buf, int length)
> > > +sim_write (SIM_DESC sd, SIM_ADDR mem, const unsigned char *buf, int length)
> > 
> > this looks hairy and will require a good bit of unwinding.  you shouldn't
> > be defining your own sim_read/sim_write anymore.  if you want memory, you
> > should be using the common memory functions to attach it.  if you want to
> > simulate hardware, you should be using the WITH_HW framework.
> 
> For the read/write functions, we have a feature read-before-write
> feature which I don't think the current sim provides.

i don't know what this feature is.  could you elaborate ?

> There is
> also a pre-initialization feature of the RAM to a certain value
> to make execution more reliable when the program reads undefined
> memory.  What would you suggest we do?

when you attach memory, the default is to be zero filled.  we do this
for all ports.  that sounds pretty reliable to me :).

if you want to use a diff value, you can do this from the command line:
$ run --memory-fill 0xff --memory-size 10Mb ...

did you need something else ?

> > > +++ b/sim/visium/visium-trace.c
> > > +++ b/sim/visium/visium-trace.h
> > 
> > i glanced through the trace logic ... it doesn't seem like it's hardware
> > specific (like you've got a hardware module that is handling this).  since
> > it's all software based, you should throw away the visium trace logic and
> > switch to the common sim-trace module.  the sim-trace.h header includes a
> > lot of macros to quickly instrument your code.
> 
> The traces have to have the the format that visium-trace generates.
> This is because the format is then exploited by other tools which
> expect that format, and so we cannot change that. I don't think
> the sim-trace module allows us to generate the data in the format
> we need, does it? If that's not the case, then we have two options:
> 1.  leave the visum-trace module as it; 2. yank it out. I don't think
> that (1) will make global maintenance of the sim project harder, but
> if you think (2) is best, then we'll keep this as an AdaCore-only
> piece of code.

currently the sim-trace module does not have output formats.  i'm open
to extending this so ports can add custom hooks to control it.  can you
provide a few sample lines ?  would hooking at trace_generic be all you
needed ?

my concern isn't so much about global maintenance (although that's always
part of it), but about users having consistent behavior across targets.
i've been trying to integrate sim-trace into more common places, and so
targets can easily sprinkle one or two lines around, and then get access
to a lot of useful data.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: new sim: Visium

[Joel Brobecker]

Hi Mike,

[sorry for the late answer, crazy week...]

> > I see that many of the small differences are comments and formatting,
> > so I will work towards normalizing. But there seems to be an important
> > difference in:
> > 
> >      const sim_fpu sim_fpu_qnan = {
> >     -  sim_fpu_class_qnan, 0, 0, 0
> >     +  sim_fpu_class_qnan, 1, 1152921367167893504, 1986400654
> > 
> > I am not sure what to do for that one...
> > 
> > I was hoping we could start with visium having its own copy for now,
> > with the understanding that we should find a solution to avoid it
> > in the future.
> 
> is this the only difference ?  iiuc, it's not unheard of for arches (either
> in the hardware or the ABI) to define different values for NaN.  as such,
> letting a target override this makes sense.
> 
> maybe for now introduce a define like:
> #ifndef SIM_FPU_QNAN_VALUE
> # define SIM_FPU_QNAN_VALUE 0, 0, 0
> #endif
> const sim_fpu sim_fpu_qnan = {
>   sim_fpu_class_qnan, SIM_FPU_QNAN_VALUE
> };
> 
> and then in your sim-main.h do:
> #define SIM_FPU_QNAN_VALUE 1, UNSIGNED64(0xfffffe000000000), 0x7666118e
> 
> while you're at it, use hex values to make it more readable :)

That works.

I also had a change to look at the differences more closely,
and there are more; but I haven't really had much time to check
how significant those changes actually are. I'll keep you posted,
of course.

> you can get to the state from the cpu:
>   SIM_DESC sd = CPU_STATE (cpu);
> 
> so i don't think you need to do any structure shuffling

Ah, good! I will make that change.

> > > this looks hairy and will require a good bit of unwinding.  you shouldn't
> > > be defining your own sim_read/sim_write anymore.  if you want memory, you
> > > should be using the common memory functions to attach it.  if you want to
> > > simulate hardware, you should be using the WITH_HW framework.
> > 
> > For the read/write functions, we have a feature read-before-write
> > feature which I don't think the current sim provides.
> 
> i don't know what this feature is.  could you elaborate ?

We can configure the simulator to raise a warning/error when
a region of memory which hasn't been written/initialized yet
is being read.

I think this is a debugging aid, to detect access to uninitialized
memory.

> > There is
> > also a pre-initialization feature of the RAM to a certain value
> > to make execution more reliable when the program reads undefined
> > memory.  What would you suggest we do?
> 
> when you attach memory, the default is to be zero filled.  we do this
> for all ports.  that sounds pretty reliable to me :).
> 
> if you want to use a diff value, you can do this from the command line:
> $ run --memory-fill 0xff --memory-size 10Mb ...
> 
> did you need something else ?

I think "need" is a slight overstatement in our context, but perhaps
we could have a to specify what byte value a port wants to use for
pre-filling the memory? I could see a #define that to provide a
common default which each sim could override for their own default;
and then, if people want, a configure option to force whatever value
they want, irrespective of the default chosen by the architecture.

The people who first implemented this visium simulator chose 0xff,
which is just as arbitrary as zero, but it just so happens that using
this value made me realize, when I wrote a test, that I was making an
invalid assumption, and that the test might actually bomb on me anytime.
At the time, I didn't see the problem because my memory region happened
to be initialized to zero, which is the assumption I was making.

> currently the sim-trace module does not have output formats.  i'm open
> to extending this so ports can add custom hooks to control it.  can you
> provide a few sample lines ?  would hooking at trace_generic be all you
> needed ?

For that part, I discussed with Olivier Hainque, who is a lot more
familiar with the traces than I am; the outcome of the discussion
is that it's going to be easier to separate that feature from this
submission, so we'll simply ignore that part for now, because we are
both a bit short on time at the moment.

Thanks!
-- 
Joel

Re: new sim: Visium

[Mike Frysinger]

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

On 17 Jan 2016 13:24, Joel Brobecker wrote:
> > > > this looks hairy and will require a good bit of unwinding.  you shouldn't
> > > > be defining your own sim_read/sim_write anymore.  if you want memory, you
> > > > should be using the common memory functions to attach it.  if you want to
> > > > simulate hardware, you should be using the WITH_HW framework.
> > > 
> > > For the read/write functions, we have a feature read-before-write
> > > feature which I don't think the current sim provides.
> > 
> > i don't know what this feature is.  could you elaborate ?
> 
> We can configure the simulator to raise a warning/error when
> a region of memory which hasn't been written/initialized yet
> is being read.
> 
> I think this is a debugging aid, to detect access to uninitialized
> memory.

this sounds like a useful feature we can add to the common core :)

> > > There is
> > > also a pre-initialization feature of the RAM to a certain value
> > > to make execution more reliable when the program reads undefined
> > > memory.  What would you suggest we do?
> > 
> > when you attach memory, the default is to be zero filled.  we do this
> > for all ports.  that sounds pretty reliable to me :).
> > 
> > if you want to use a diff value, you can do this from the command line:
> > $ run --memory-fill 0xff --memory-size 10Mb ...
> > 
> > did you need something else ?
> 
> I think "need" is a slight overstatement in our context, but perhaps
> we could have a to specify what byte value a port wants to use for
> pre-filling the memory? I could see a #define that to provide a
> common default which each sim could override for their own default;
> and then, if people want, a configure option to force whatever value
> they want, irrespective of the default chosen by the architecture.

the default should be the same across architectures imo, and that default
today is zero-filled memory.  having a configure flag might be useful, but
i'm not sure how many people would actually leverage it.  especially when
there are flags already (that i showed above) that do what they want.

to clarify some background in case it helps: i see the simulator core as a
bunch of generic building blocks.  the arch port itself only adds support
for the ISA (insns & registers) to the equation.  once you get beyond that
(e.g. the memory or devices), now you're talking about more building blocks
than stuff that should be baked in/architecture defaults.  so even if today
all of the systems that have a visium cpu also initialize the system mem in
a specific way, the visium arch core should not be doing any of that.  this
makes it very easy to take just the visium ISA and construct a new cpu from
scratch that has new/different behavior and play around with things.  when
you do want behavior that matches an existing board, that's where the model
framework comes into play.  you can define specific cpu/system combinations
that match existing products and users can pick those via the --model flag.

does that make sense ?

> The people who first implemented this visium simulator chose 0xff,
> which is just as arbitrary as zero, but it just so happens that using
> this value made me realize, when I wrote a test, that I was making an
> invalid assumption, and that the test might actually bomb on me anytime.
> At the time, I didn't see the problem because my memory region happened
> to be initialized to zero, which is the assumption I was making.

this can easily be dangerous when you write asm code, but i feel like this
doesn't translate (that much) across to C code.  static/uninitialized vars
(i.e. bss) are required to be zero-initialized.  people often run into
trouble with uninitialized stack values, but you'd get same behavior there
i think in the sim today as you would on a real system.

> > currently the sim-trace module does not have output formats.  i'm open
> > to extending this so ports can add custom hooks to control it.  can you
> > provide a few sample lines ?  would hooking at trace_generic be all you
> > needed ?
> 
> For that part, I discussed with Olivier Hainque, who is a lot more
> familiar with the traces than I am; the outcome of the discussion
> is that it's going to be easier to separate that feature from this
> submission, so we'll simply ignore that part for now, because we are
> both a bit short on time at the moment.

i've started a wiki page here:
  https://sourceware.org/gdb/wiki/Sim/TODO

feel free to add a bullet point there with sub points that outline your
requirements.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]