RFA: AArch64 sim

RFA: AArch64 sim

[Nick Clifton]

[-- Attachment #1: Type: text/plain, Size: 2601 bytes --]

Hi Mike,

  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.

  This is still a work in progress as it does not yet emulate all the
  aarch64 instructions, nor does it have a testsuite.  Plus of course
  there are bound to be bugs to be fixed.  But with this sim in place I
  am able to run the gcc and g++ testsuites for an aarch64-elf toolchain
  and see results like this:

		=== gcc Summary for aarch64-sim ===

    # of expected passes		89895
    # of unexpected failures	        3462
    # of expected failures		243
    # of unresolved testcases	        3
    # of unsupported tests		1903

		=== gcc Summary for aarch64-sim/-mabi=ilp32 ===

    # of expected passes		90026
    # of unexpected failures	        3276
    # of expected failures		242
    # of unresolved testcases	        4
    # of unsupported tests		1936

		=== g++ Summary for aarch64-sim ===

    # of expected passes		78136
    # of unexpected failures	        1318
    # of expected failures		261
    # of unsupported tests		3903

		=== g++ Summary for aarch64-sim/-mabi=ilp32 ===

    # of expected passes		78608
    # of unexpected failures	        960
    # of unexpected successes	        4
    # of expected failures		257
    # of unsupported tests		3895

  So an approximate 96% pass rate for gcc and a 98% pass rate for G++.
  Not too bad for a first attempt.

  Is this patch OK to apply ?

Cheers
  Nick

sim/ChangeLog
2015-06-28  Nick Clifton  <nickc@redhat.com>

	* configure.tgt (sim_arch): Add aarch64 target.
	* configure: Regenerate.
        * aarch64: New directory.
        * aarch64/aclocal.m4: Generate.
	* aarch64/config.in: Generate.
	* aarch64/configure: Generate.
	* aarch64/configure.ac: New file.
	* aarch64/cpustate.c: New file. Models the AArch64 registers.
	* aarch64/cpustate.h: New file.
	* aarch64/decode.h: Prototypes and types for the decoder
	functions. 
	* aarch64/gdb-if.c: New file. Interface with GDB.
	* aarch64/main.c: New file.  Stand alone front end for the
	simulator.
	* aarch64/Makefile.in: New file.
	* aarch64/memory.c: New file. Models the AArch64 memory subsystem.
	* aarch64/memory.h: New file.
	* aarch64/simulator.c: New file. Emulated AArch64 instructions.
	* aarch64/simulator.h: New file.

include/gdb/ChangeLog
2015-06-28  Nick Clifton  <nickc@redhat.com>

	* sim-aarch64.h: New file.


[-- Attachment #2: aarch64.sim.patch.xz --]
[-- Type: application/x-xz, Size: 132012 bytes --]

Re: RFA: AArch64 sim

[Andre Vieira]

On 28/06/15 13:24, Nick Clifton wrote:
> Hi Mike,
>
>    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.
>
>    This is still a work in progress as it does not yet emulate all the
>    aarch64 instructions, nor does it have a testsuite.  Plus of course
>    there are bound to be bugs to be fixed.  But with this sim in place I
>    am able to run the gcc and g++ testsuites for an aarch64-elf toolchain
>    and see results like this:
>
> 		=== gcc Summary for aarch64-sim ===
>
>      # of expected passes		89895
>      # of unexpected failures	        3462
>      # of expected failures		243
>      # of unresolved testcases	        3
>      # of unsupported tests		1903
>
> 		=== gcc Summary for aarch64-sim/-mabi=ilp32 ===
>
>      # of expected passes		90026
>      # of unexpected failures	        3276
>      # of expected failures		242
>      # of unresolved testcases	        4
>      # of unsupported tests		1936
>
> 		=== g++ Summary for aarch64-sim ===
>
>      # of expected passes		78136
>      # of unexpected failures	        1318
>      # of expected failures		261
>      # of unsupported tests		3903
>
> 		=== g++ Summary for aarch64-sim/-mabi=ilp32 ===
>
>      # of expected passes		78608
>      # of unexpected failures	        960
>      # of unexpected successes	        4
>      # of expected failures		257
>      # of unsupported tests		3895
>
>    So an approximate 96% pass rate for gcc and a 98% pass rate for G++.
>    Not too bad for a first attempt.
>
>    Is this patch OK to apply ?
>
> Cheers
>    Nick
>
> sim/ChangeLog
> 2015-06-28  Nick Clifton  <nickc@redhat.com>
>
> 	* configure.tgt (sim_arch): Add aarch64 target.
> 	* configure: Regenerate.
>          * aarch64: New directory.
>          * aarch64/aclocal.m4: Generate.
> 	* aarch64/config.in: Generate.
> 	* aarch64/configure: Generate.
> 	* aarch64/configure.ac: New file.
> 	* aarch64/cpustate.c: New file. Models the AArch64 registers.
> 	* aarch64/cpustate.h: New file.
> 	* aarch64/decode.h: Prototypes and types for the decoder
> 	functions.
> 	* aarch64/gdb-if.c: New file. Interface with GDB.
> 	* aarch64/main.c: New file.  Stand alone front end for the
> 	simulator.
> 	* aarch64/Makefile.in: New file.
> 	* aarch64/memory.c: New file. Models the AArch64 memory subsystem.
> 	* aarch64/memory.h: New file.
> 	* aarch64/simulator.c: New file. Emulated AArch64 instructions.
> 	* aarch64/simulator.h: New file.
>
> include/gdb/ChangeLog
> 2015-06-28  Nick Clifton  <nickc@redhat.com>
>
> 	* sim-aarch64.h: New file.
>
Hi Nick,

I realize EXTRA_SIM_CFLAGS with "-lm" is in there to deal with the use 
of math lib. Though I think EXTRA_SIM_CFLAGS is being expanded too early 
in the compile command, i.e. before libsim.a is passed on. Shouldn't it 
be put after libsim.a? As it could potentially cause a linker failure.

Kind Regards,
Andre Vieira

Re: RFA: AArch64 sim

[Nicholas Clifton]

Hi Andre,

> I realize EXTRA_SIM_CFLAGS with "-lm" is in there to deal with the use
> of math lib. Though I think EXTRA_SIM_CFLAGS is being expanded too early
> in the compile command, i.e. before libsim.a is passed on. Shouldn't it
> be put after libsim.a? As it could potentially cause a linker failure.

You are right, although strangely it still works for me...

Anyway, please could you test out the patch below.  If it works for you 
then I will check it in.

Cheers
   Nick

diff --git a/sim/arm/Makefile.in b/sim/arm/Makefile.in
index 7605588..1eeec25 100644
--- a/sim/arm/Makefile.in
+++ b/sim/arm/Makefile.in
@@ -17,7 +17,8 @@

  ## COMMON_PRE_CONFIG_FRAG

-SIM_EXTRA_CFLAGS = -DMODET -lm
+SIM_EXTRA_CFLAGS = -DMODET
+SIM_EXTRA_LIBS = -lm

  SIM_OBJS = \
         wrapper.o \

Re: RFA: AArch64 sim

[Andre Vieira]

That works for me!

Thank you,
Andre Vieira

On 02/07/15 14:53, Nicholas Clifton wrote:
> Hi Andre,
>
>> I realize EXTRA_SIM_CFLAGS with "-lm" is in there to deal with the use
>> of math lib. Though I think EXTRA_SIM_CFLAGS is being expanded too early
>> in the compile command, i.e. before libsim.a is passed on. Shouldn't it
>> be put after libsim.a? As it could potentially cause a linker failure.
>
> You are right, although strangely it still works for me...
>
> Anyway, please could you test out the patch below.  If it works for you
> then I will check it in.
>
> Cheers
>     Nick
>
> diff --git a/sim/arm/Makefile.in b/sim/arm/Makefile.in
> index 7605588..1eeec25 100644
> --- a/sim/arm/Makefile.in
> +++ b/sim/arm/Makefile.in
> @@ -17,7 +17,8 @@
>
>    ## COMMON_PRE_CONFIG_FRAG
>
> -SIM_EXTRA_CFLAGS = -DMODET -lm
> +SIM_EXTRA_CFLAGS = -DMODET
> +SIM_EXTRA_LIBS = -lm
>
>    SIM_OBJS = \
>           wrapper.o \
>

Re: RFA: AArch64 sim

[Nicholas Clifton]

Hi Andre,

> That works for me!

Great - I have checked the patch in along with this changelog entry.

Cheers
   Nick

sim/arm/ChangeLog
2015-07-02  Nick Clifton  <nickc@redhat.com>

	* Makefile.in (SIM_EXTRA_CFLAGS): Revert previous delta.
	(SIM_EXTRA_LIBS): Add -lm.

Re: RFA: AArch64 sim

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 8877 bytes --]

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: RFA: AArch64 sim

[Nick Clifton]

[-- Attachment #1: Type: text/plain, Size: 2377 bytes --]

Hi Mike,

  Thanks very much for the detailed review.  Attached is a second
  attempt at contributing this work.  New in this edition are:

  * A basic pass/fail testsuite based upon similar code in the moxie
    directory.  I am planning to expand this to a proper, full testsuite
    later on.

  * The gdb-if.c file has been renamed to interp.c.

  * The files now have an appropriate FSF copyright notice.

  * The enum values in sim-aarch64.h are now in uppercase.

  * The configure.ac file is now a clone of the one in sim/arm.

  * The sources now compile in ISO C90 mode.

  * Extraneous whitespace has been removed.

  * The standard uintX_t types are used throughout.

  * Most global state is now held in the SIM_DESC structure.

  * // comments have been replaced with /* ... */

  * Prints to stderr have been replaced by calls to sim_io_eprintf

  * Register numbers are stored in an enum.

  * Updated Makefile.in.

  * The tracing code has been replaced with the sim/common tracing
    functions.

  * The sim common core memory functions are now used to read and write
    memory.


  I hope that this rewrite meets your approval.

  OK to apply ?

Cheers
  Nick

sim/ChangeLog
2015-07-15  Nick Clifton  <nickc@redhat.com>

	* configure.tgt (sim_arch): Add aarch64 target.
	* configure: Regenerate.
        * aarch64: New directory.
        * aarch64/aclocal.m4: Generate.
	* aarch64/config.in: Generate.
	* aarch64/configure: Generate.
	* aarch64/configure.ac: New file.
	* aarch64/cpustate.c: New file. Models the AArch64 registers.
	* aarch64/cpustate.h: New file.
	* aarch64/decode.h: Prototypes and types for the decoder
	functions. 
	* aarch64/interp.c: New file. Interface with GDB.
	* aarch64/Makefile.in: New file.
	* aarch64/memory.c: New file. Models the AArch64 memory subsystem.
	* aarch64/memory.h: New file.
	* aarch64/simulator.c: New file. Emulated AArch64 instructions.
	* aarch64/simulator.h: New file.
	* aarch64/sim-main.h: New file.  Interface with common code.

sim/testsuite/ChangeLog
2015-07-15  Nick Clifton  <nickc@redhat.com>

	* sim/aarch64: New directory.
	* sim/aarch64/pass.s: New file.  Tests simulator operation.
	* sim/aarch64/allinsn.exp: New file.  Test driver.
	* sim/aarch64/testutils.inc: New file.  Test support macros.

include/gdb/ChangeLog
2015-07-15  Nick Clifton  <nickc@redhat.com>

	* sim-aarch64.h: New file.


[-- Attachment #2: aarch64.sim.patch.2.xz --]
[-- Type: application/x-xz, Size: 51512 bytes --]

Re: RFA: AArch64 sim

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 4979 bytes --]

On 15 Jul 2015 17:58, Nick Clifton wrote:

it's looking pretty good.  comments inline.

> +++ include/gdb/sim-aarch64.h
>
> +/* sim-aarch64.h --- interface between AArch64  simulator and GDB.

only one space before "simulator"

> +#ifdef __cplusplus
> +extern "C" { // }
> +#endif

hmm, i see a few arches do this, but most don't.  is there any reason we should 
keep this ?  or should we scrub all targets to not do this ?

> +++ sim/aarch64/cpustate.c
>
> +#define reg_num(reg) ((reg == R31 && !r31_is_sp) ? 32 : reg)

prob doesn't really matter, but it should be (reg) in both places

> +  if (val != REG (sd)->gr[reg].u64)
> +    TRACE_REGISTER (STATE_CPU (sd, 0),

ideally you'd pass in the cpu instead of hardcoding CPU 0 everywhere via sd.  
you can get sd from the cpu via CPU_STATE().

> +/* Set the CPSR register as an int.  */
> +StatusCode
> +aarch64_set_CPSR (SIM_DESC sd, uint32_t new_flags)
> +{
> ...
> +    }	

should double check trailing whitespace

> +++ sim/aarch64/cpustate.h
>
> +#if __WORDSIZE == 64
> +#define PRINT_64 "l"
> +#else
> +#define PRINT_64 "ll"
> +#endif

you want inttypes.h and PRIi64/etc... and then delete PRINT_64 entirely

> +typedef enum
> +{
> +  STATUS_READY   = 0, /* May continue stepping or running.  */
> +  STATUS_RETURN  = 1, /* Via normal return from initial frame.  */
> +  STATUS_HALT    = 2, /* Via HALT pseudo-instruction.  */
> +  STATUS_BREAK   = 3, /* Via BRK instruction.  */
> +  STATUS_CALLOUT = 4, /* Via CALLOUT pseudo-instruction.  */
> +  STATUS_ERROR   = 5, /* Simulator detected problem.  */
> +  STATUS_MAX     = 6
> +} StatusCode;

a scan of the code indicates that most of this looks like you're setting state 
and then acting on it later yourself when you really should be calling 
sim_engine_halt directly.  any reason for doing it this way ?

> +#endif /* ifndef DECODE_H */

usually our comments don't include "ifndef" and such, just the macro name 
(which is _DECODE_H here i think)

> +++ sim/aarch64/interp.c
>
> +static char opbuf[1000];

should you have a sanity check on this to make sure it doesn't overflow ?

> +static int
> +op_printf (char *buf, char *fmt, ...)

should have a printf attribute on it

> +static int
> +sim_dis_read (bfd_vma                     memaddr,
> +	      bfd_byte *                  ptr,
> +	      unsigned int                length,
> +	      struct disassemble_info *   info)
> +{
> +  aarch64_get_mem_blk (NULL, memaddr, (char *) ptr, length);
> +  return 0;
> +}

shouldn't

> +compare_symbols (const void * ap, const void * bp)

no space after the *.  this comes up a few times in this patch.

> +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)

hmm, do you even need these anymore ?  now that you're using the sim core for 
memory handling, the common ones should just work.

> +++ sim/aarch64/Makefile.in
>
> +INCLUDE = cpustate.h memory.h decode.h

pretty sure this isn't needed so you can delete it

> +interp.o:    cpustate.h memory.h simulator.h sim-main.h
> +cpustate.o:  cpustate.h memory.h simulator.h sim-main.h
> +simulator.o: cpustate.h memory.h simulator.h sim-main.h
> +main.o:      cpustate.h memory.h simulator.h sim-main.h
> +memory.o:    cpustate.h memory.h simulator.h sim-main.h

you should be able to delete all these rules as they'll get automatically generated now

> +++ sim/aarch64/memory.c
>
> +static bfd * saved_prog = NULL;

this whole handling of saved_prog is so tortured.  it looks like you should 
change aarch64_init to accept the bfd directly from the sim func.

> +static inline void

> +StatusCode
> +aarch64_get_mem_blk (SIM_DESC sd, uint64_t address, char * buffer, unsigned length)
> +{
> +  char * addr = sim_core_trans_addr (sd, STATE_CPU (sd, 0), 0, address);

pretty sure map should be set to read_map instead of 0

> +
> +  if (addr == NULL)
> +    {
> +      memset (buffer, 0, length);
> +      if (sd)
> +	{
> +	  mem_error (sd, "read of non-existant mem block at", address);
> +	  return aarch64_set_status (sd, STATUS_ERROR, ERROR_EXCEPTION);
> +	}
> +      return STATUS_ERROR;
> +    }
> +
> +  memcpy (buffer, addr, length);
> +  return STATUS_READY;
> +}

seems like this whole func should be replaced with sim_core_read_buffer.

> +/* Accessor macro to obtain the aarch64 sim registers.  */
> +#define REG(sd)   STATE_CPU ((sd), 0)

would really really prefer the cpu get passed down into funcs instead of 
hardcoding 0

> --- /dev/null
> +++ sim/aarch64/simulator.c
>
> +  return aarch64_set_reg_u64 (sd, rd, NO_SP, aarch64_get_mem_u32 (sd, aarch64_get_PC (sd) + offset * 4));

there's a few lines in this file that need wrapping

largely scanned the rest as i'm not familiar with the aarch64 isa and don't 
have time to learn atm ;)
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: RFA: AArch64 sim

