Re: RFA: AArch64 sim

[Mike Frysinger <vapier at gentoo dot org>]

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

Attachment: signature.asc
Description: Digital signature