This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: new sim: Visium
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Michael Frysinger <vapier at sourceware dot org>, gdb-patches at sourceware dot org, Olivier Hainque <hainque at adacore dot com>
- Date: Thu, 7 Jan 2016 07:35:05 +0400
- Subject: Re: new sim: Visium
- Authentication-results: sourceware.org; auth=none
- References: <20160106044754 dot GC23304 at adacore dot com> <20160106193601 dot GR25548 at vapier dot lan>
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