Re: new sim: Visium

[Mike Frysinger <vapier at gentoo dot org>]

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: signature.asc
Description: Digital signature