new sim: Visium
Mike Frysinger
vapier@gentoo.org
Thu Jan 7 05:53:00 GMT 2016
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20160107/4e4ebd15/attachment.sig>
More information about the Gdb-patches
mailing list