* 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
[parent not found: <55954DEE.50609@arm.com>]
* 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).