RFA: AArch64 sim
Mike Frysinger
vapier@gentoo.org
Tue Jul 7 17:12:00 GMT 2015
On 28 Jun 2015 13:24, Nick Clifton wrote:
> The attached patch adds an aarch64 simulator to gdb's sim library. It
> is based upon the smallaarch64sim project:
>
> http://sourceforge.net/projects/smallaarch64sim/
>
> I converted it to C (from C++) because I prefer coding in C, extended
> it so that it could run on non x86 hosts, and added some more
> instruction emulations.
i guess that's why it's pretty rough :)
> This is still a work in progress as it does not yet emulate all the
> aarch64 instructions, nor does it have a testsuite.
a simple pass testsuite is pretty trivial now -- just look at the existing
sim/testsuite/sim/moxie/ tree to get that. of course, small basic tests for
each insn would also be super useful.
> --- /dev/null
> +++ include/gdb/sim-aarch64.h
>
> +/* sim-aarch64.h --- interface between AArch64 simulator and GDB.
> +
> + Copyright 2013 by Red Hat Inc.
> +
> + THIS FILE IS NOT TO BE CONTRIBUTED.
> +
> + This file is part of GDB. */
this header looks very very wrong ...
> +#if !defined (SIM_AARCH64_H)
i know this dir is inconsistent, can we move in the direction of using
"#ifndef" for consistency sake ?
> +enum sim_aarch64_regnum
> +{
> + sim_aarch64_r0_regnum,
looks like not all targets use caps or lower case. i prefer caps myself and
most sims use that instead.
> --- empty/configure.ac
> +++ sim/aarch64/configure.ac
>
> +dnl Process this file with autoconf to produce a configure script.
> +
> +dnl Copyright (C) 2013-2015 Red Hat, Inc.
the FSF peeps want to use consistent copyright/license on new files. shouldn't
be a problem with all these Redhat copyrights to change them to FSF right ?
although for configure.ac, we've so far omitted this header (but the comment
applies to all the other files in this patch).
> +AC_PREREQ(2.64)dnl
> +AC_INIT(Makefile.in)
> +sinclude(../common/acinclude.m4)
> +
> +SIM_AC_COMMON
> +
> +AC_CHECK_HEADERS(getopt.h)
> +
> +SIM_AC_OUTPUT
this looks pretty out of date. i think you want to copy over arm/configure.ac
as-is ?
> --- empty/cpustate.c
> +++ sim/aarch64/cpustate.c
>
> +
can you make sure all files have trailing whitespace stripped from them ?
> +static u_int64_t pc;
> +static u_int64_t nextpc;
> +static u_int32_t CPSR;
> +static u_int32_t FPSR;
two things:
* please use the standard names for types -- "uint32_t" instead of "u_int32_t"
* there should be no global state like this. all registers should live in your
sim_cpu structure and all accesses to those registers should go through the
SIM_CPU variable (that gets passed down into funcs).
> +static GRegister gr[33]; // extra register at index 32 is used to hold zero value
stick to /* ... */ for comments
> --- empty/gdb-if.c
> +++ sim/aarch64/gdb-if.c
personal preference -- name this file interp.c instead
> +/* Ideally, we'd wrap up all the minisim's data structures in an
> + object and pass that around. However, neither GDB nor run needs
> + that ability.
> +
> + So we just have one instance, that lives in global variables, and
> + each time we open it, we re-initialize it. */
> +struct sim_state
> +{
> + const char * message;
> +};
> +
> +static struct sim_state the_minisim =
> +{
> + "This is the sole aarch64 minisim instance. See libsim.a's global variables."
> +};
> +
> +static bfd_boolean open = FALSE;
> +
> +
> +static void
> +check_desc (SIM_DESC sd)
> +{
> + if (sd != & the_minisim)
> + fprintf (stderr, "aarch64 minisim: desc != &the_minisim\n");
> +}
this code all needs to die -- see the comment about sim_open below
a note about fprintf(stderr) -- never use that. we have sim_io_eprintf instead.
> +SIM_RC
> +sim_create_inferior (SIM_DESC sd, struct bfd * abfd, char ** argv, char ** env)
pretty sure we omit the space after the *
> +{
> + check_desc (sd);
> +
> + if (abfd)
> + aarch64_load (abfd);
> +
> + return SIM_RC_OK;
> +}
you don't want to load the bfd here. simply initialize some registers/cpu
state based on details in the bfd. see bfin/interp.c:sim_create_inferior as an
example (although you can ignore the switch statement as that probably does
more than you need).
> +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)
instead of handling memory yourself, you should let the sim core do it. this
is why many ports do something like this in sim_open:
sim_do_commandf (sd, "memory-size 0x800000");
and then the common sim_read/sim_write funcs handle the rest. your use of
aarch64_[gs]et_mem_xxx looks pretty heavily embedded though, so you'll probably
need to run a sed to use the funcs from sim-core.h instead. or cheat and use a
define to rewire to the appropriate function.
> +check_regno (enum sim_aarch64_regnum regno)
> +reg_size (enum sim_aarch64_regnum regno)
> +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)
shouldn't you be using the enums in the sim header instead of hardcoding
constants in all of these funcs ?
> +sim_load (SIM_DESC sd, const char * prog, struct bfd * abfd, int from_tty)
> +sim_info (SIM_DESC sd, int verbose)
> +static enum sim_stop reason;
> +int siggnal;
> +handle_status (int rc)
> +sim_resume (SIM_DESC sd, int step, int sig_to_deliver)
> +sim_stop (SIM_DESC sd)
> +sim_stop_reason (SIM_DESC sd, enum sim_stop *reason_p, int *sigrc_p)
> +sim_do_command (SIM_DESC sd, const char *cmd)
> +sim_complete_command (SIM_DESC sd, const char *text, const char *word)
there's no reason you should implement these in the aarch64 port. delete these
and use the ones from common/.
> +SIM_DESC
> +sim_open (SIM_OPEN_KIND kind,
> + struct host_callback_struct * callback,
> + struct bfd * abfd,
> + char ** argv)
> +{
> + if (open)
> + fprintf (stderr, "aarch64 minisim: re-opened sim\n");
> +
> + /* The 'run' interface doesn't use this function, so we don't care
> + about KIND; it's always SIM_OPEN_DEBUG. */
> + if (kind != SIM_OPEN_DEBUG)
> + fprintf (stderr, "aarch64 minisim: sim_open KIND != SIM_OPEN_DEBUG: %d\n",
> + kind);
> +
> + /* We don't expect any command-line arguments. */
> + if (aarch64_load (abfd) && aarch64_init ())
> + open = TRUE;
> +
> + return & the_minisim;
> +}
this func should be gutted and replaced with the code that lives in
microblaze/interp.c:sim_open (long term i want to unify the common stuff for
this func, but i haven't gotten that far yet)
> --- empty/main.c
> +++ sim/aarch64/main.c
this file should not exist at all. once you fix the Makefile.in, the
common/nrun.c will be used instead.
> --- empty/Makefile.in
> +++ sim/aarch64/Makefile.in
>
> +SIM_EXTRA_CFLAGS = -Wall -Werror
SIM_AC_OPTION_WARNINGS should handle this for you
> +SIM_RUN_OBJS = \
> + main.o \
> + $(ENDLIST)
we don't want any new sims that aren't using nrun.o. sorry, but i've spent a
ton of cycles already converting old sims and it's still not done, and i don't
want to regress further.
> +SIM_OBJS = \
> + gdb-if.o \
> + cpustate.o \
> + simulator.o \
> + memory.o \
missing $(SIM_NEW_COMMON_OBJS) as the first entry
> + $(ENDLIST)
this looks pointless -- delete
> +LIBS = $B/bfd/libbfd.a $B/libiberty/libiberty.a
i can't think of any reason why you need this. the common code should handle
it for you.
> +arch = aarch64
this is needed only if you have custom newlib/libgloss logic in
sim/common/gennltvals.sh which you don't, so just delete it
> +gdb-if.o : memory.h cpustate.h simulator.h \
> + $(srcdir)/../../include/gdb/callback.h \
> + $(srcdir)/../../include/gdb/remote-sim.h \
> + $(srcdir)/../../include/gdb/signals.h \
> + $(srcdir)/../../include/gdb/sim-aarch64.h
> +
> +cpustate.o: cpustate.h simulator.h
> +simulator.o: cpustate.h memory.h simulator.h
> +main.o: cpustate.h memory.h simulator.h
> +memory.o: memory.h simulator.h
delete all this
> --- empty/simulator.c
> +++ sim/aarch64/simulator.c
>
> +#include "../../libgloss/aarch64/svc.h"
this won't work -- libgloss/newlib is no longer in the gdb/binutils combined
repo (i wish it was ...)
> --- empty/simulator.h
> +++ sim/aarch64/simulator.h
>
> +#define TRACE_MEM_WRITES (1 << 0)
> +#define TRACE_REG_WRITES (1 << 1)
> +#define TRACE_FUNCTIONS (1 << 2)
> +#define TRACE_MISC (1 << 3)
> +#define TRACE_ALL ((1 << 4) - 1)
please delete all this and use the existing common trace code
-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/20150707/c119f424/attachment.sig>
More information about the Gdb-patches
mailing list