[Nicholas Clifton]

[-- Attachment #1: Type: text/plain, Size: 1713 bytes --]

Hi Mike,

   Thanks for the further review.  I am attaching a third version of the 
patch with all of the issues you raised fixed, except for two...

>> +++ include/gdb/sim-aarch64.h

>> +#ifdef __cplusplus
>> +extern "C" { // }
>> +#endif
>
> hmm, i see a few arches do this, but most don't.  is there any reason we should
> keep this ?  or should we scrub all targets to not do this ?

It is your call.  I saw that other header files in this directory were 
doing it, so I thought that it would be wise to follow their example. 
The extra code does not hurt when compiling with C and I presume that it 
is necessary when compiling with C++.  (I do not know this for sure 
though - I hate C++).  I am happy to remove the code if you want however.


>> +typedef enum
>> +{
>> +  STATUS_READY   = 0, /* May continue stepping or running.  */
>> +  STATUS_RETURN  = 1, /* Via normal return from initial frame.  */
>> +  STATUS_HALT    = 2, /* Via HALT pseudo-instruction.  */
>> +  STATUS_BREAK   = 3, /* Via BRK instruction.  */
>> +  STATUS_CALLOUT = 4, /* Via CALLOUT pseudo-instruction.  */
>> +  STATUS_ERROR   = 5, /* Simulator detected problem.  */
>> +  STATUS_MAX     = 6
>> +} StatusCode;
>
> a scan of the code indicates that most of this looks like you're setting state
> and then acting on it later yourself when you really should be calling
> sim_engine_halt directly.  any reason for doing it this way ?

Originally it was simply historical - this is the way the code was 
written in the smallaarch64sim.  Now it is because it allows better 
tracing and disassembler output, and cleaner code - the halt and error 
returns are only handled in one place.

Is this version OK to apply ?

Cheers
   Nick


[-- Attachment #2: aarch64.sim.patch.3.xz --]
[-- Type: application/x-xz, Size: 51300 bytes --]

Re: RFA: AArch64 sim

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 5606 bytes --]

On 17 Jul 2015 15:10, Nicholas Clifton wrote:
> >> +++ include/gdb/sim-aarch64.h
> 
> >> +#ifdef __cplusplus
> >> +extern "C" { // }
> >> +#endif
> >
> > hmm, i see a few arches do this, but most don't.  is there any reason we should
> > keep this ?  or should we scrub all targets to not do this ?
> 
> It is your call.  I saw that other header files in this directory were 
> doing it, so I thought that it would be wise to follow their example. 
> The extra code does not hurt when compiling with C and I presume that it 
> is necessary when compiling with C++.  (I do not know this for sure 
> though - I hate C++).  I am happy to remove the code if you want however.

i've posted a patch to clean up the others.  you should trim it from this
one in the meantime.

> >> +typedef enum
> >> +{
> >> +  STATUS_READY   = 0, /* May continue stepping or running.  */
> >> +  STATUS_RETURN  = 1, /* Via normal return from initial frame.  */
> >> +  STATUS_HALT    = 2, /* Via HALT pseudo-instruction.  */
> >> +  STATUS_BREAK   = 3, /* Via BRK instruction.  */
> >> +  STATUS_CALLOUT = 4, /* Via CALLOUT pseudo-instruction.  */
> >> +  STATUS_ERROR   = 5, /* Simulator detected problem.  */
> >> +  STATUS_MAX     = 6
> >> +} StatusCode;
> >
> > a scan of the code indicates that most of this looks like you're setting state
> > and then acting on it later yourself when you really should be calling
> > sim_engine_halt directly.  any reason for doing it this way ?
> 
> Originally it was simply historical - this is the way the code was 
> written in the smallaarch64sim.  Now it is because it allows better 
> tracing and disassembler output, and cleaner code - the halt and error 
> returns are only handled in one place.

i'm not seeing how this is cleaner.  when you call sim_engine_halt, the
code stops at that point.  there is no returning/etc... afterwards.

plus, when i look at some of these funcs, they only ever return READY.
which leads to a lot of return code paths that are pointless.

+#define STORE_FUNC(TYPE, NAME, N)                                      \
+  StatusCode                                                           \
+  aarch64_set_mem_##NAME (sim_cpu *cpu, uint64_t address, TYPE value)  \
+  {                                                                    \
+    TRACE_MEMORY (cpu,                                                 \
+                 "write of %" PRIx64 " (%d bytes) to %" PRIx64,        \
+                 (uint64_t) value, N, address);                        \
+                                                                       \
+    sim_core_write_unaligned_##N (cpu, 0, write_map, address, value);  \
+    return STATUS_READY;                                               \
+  }
...
+static StatusCode
+stur32 (sim_cpu *cpu, int32_t offset)
+{
+  unsigned rn = uimm (cpu->instr, 9, 5);
+  unsigned rd = uimm (cpu->instr, 4, 0);
+
+  return aarch64_set_mem_u32 (cpu,
+                             aarch64_get_reg_u64 (cpu, rn, SP_OK) + offset,
+                             aarch64_get_reg_u32 (cpu, rd, NO_SP));
+}
...
+static StatusCode
+dexLoadUnscaledImmediate (sim_cpu *cpu)
... i scanned a bunch of these and they look like above ...
+       case 0:  return sturb (cpu, imm);
+       case 1:  return ldurb32 (cpu, imm);
+       case 2:  return ldursb64 (cpu, imm);
+       case 3:  return ldursb32 (cpu, imm);
+       case 4:  return sturh (cpu, imm);
+       case 5:  return ldurh32 (cpu, imm);
+       case 6:  return ldursh64 (cpu, imm);
+       case 7:  return ldursh32 (cpu, imm);
+       case 8:  return stur32 (cpu, imm);
+       case 9:  return ldur32 (cpu, imm);
+       case 10: return ldursw (cpu, imm);
+       case 12: return stur64 (cpu, imm);
+       case 13: return ldur64 (cpu, imm);
...

in this func, the only thing that doesn't return READY are:
+         return_NYI;
+         return_UNALLOC;

which look like:
+#define return_UNALLOC                                                 \
+  do                                                                   \
+    {                                                                  \
+      if (TRACE_INSN_P (cpu))                                          \
+       {                                                               \
+         aarch64_print_insn (CPU_STATE (cpu), aarch64_get_PC (cpu));   \
+         TRACE_INSN (cpu,                                              \
+                     "Unallocated instruction detected at sim line %d,"\
+                     " exe addr %" PRIx64,                             \
+                     __LINE__, aarch64_get_PC (cpu));                  \
+       }                                                               \
+      cpu->errorCode = ERROR_UNALLOC;                                  \
+      return STATUS_ERROR;                                             \
+    }                                                                  \
+  while (1)

so instead of returning, just call sim_engine_halt directly

in fact, i only see the errorCode field being set.  nowhere is it being
read.  you might think "wait but there's aarch64_get_ErrorCode and 
aarch64_get_error_text" ... except nothing calls those functions.

so i can't see how errorCode adds any value, just noise, nor does it have
any impact on debugging/tracing output.

> Is this version OK to apply ?

you should scan the files and trim trailing whitespace while you're at it.
sed -i -r 's:[[:space:]]+$::' *.[ch]
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: RFA: AArch64 sim

[Nick Clifton]

[-- Attachment #1: Type: text/plain, Size: 805 bytes --]

Hi Mike,

   Thanks for review.

> i've posted a patch to clean up the others.  you should trim it from this
> one in the meantime.

OK - I have done that.

> i'm not seeing how this is cleaner.  when you call sim_engine_halt, the
> code stops at that point.  there is no returning/etc... afterwards.
>
> plus, when i look at some of these funcs, they only ever return READY.
> which leads to a lot of return code paths that are pointless.

Okey dokey - I have now refactored the code, removing the StatusCode 
return value from lots of functions, and calling sim_engine_halt from 
any location where a problem or break is encountered.

> you should scan the files and trim trailing whitespace while you're at it.
> sed -i -r 's:[[:space:]]+$::' *.[ch]

Also done.

Is this version OK ?

Cheers
   Nick



[-- Attachment #2: aarch64.sim.patch.4.xz --]
[-- Type: application/x-xz, Size: 50424 bytes --]

Re: RFA: AArch64 sim

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1372 bytes --]

On 19 Nov 2015 14:51, Nick Clifton wrote:
> +  return cpu->gr[reg_num(reg)].u64;

you should scan the code and make sure it's doing func (foo) everywhere.
i noticed a few places like this where it's not.

> +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)

please convert to CPU_REG_FETCH/CPU_REG_STORE instead of defining these.
look at commit 9ef4651c4930423f9678832f793343059d4ef9ad as an easy example.
just ignore the Makefile changes as the common core handles that now.

> +SIM_OBJS = \
> ...
> +	sim-reason.o \
> +	sim-stop.o

you can delete these two from your Makefile as the common core adds them.

> +#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)

unused -> delete

> +#define TST( _flag )   (aarch64_test_CPSR_bit (cpu, _flag))
> +#define IS_SET( _X )   ( TST (( _X )))
> +#define IS_CLEAR( _X ) (!TST (( _X )))

drop the weird spacing here

> +report_and_die (sim_cpu *cpu, int exitCode)
> +{
> +  TRACE_EVENTS (cpu, "Exiting simulator with exit code %d", exitCode);
> +  exit (exitCode);
> +}

pretty sure you can use one of the existing abort funcs instead
of defining your own.  like sim_engine_abort.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: RFA: AArch64 sim

[Nick Clifton]

[-- Attachment #1: Type: text/plain, Size: 1290 bytes --]

Hi Mike,

> you should scan the code and make sure it's doing func (foo) everywhere.
> i noticed a few places like this where it's not.

Done.

>> +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)
>
> please convert to CPU_REG_FETCH/CPU_REG_STORE instead of defining these.

Done.

>> +SIM_OBJS = \
>> ...
>> +	sim-reason.o \
>> +	sim-stop.o
>
> you can delete these two from your Makefile as the common core adds them.

Thanks.

>> +#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)
>
> unused -> delete

Done.

>> +#define TST( _flag )   (aarch64_test_CPSR_bit (cpu, _flag))
>> +#define IS_SET( _X )   ( TST (( _X )))
>> +#define IS_CLEAR( _X ) (!TST (( _X )))
>
> drop the weird spacing here

Done.

>> +report_and_die (sim_cpu *cpu, int exitCode)
>> +{
>> +  TRACE_EVENTS (cpu, "Exiting simulator with exit code %d", exitCode);
>> +  exit (exitCode);
>> +}
>
> pretty sure you can use one of the existing abort funcs instead
> of defining your own.  like sim_engine_abort.

Done.

Revised patch attached.  OK to apply ?

Cheers
   Nick



[-- Attachment #2: aarch64.sim.patch.5.xz --]
[-- Type: application/x-xz, Size: 50628 bytes --]

Re: RFA: AArch64 sim

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 105 bytes --]

On 20 Nov 2015 10:56, Nick Clifton wrote:
> Revised patch attached.  OK to apply ?

go for it ! :)
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: RFA: AArch64 sim

[Nick Clifton]

[-- Attachment #1: Type: text/plain, Size: 226 bytes --]

Hi Mike,

> go for it ! :)

Yay!  Thanks very much for the full review(s).

For the record, I am attaching the aarch64-sim.exp file that I use with 
dejagnu in order to run the gcc testsuites using this sim.

Cheers
   Nick



[-- Attachment #2: aarch64-sim.exp --]
[-- Type: text/plain, Size: 2034 bytes --]

# Copyright (C) 2013 Free Software Foundation, Inc.
#
# This file is part of DejaGnu.
#
# DejaGnu is free software; you can redistribute it and/or modify it
# under the terms of the GNU General Public License as published by
# the Free Software Foundation; either version 2 of the License, or
# (at your option) any later version.
#
# DejaGnu is distributed in the hope that it will be useful, but
# WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
# General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with DejaGnu; if not, write to the Free Software Foundation,
# Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA.

# Load the generic configuration for this board. This will define a basic
# set of routines used to communicate with the board.
load_generic_config "sim"

# No multilib flags needed by default.
process_multilib_options ""

# basic-sim.exp is a basic description for the standard Cygnus simulator.
load_base_board_description "basic-sim"

# The name of the directory in the build tree where the simulator lives.
setup_sim aarch64

# The compiler used to build for this board. This has *nothing* to do
# with what compiler is tested if we're testing gcc.
set_board_info compiler 	"[find_gcc]"

# The basic set of flags needed to build "hello world" for this
# board. This board uses libgloss and newlib.
set_board_info cflags 	"[libgloss_include_flags] [newlib_include_flags]"
set_board_info ldflags 	"[libgloss_link_flags] [newlib_link_flags] -Wl,-u,_exit -Wl,-u,_kill -lc -lrdimon"

# This board doesn't use a linker script.
# set_board_info ldscript ""

# Used by a few gcc.c-torture testcases to delimit how large the stack can
# be.
set_board_info gcc,stack_size  16384

# No support for signals and fileio.
set_board_info gdb,nosignals 1
set_board_info gdb,nofileio  1

# More time is needed to compile PlumHall tests
set_board_info gcc,timeout 800