From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30819 invoked by alias); 16 Jul 2015 15:19:09 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 30776 invoked by uid 89); 16 Jul 2015 15:19:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: smtp.gentoo.org Received: from smtp.gentoo.org (HELO smtp.gentoo.org) (140.211.166.183) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 16 Jul 2015 15:19:05 +0000 Received: from vapier (localhost [127.0.0.1]) by smtp.gentoo.org (Postfix) with SMTP id AB078340662; Thu, 16 Jul 2015 15:19:02 +0000 (UTC) Date: Thu, 16 Jul 2015 15:19:00 -0000 From: Mike Frysinger To: Nick Clifton Cc: gdb-patches@sourceware.org Subject: Re: RFA: AArch64 sim Message-ID: <20150716151902.GE5641@vapier> Mail-Followup-To: Nick Clifton , gdb-patches@sourceware.org References: <87vbe8hvfo.fsf@redhat.com> <87y4ih8ij4.fsf@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="LSp5EJdfMPwZcMS1" Content-Disposition: inline In-Reply-To: <87y4ih8ij4.fsf@redhat.com> X-IsSubscribed: yes X-SW-Source: 2015-07/txt/msg00473.txt.bz2 --LSp5EJdfMPwZcMS1 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Content-length: 4892 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 sh= ould=20 keep this ? or should we scrub all targets to not do this ? > +++ sim/aarch64/cpustate.c > > +#define reg_num(reg) ((reg =3D=3D R31 && !r31_is_sp) ? 32 : reg) prob doesn't really matter, but it should be (reg) in both places > + if (val !=3D 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= .=20=20 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) > +{ > ... > + }=09 should double check trailing whitespace > +++ sim/aarch64/cpustate.h > > +#if __WORDSIZE =3D=3D 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 =3D 0, /* May continue stepping or running. */ > + STATUS_RETURN =3D 1, /* Via normal return from initial frame. */ > + STATUS_HALT =3D 2, /* Via HALT pseudo-instruction. */ > + STATUS_BREAK =3D 3, /* Via BRK instruction. */ > + STATUS_CALLOUT =3D 4, /* Via CALLOUT pseudo-instruction. */ > + STATUS_ERROR =3D 5, /* Simulator detected problem. */ > + STATUS_MAX =3D 6 > +} StatusCode; a scan of the code indicates that most of this looks like you're setting st= ate=20 and then acting on it later yourself when you really should be calling=20 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=20 (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 len= gth) hmm, do you even need these anymore ? now that you're using the sim core f= or=20 memory handling, the common ones should just work. > +++ sim/aarch64/Makefile.in > > +INCLUDE =3D 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 g= enerated now > +++ sim/aarch64/memory.c > > +static bfd * saved_prog =3D NULL; this whole handling of saved_prog is so tortured. it looks like you should= =20 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, unsig= ned length) > +{ > + char * addr =3D 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 =3D=3D 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=20 hardcoding 0 > --- /dev/null > +++ sim/aarch64/simulator.c > > + return aarch64_set_reg_u64 (sd, rd, NO_SP, aarch64_get_mem_u32 (sd, aa= rch64_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= =20 have time to learn atm ;) -mike --LSp5EJdfMPwZcMS1 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature Content-length: 819 -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJVp8tmAAoJEEFjO5/oN/WBspUP/jY5ot3ff66lc9mkRjdu9O3/ Ax9qedWPsOBHsUKwQUPDoaRoeL3SZRUAmispRPmL3lQA65oUWblYPizT4A/m/Fp+ anpxEfjzSga3xC0ojo/5GYA3zRWJ7984cynjNwzBrgYTpm9fVM8X70008U7muI/I GNyfIPdMZZ1XRRzzqDhU/oGhMSQ5xnjs8baliTTgLlQtZGWfQphcNNFEpG6dPeWt MM7Lrh6zQIbJ0zQnMNpQ03oD5hkTxtP8uXBE8GPJxS0YmNvfPek7uaYZ/pT0vOqW NR6ovgAac4YEgNpJNk2VjqwUcGn43efU77ATan3qncLje/l9Gy7mAzJsvKIbsfTB cMdXxUXASTOGcdXql26qabsoicC6zO7V7UbMeJZhlkyTp3oOKzqqaoXZlZzz0gl+ 4vbOzx+eMW2ceyUiBDQ08SvgEMpHSSarR0wSOEawuAigwYYsF3/z8JvowUMUhAKD le0MxPQ7DdS65GnkTtlC04k+aabN8sxvYBvFCQwbktAJ1mI9YG07hev+VZYa+T+i vOJREP9ILmQ31EGcdPU7AIbGI6uszQl/QtVP3S/WkFPW1bVq25OAOrc0x6yCE6bV FxMUIgRnGBa4xluUFkC68BEOQQXYtSPsVnow1/9iH7kyYsU46tTj7oNV8Uu0zD+u mFKaqIcBFhPWMOfyO+6r =M19B -----END PGP SIGNATURE----- --LSp5EJdfMPwZcMS1--