From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 53847 invoked by alias); 6 Dec 2016 23:17:45 -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 53827 invoked by uid 89); 6 Dec 2016 23:17:44 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=xin, Solutions, sims, *cb 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 ESMTP; Tue, 06 Dec 2016 23:17:34 +0000 Received: from vapier.lan (localhost [127.0.0.1]) by smtp.gentoo.org (Postfix) with SMTP id 4C6173415B7; Tue, 6 Dec 2016 23:17:32 +0000 (UTC) Date: Tue, 06 Dec 2016 23:17:00 -0000 From: Mike Frysinger To: Dimitar Dimitrov Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] PRU Simulator port Message-ID: <20161206231731.GG10558@vapier.lan> Mail-Followup-To: Dimitar Dimitrov , gdb-patches@sourceware.org References: <20161205211108.26616-1-dimitar@dinux.eu> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="w2JjAQZceEVGylhD" Content-Disposition: inline In-Reply-To: <20161205211108.26616-1-dimitar@dinux.eu> X-IsSubscribed: yes X-SW-Source: 2016-12/txt/msg00187.txt.bz2 --w2JjAQZceEVGylhD Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Content-length: 8636 On 05 Dec 2016 23:11, Dimitar Dimitrov wrote: a very good start! > --- a/sim/configure.tgt > +++ b/sim/configure.tgt > @@ -76,6 +76,9 @@ case "${target}" in > msp430*-*-*) > SIM_ARCH(msp430) > ;; > + pru*-*-*) > + SIM_ARCH(pru) > + ;; > rl78-*-*) > SIM_ARCH(rl78) > ;; you'll need to run `autoconf` in this dir to regen configure. then include that in your commit. > --- /dev/null > +++ b/sim/pru/Makefile.in > @@ -0,0 +1,25 @@ > +# Makefile template for Configure for the MCore sim library. looks like you'll need to update some of the comments -- this isn't mcore :) > +# Written by Cygnus Solutions. Cygnus didn't write this :) > +SIM_OBJS =3D \ > + interp.o \ > + $(SIM_NEW_COMMON_OBJS) \ > + sim-resume.o interp.o should come after $(SIM_NEW_COMMON_OBJS) > --- /dev/null > +++ b/sim/pru/configure.ac > > +SIM_AC_OPTION_ENDIAN can PRU be little or big endian ? or is it always one or the other ? if it's always one or the other, then you should pass in LITTLE or BIG here. > --- /dev/null > +++ b/sim/pru/interp.c > > + This file is part of GDB, the GNU debugger. should say: This file is part of simulators. > +#include doesn't look like you use this, so drop the include > +#include don't use this header or assert(). use the existing SIM_ASSERT() helpers instead. those provide better error messages and integrate with the sim framework. > +#include pretty sure you don't use stuff from it, so drop the include > +#include "sim-utils.h" your sim-main.h should include sim-base.h which includes sim-utils.h, so no need to include it here > +#include "sim-main.h" you included this already like 5 lines up > +#include "sim-base.h" your sim-main.h should be including sim-base.h, so no need to include it here > +#include "sim-options.h" you don't use these, so drop the include. unless you do below ... > +#define max(a,b) ((a) > (b) ? (a) : (b)) you don't need this -- sim-basics.h already provides it which is included (eventually) by your sim-main.h > +/* DMEM zero address is perfectly valid. But if CRT leaves the first wo= rd > + alone, we can use it as a trap to catch NULL pointer access. */ > +static const int abort_on_dmem_zero_access =3D 0; seems like this should be a debug option so people can change it on the fly ? you could leverage the sim-options.h API to change the value based on command line flags. > +static uint32_t > +pru_extract_unsigned_integer (uint8_t *addr, int len) > +{ > ... > + if (len > (int) sizeof (unsigned long)) change the prototype so that len is a size_t instead of len, then you don't need this cast. size_t is a better type anyways. > + printf ("That operation is not available on integers of more than " sim's should never use printf. you can use sim_io_eprintf instead. then again, the only caller of this func already does a len check which means this scenario should never happen. use sim_io_error instead and that'll trigger an exit/abort. after all, if you let this code continue to run, you'll clobber random memory. > + "%ld bytes.", sizeof (unsigned long)); sizeof returns a size_t which means this should be %zu, not %ld > +static void > +pru_store_unsigned_integer (uint8_t *addr, int len, uint32_t val) same feedback on this func as the extract one above > +static inline uint32_t > +extract_regval (uint32_t val, uint32_t regsel) > +{ > ... > + default: assert (0); return 0; if you want this to abort, call sim_io_error instead with a good error message > +static inline void > +write_regval (uint32_t val, uint32_t *reg, uint32_t regsel) > +{ > ... > + default: assert (0); mask =3D sh =3D 0; same here > +static inline void > +pru_reg2dmem (SIM_CPU *cpu, uint32_t addr, unsigned int nbytes, > + int regn, int regb) > +{ > + if (abort_on_dmem_zero_access && addr < 4) > + { > + sim_core_signal (CPU_STATE (cpu), cpu, PC_byteaddr, write_map, > + nbytes, addr, write_transfer, > + SIM_SIGSEGV); > + } > + else if ((addr >=3D PC_ADDR_SPACE_MARKER) > + || (addr + nbytes > PC_ADDR_SPACE_MARKER)) > + { > + sim_core_signal (CPU_STATE (cpu), cpu, PC_byteaddr, write_map, > + nbytes, addr, write_transfer, > + SIM_SIGSEGV); > + } > + else if ((regn * 4 + regb + nbytes) > (32 * 4)) > + { > + /* Register and load size are not valid. */ > + sim_core_signal (CPU_STATE (cpu), cpu, PC_byteaddr, write_map, > + nbytes, addr, write_transfer, > + SIM_SIGILL); > + } do you really need to do all this ? seems like the existing sim_core_write_1 function already deals properly with writes to out-of-bind addresses. > +static inline void > +pru_dmem2reg (SIM_CPU *cpu, uint32_t addr, unsigned int nbytes, > + int regn, int regb) > +{ > + if (abort_on_dmem_zero_access && addr < 4) > + { > + sim_core_signal (CPU_STATE (cpu), cpu, PC_byteaddr, read_map, > + nbytes, addr, read_transfer, > + SIM_SIGSEGV); > + } > + else if ((addr >=3D PC_ADDR_SPACE_MARKER) > + || (addr + nbytes > PC_ADDR_SPACE_MARKER)) > + { > + /* This check is necessary because our IMEM "address space" > + is not really accessible, yet we have mapped it as a generic > + memory space. */ > + sim_core_signal (CPU_STATE (cpu), cpu, PC_byteaddr, read_map, > + nbytes, addr, read_transfer, > + SIM_SIGSEGV); > + } > + else if ((regn * 4 + regb + nbytes) > (32 * 4)) > + { > + /* Register and load size are not valid. */ > + sim_core_signal (CPU_STATE (cpu), cpu, PC_byteaddr, read_map, > + nbytes, addr, read_transfer, > + SIM_SIGILL); > + } same here > +static void > +set_initial_gprs (SIM_CPU *cpu) > +{ > + long space; unused var -> delete > +static inline unsigned int > +regsel_width (uint32_t regsel) > +{ > ... > + default: assert (0); return 0; sim_io_error instead please > +static void > +pru_sim_xin_mac (SIM_DESC sd, SIM_CPU *cpu, unsigned int rd_regn, > + unsigned int rdb, unsigned int length) > +{ > + if (rd_regn < 25 || (rd_regn * 4 + rdb + length) > (27 + 1) * 4) > + { > + fprintf (stderr, "XIN MAC: invalid transfer regn=3D%u.%u, length= =3D%u\n", > + rd_regn, rdb, length); > + RAISE_SIGILL (); > + return; use sim_io_error instead i think comes up multiple times in this file > +static void > +pru_sim_syscall (SIM_DESC sd, SIM_CPU *cpu) seems like you should use sim_syscall instead of implementing your own ad-hoc syscall ABI > +void > +sim_engine_run (SIM_DESC sd, > + int next_cpu_nr, /* ignore */ > + int nr_cpus, /* ignore */ > + int siggnal) /* ignore */ > +{ sim_engine_run should be pretty small. all the business work of your sim should be in a new function instead. so in the end, you'd want something like: while (1) {=20 step_once (cpu); // Name this whatever you want. if (sim_events_tick (sd)) sim_events_process (sd); } > + /* don't treat r30 and r31 as regular registers, they are I/O! */ GNU style says "Don't", and two spaces after "!" > +void > +sim_info (SIM_DESC sd, int verbose) don't define this func -- let the common one do the work for you > +SIM_DESC > +sim_open (SIM_OPEN_KIND kind, host_callback *cb, > + struct bfd *abfd, char * const *argv) > +{ > ... > + cpu =3D STATE_CPU (sd, 0); doesn't seem like you use this, so delete it > + sim_do_commandf (sd, "memory-region 0x%lx,0x%lx", > + (unsigned long)0, > + (unsigned long)DMEM_DEFAULT_SIZE); > + sim_do_commandf (sd, "memory-region 0x%lx,0x%lx", > + (unsigned long)0x20000000, > + (unsigned long)IMEM_DEFAULT_SIZE); pretty sure you can drop the (unsigned long) cast and change %lx to %x > +void > +sim_close (SIM_DESC sd, int quitting) don't define this func -- let the common one do the work for you > +SIM_RC > +sim_create_inferior (SIM_DESC sd, struct bfd *prog_bfd, > + char * const *argv, char * const *env) > +{ i think you need to deal with STATE_PROG_ARGV here. grep for 'Standalone mode' in other sims as examples. > --- /dev/null > +++ b/sim/pru/pru.h > +#ifndef PRU_H > +#define PRU_H > + > +/* Copyright 2014-2016 Free Software Foundation, Inc. > + Contributed by Dimitar Dimitrov the ifdef should be after the file comment, not before it > --- /dev/null > +++ b/sim/pru/sim-main.h > +#ifndef PRU_SIM_MAIN > +#define PRU_SIM_MAIN > + > +/* Copyright 2014-2016 Free Software Foundation, Inc. the ifdef should be after the file comment, not before it -mike --w2JjAQZceEVGylhD Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature Content-length: 819 -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJYR0cLAAoJEEFjO5/oN/WB7J4P/RCkpXN608ZMWNdDns3O1XCx zNCs/B9KUMlt2l1tu1DkcI3OJuUbW1DAGwG2m2q9mFe0uKdkNqyoGOZRfETVblOv D7aYNEq1hSb7DXFc341XmQgOJYwhcjfdI4aJcsOW9CKsrejG5EYtFvCFTf58fbpm 5FWQh28NCNu6ZvyFwLolgF/ZGp9FbvAzKg9QOv+rWE/X7t0C95+eQQb3cXKULh+U PmgEd70YDAjiUfahk6wx7Zi2zoKMrAoQe+97FaiJ9CCyn5G32BJckLJTVpdsU9ol Yy0OoBWu/S3U7HtKHrzetogfTPOS6hF+fodNcY80ATZvztwaAeptiZWqgrIg0dfF hMHGLWgzXMCGja/cB58PhDU5YzA8DSoi80BobJxRgQnNw4yNBx17Rwa2NVs6t9h0 tlxus8nLWrF6+d86Tji0/DWF92K8mFzo/SQ4L/bE3rldaeLs3fovD5eISmLex9q3 4MW6Q6TT6lrY5FNnTLy47p5QUqJdrbO3r1LxXoATAWPpYKFQLjw11XqTyuXnF8PK qFFN/6eB5mH3h1+Kr1Bbp7axR8Yp3qGkkVzoiXpQiedsSJiQ+5Z3BPPxuz44gG0a 3XXs31HT/gvk+o33gLj5pBSRacwM3REiiiujtU38YB1fxB1PFL0J7Bvm8HTEHjDU HZ+ws3phxTfvARORGk/r =ySP1 -----END PGP SIGNATURE----- --w2JjAQZceEVGylhD--