Re: new sim: Visium

[Joel Brobecker <brobecker@adacore.com>]

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