Re: new sim: Visium

[Mike Frysinger <vapier@gentoo.org>]

[-- 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 --]