* RFA: AArch64 sim
@ 2015-06-28 12:25 Nick Clifton
2015-07-02 9:17 ` Andre Vieira
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Nick Clifton @ 2015-06-28 12:25 UTC (permalink / raw)
To: vapier; +Cc: gdb-patches
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: RFA: AArch64 sim
2015-06-28 12:25 RFA: AArch64 sim Nick Clifton
@ 2015-07-02 9:17 ` Andre Vieira
2015-07-02 13:53 ` Nicholas Clifton
2015-07-07 17:12 ` Mike Frysinger
2015-07-15 16:58 ` Nick Clifton
2 siblings, 1 reply; 15+ messages in thread
From: Andre Vieira @ 2015-07-02 9:17 UTC (permalink / raw)
To: Nick Clifton, vapier; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: RFA: AArch64 sim
2015-07-02 9:17 ` Andre Vieira
@ 2015-07-02 13:53 ` Nicholas Clifton
2015-07-02 14:43 ` Andre Vieira
[not found] ` <55954DEE.50609@arm.com>
0 siblings, 2 replies; 15+ messages in thread
From: Nicholas Clifton @ 2015-07-02 13:53 UTC (permalink / raw)
To: Andre Vieira, vapier; +Cc: gdb-patches
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 \
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: RFA: AArch64 sim
2015-07-02 13:53 ` Nicholas Clifton
@ 2015-07-02 14:43 ` Andre Vieira
[not found] ` <55954DEE.50609@arm.com>
1 sibling, 0 replies; 15+ messages in thread
From: Andre Vieira @ 2015-07-02 14:43 UTC (permalink / raw)
To: Nicholas Clifton, vapier; +Cc: gdb-patches
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 \
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: RFA: AArch64 sim
[not found] ` <55954DEE.50609@arm.com>
@ 2015-07-02 15:20 ` Nicholas Clifton
0 siblings, 0 replies; 15+ messages in thread
From: Nicholas Clifton @ 2015-07-02 15:20 UTC (permalink / raw)
To: Andre Simoes Dias Vieira, vapier; +Cc: gdb-patches
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: RFA: AArch64 sim
2015-06-28 12:25 RFA: AArch64 sim Nick Clifton
2015-07-02 9:17 ` Andre Vieira
@ 2015-07-07 17:12 ` Mike Frysinger
2015-07-15 16:58 ` Nick Clifton
2 siblings, 0 replies; 15+ messages in thread
From: Mike Frysinger @ 2015-07-07 17:12 UTC (permalink / raw)
To: Nick Clifton; +Cc: gdb-patches
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: RFA: AArch64 sim
@ 2015-07-15 16:58 ` Nick Clifton
2015-07-16 15:19 ` Mike Frysinger
0 siblings, 1 reply; 15+ messages in thread
From: Nick Clifton @ 2015-07-15 16:58 UTC (permalink / raw)
To: vapier; +Cc: gdb-patches
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: RFA: AArch64 sim
2015-07-15 16:58 ` Nick Clifton
@ 2015-07-16 15:19 ` Mike Frysinger
2015-07-17 14:10 ` Nicholas Clifton
0 siblings, 1 reply; 15+ messages in thread
From: Mike Frysinger @ 2015-07-16 15:19 UTC (permalink / raw)
To: Nick Clifton; +Cc: gdb-patches
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: RFA: AArch64 sim
2015-07-16 15:19 ` Mike Frysinger
@ 2015-07-17 14:10 ` Nicholas Clifton
2015-11-10 7:32 ` Mike Frysinger
0 siblings, 1 reply; 15+ messages in thread
From: Nicholas Clifton @ 2015-07-17 14:10 UTC (permalink / raw)
To: Mike Frysinger; +Cc: gdb-patches
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: RFA: AArch64 sim
2015-07-17 14:10 ` Nicholas Clifton
@ 2015-11-10 7:32 ` Mike Frysinger
2015-11-19 14:51 ` Nick Clifton
0 siblings, 1 reply; 15+ messages in thread
From: Mike Frysinger @ 2015-11-10 7:32 UTC (permalink / raw)
To: Nicholas Clifton; +Cc: gdb-patches
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: RFA: AArch64 sim
2015-11-10 7:32 ` Mike Frysinger
@ 2015-11-19 14:51 ` Nick Clifton
2015-11-20 9:13 ` Mike Frysinger
0 siblings, 1 reply; 15+ messages in thread
From: Nick Clifton @ 2015-11-19 14:51 UTC (permalink / raw)
To: gdb-patches, Mike Frysinger
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: RFA: AArch64 sim
2015-11-19 14:51 ` Nick Clifton
@ 2015-11-20 9:13 ` Mike Frysinger
2015-11-20 10:56 ` Nick Clifton
0 siblings, 1 reply; 15+ messages in thread
From: Mike Frysinger @ 2015-11-20 9:13 UTC (permalink / raw)
To: Nick Clifton; +Cc: gdb-patches
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: RFA: AArch64 sim
2015-11-20 9:13 ` Mike Frysinger
@ 2015-11-20 10:56 ` Nick Clifton
2015-11-20 19:28 ` Mike Frysinger
0 siblings, 1 reply; 15+ messages in thread
From: Nick Clifton @ 2015-11-20 10:56 UTC (permalink / raw)
To: gdb-patches
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: RFA: AArch64 sim
2015-11-20 10:56 ` Nick Clifton
@ 2015-11-20 19:28 ` Mike Frysinger
2015-11-24 8:50 ` Nick Clifton
0 siblings, 1 reply; 15+ messages in thread
From: Mike Frysinger @ 2015-11-20 19:28 UTC (permalink / raw)
To: Nick Clifton; +Cc: gdb-patches
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: RFA: AArch64 sim
2015-11-20 19:28 ` Mike Frysinger
@ 2015-11-24 8:50 ` Nick Clifton
0 siblings, 0 replies; 15+ messages in thread
From: Nick Clifton @ 2015-11-24 8:50 UTC (permalink / raw)
To: gdb-patches
[-- 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
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2015-11-24 8:50 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-28 12:25 RFA: AArch64 sim Nick Clifton
2015-07-02 9:17 ` Andre Vieira
2015-07-02 13:53 ` Nicholas Clifton
2015-07-02 14:43 ` Andre Vieira
[not found] ` <55954DEE.50609@arm.com>
2015-07-02 15:20 ` Nicholas Clifton
2015-07-07 17:12 ` Mike Frysinger
2015-07-15 16:58 ` Nick Clifton
2015-07-16 15:19 ` Mike Frysinger
2015-07-17 14:10 ` Nicholas Clifton
2015-11-10 7:32 ` Mike Frysinger
2015-11-19 14:51 ` Nick Clifton
2015-11-20 9:13 ` Mike Frysinger
2015-11-20 10:56 ` Nick Clifton
2015-11-20 19:28 ` Mike Frysinger
2015-11-24 8:50 ` Nick Clifton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).