From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21999 invoked by alias); 19 Feb 2015 07:06:17 -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 21979 invoked by uid 89); 19 Feb 2015 07:06:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,SPF_PASS,T_RP_MATCHES_RCVD 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, 19 Feb 2015 07:06:13 +0000 Received: from vapier (localhost [127.0.0.1]) by smtp.gentoo.org (Postfix) with SMTP id 59CFE34088B; Thu, 19 Feb 2015 07:06:10 +0000 (UTC) Date: Thu, 19 Feb 2015 07:06:00 -0000 From: Mike Frysinger To: James Bowman Cc: "gdb-patches@sourceware.org" Subject: Re: [PATCH, FT32] gdb and sim support Message-ID: <20150219070610.GI544@vapier> Mail-Followup-To: James Bowman , "gdb-patches@sourceware.org" References: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Y4/B1uKDHmWlpzsz" Content-Disposition: inline In-Reply-To: X-IsSubscribed: yes X-SW-Source: 2015-02/txt/msg00515.txt.bz2 --Y4/B1uKDHmWlpzsz Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Content-length: 19328 On 03 Feb 2015 04:06, James Bowman wrote: > FT32 is a new high performance 32-bit RISC core developed by FTDI for emb= edded applications. ah, we def don't have enough CPUs in the market as is ;) > Support for FT32 has already been added to binutils. This patch adds gdb = and sim support. where's the testsuite man ? :) it should be trivial to start one with .s files -- just look at sim/testsuite/sim/. otherwise there's no way to keep regressions from slipping in. > 2014-02-03 James Bowman >=20 > * gdb/Makefile.in, gdb/configure.tgt: FT32 target added > * sim/configure.tgt: FT32 target added > * sim/configure: Regenerated > * sim/ft32/configure: Regenerated > * gdb/ft32-tdep.c,h: Support FT32 > * sim/ft32/*: FT32 simulator notes: - the date reflects commit time, not original creation - ChangeLog entries are split up across dirs - line items are indented with tabs - the notes should be full sentences; i.e. you should have a period at the= end on to the code ... a few more notes: - i wrote this feedback over a few "sessions", so there might be incomplet= e=20 thoughts as i paused to do other things - feel free to ask questions about new things or things that aren't clear - as you make the adjustments below, make sure to periodically save & buil= d &=20 run & test ... the sim can be really easy to run into the weeds and trying = to=20 figure out what exactly you got wrong hard to trackback > --- a/gdb/Makefile.in > +++ b/gdb/Makefile.in > > - glibc-tdep.c \ > + ft32-tdep.c \ > + glibc-tdep.c \ looks like you broke the indentation for glibc-tdep.c > --- /dev/null > +++ b/gdb/ft32-tdep.c > > Copyright (C) 2009-2014 Free Software Foundation, Inc. it is 2015 now. should fix in all your files. > +#include do you really need to include this ? other tdeps don't seem to. > +// #include "record-full.h" dead code -> delete ? looks like you've got a couple of dead lines in your patch. please go thro= ugh=20 the whole thing and clean this up. > +/* Use an invalid address value as 'not available' marker. */ > +enum { REG_UNAVAIL =3D (CORE_ADDR) -1 }; space after the - > + int established; // Has the new frame been LINKed use /* comments */ everywhere. seems to come up a few times -- please fix= =20 globally. we've got bfd_boolean for bool fields too rather than open code it with an = int > +static const unsigned char * > +ft32_breakpoint_from_pc (struct gdbarch *gdbarch, > + CORE_ADDR *pcptr, int *lenptr) indentation is slightly broken > +{ > + static gdb_byte breakpoint[] =3D { 0x02, 0x00, 0x34, 0x00 }; const ? > +char *ft32_register_names[] =3D static const char * const ft32_register_names[] ? > +static struct type * > +ft32_register_type (struct gdbarch *gdbarch, int reg_nr) > +{ > + if (reg_nr =3D=3D FT32_PC_REGNUM) > + return builtin_type (gdbarch)->builtin_func_ptr; only one space after the return > #define IS_PUSH(inst) (((inst) & 0xfff00000) =3D=3D 0x84000000) i think usually we put insn constants/etc... into the corresponding=20 include/opcode/ header rather than pasting them inline. this makes sharing= =20 between opcodes, gdb, sim, etc... much easier. > +static CORE_ADDR > +ft32_analyze_prologue (CORE_ADDR start_addr, CORE_ADDR end_addr, > + struct ft32_frame_cache *cache, > + struct gdbarch *gdbarch) indentation is slightly off > if (start_addr >=3D end_addr) > { > return end_addr; > } pretty sure you can (and should) omit braces in cases like this > + if (IS_PUSH(inst)) space needed between func and args: if (IS_PUSH (inst)) a quick scan shows this comes up multiple times -- please fix globally > for (regnum =3D FT32_R0_REGNUM; regnum < FT32_PC_REGNUM; regnum++) > { > if (cache->saved_regs[regnum] !=3D REG_UNAVAIL) > cache->saved_regs[regnum] =3D cache->framesize - cache->saved_reg= s[regnum]; should use a tab here since you hit 8 spaces. this comes up a couple of ti= mes=20 -- please fix globally. > + } > + // printf("(Framesize=3D%lld,cache->saved_regs[FT32_PC_REGNUM]=3D%ld)\= n", cache->framesize, cache->saved_regs[FT32_PC_REGNUM]); > + return next_addr; > + > +} there should be a blank line above the return, not below it ;) > + plg_end =3D ft32_analyze_prologue (func_addr, > + func_end, &cache, gdbarch); indentation is off > + /* Don't use line number debug info for assembly source > + files. */ this doesn't need to be wrapped > + /* No useable line symbol. Use result of prologue parsing > + method. */ neither does this > +static void > +ft32_frame_this_id (struct frame_info *this_frame, > + void **this_prologue_cache, struct frame_id *this_id) > +{ > + struct ft32_frame_cache *cache =3D ft32_frame_cache (this_frame, > + this_prologue_cache); indentation is off > +static struct value * > +ft32_frame_prev_register (struct frame_info *this_frame, > + void **this_prologue_cache, int regnum) > +{ > + struct ft32_frame_cache *cache =3D ft32_frame_cache (this_frame, > + this_prologue_cache); same here > + if (regnum < FT32_NUM_REGS && cache->saved_regs[regnum] !=3D REG_UNAVA= IL) > + { > + return frame_unwind_got_memory (this_frame, regnum, > + 0x800000 | cache->saved_regs[regnu= m]); > + } drop the braces > +static CORE_ADDR > +ft32_frame_base_address (struct frame_info *this_frame, void **this_cach= e) > +{ > + struct ft32_frame_cache *cache =3D ft32_frame_cache (this_frame, > + this_cache); indentation is broken > +#if 0 drop all the #if 0 code. this comes up a few times -- please fix globally. > +static int > +ft32_process_record (struct gdbarch *gdbarch, struct regcache *regcache, > + CORE_ADDR addr) > +{ > + return -1; > +#if 0 > ... 400 more lines ... > +#endif > +} err, what now ? > --- /dev/null > +++ b/gdb/ft32-tdep.h > > +/* Register numbers of various important registers. */ > + > +enum ft32_regnum > +{ > + FT32_FP_REGNUM, /* Address of executing stack frame. */ > + FT32_SP_REGNUM, /* Address of top of stack. */ > + FT32_R0_REGNUM, > + FT32_R1_REGNUM, > + FT32_PC_REGNUM =3D 32 /* Program counter. */ > +}; > + > +/* Number of machine registers. */ > +#define FT32_NUM_REGS 33 /* 32 real registers + PC */ this probably should be in include/gdb/sim-ft32.h instead so you can share = it also, define FT32_NUM_REGS in terms of FT32_PC_REGNUM ? > +#endif /* ft32-tdep.h */ the style is to list the macro (FT32_TDEP_H), not the file > --- /dev/null > +++ b/sim/ft32/Makefile.in > > +dtbdir =3D @datadir@/gdb-`sed q ${srcdir}/../../gdb/version.in`/dtb unused -> delete > +SIM_OBJS =3D interp.o sim-load.o sim-io.o sim-config.o sim-utils.o \ > +sim-options.o sim-module.o sim-core.o sim-endian.o sim-trace.o \ > +sim-engine.o sim-fpu.o sim-bits.o sim-profile.o sim-events.o \ > +sim-memopt.o please one-line & sort this list: SIM_OBJS =3D \ bar.o \ foo.o you also want to use $(SIM_NEW_COMMON_OBJS) at the start and $(SIM_EXTRA_OB= JS)=20 at the end missing: SIM_RUN_OBJS =3D nrun.o > +SIM_EXTRA_LIBS =3D -lm -lz -ldl you don't seem to use any of these libs -> delete > +SIM_EXTRA_CLEAN =3D ft32-clean > +# SIM_EXTRA_INSTALL =3D install-dtb > +# SIM_CFLAGS =3D -DDTB=3D"\"$(dtbdir)/ft32-gdb.dtb\"" unused -> delete > +all: interp.o > + > +interp.o: interp.c > + > +ft32-clean: don't need any of this -> delete > --- /dev/null > +++ b/sim/ft32/configure.ac > > +AC_CHECK_TOOL(DTC, dtc) unused -> delete > +SIM_AC_OPTION_INLINE() drop the parens since you're a new port, you should start with SIM_AC_OPTION_WARNINGS enabl= ed.=20=20 obviously that also means cleaning up all the warnings generated in the ft3= 2/=20 subdir once you do :). > +AC_CHECK_HEADERS(unistd.h) unused -> delete > --- /dev/null > +++ b/sim/ft32/interp.c > > + Contributed by FTDI (support@ftdichip.com) use instead of (support@ftdichip.com) > +This file is part of GDB, the GNU debugger. this isn't part of gdb ;). please fix the indentation in this comment bloc= k.=20=20 these comments apply to all your sim files it looks like. > +#include > +#include you've got a lot of weird system header includes in your files. please che= ck=20 each one to see if they're actually used. > +#include /* for byte ordering macros */ i really hope you're not using macros (like hton/ntoh) from this header :).= =20=20 that's not how the sim handles endianness. a quick glance shows you're not= , so=20 just delete the header and be done. > +typedef int word; > +typedef unsigned int uword; at least uword is not used, and word looks hardly used. i guess you should= punt=20 both and tweak the little code relying on them. > +extern const ft32_opc_info_t ft32_opc_info[128]; > + > +host_callback * callback; > + > +FILE *tracefile; > + > +static char *myname; > +static SIM_OPEN_KIND sim_kind; > +static int issue_messages =3D 0; > + > +struct { > + uint32_t regs[32]; > + uint32_t pc; > + uint8_t pm[262144]; > + uint8_t ram[65536]; > + uint64_t num_i; > + uint64_t cycles; > + uint64_t next_tick_cycle; > + enum sim_stop reason; > + int pm_unlock; > + uint32_t pm_addr; > + int exception; > +} cpu; there should be no global state. read-only constants are OK, but that's no= t=20 what these are. > +unsigned long > +ft32_extract_unsigned_integer (addr, len) should be static > + unsigned char * addr; > + int len; we want these old style prototypes to die ... please convert globally > +{ > + unsigned long retval; > + unsigned char * p; > + unsigned char * startaddr =3D (unsigned char *)addr; > + unsigned char * endaddr =3D startaddr + len; there should be no space after the * as part of the decl: unsigned char *p; but there should be a space after the cast: ... =3D (unsigned char *) addr; > + if (len > (int) sizeof (unsigned long)) > + printf ("That operation is not available on integers of more than %l= d bytes.", > + sizeof (unsigned long)); general rule: a sim should never write directly to stdout/stderr (i.e. use= =20 printf or fprintf). look at common/sim-io.h for the funcs you should inste= ad. > +void > +ft32_store_unsigned_integer (addr, len, val) same feedback for this func as above > +void > +sim_size (int s) > +{ > +} once you convert to nrun.c (see earlier comments), you can drop this > +static void > +set_initial_gprs () funcs that take no args must use (void) in C and not (). > +{ > + int i; > + long space; > +} pointless func ? delete it. > +static void > +interrupt () > +{ > + // cpu.asregs.exception =3D SIGINT; > +} pointless func ? delete it. > +uint32_t cpu_pm_read(SIM_DESC sd, int dw, uint32_t ea) static, and space before the ( > +{ > + sim_cpu *scpu =3D STATE_CPU (sd, 0); /* FIXME */ > + address_word cia =3D CIA_GET (scpu); > + uint32_t r; > + > + if ((ea & ~0x3ffff) !=3D 0) > + { > + fprintf(stderr, "Illegal PM address %08x, pc %#x\n", ea, cpu.pc); > + exit(0); > + } i'm not sure what this mask is trying to accomplish ... are you attemting t= o=20 enforce some kind of address space ? when you initialized the sim, you sho= uld=20 have allocated the memory then, and let the sim itself take care of valid=20 addresses. > +void > +sim_resume (sd, step, siggnal) you should use sim-resume.o instead then you'll need a sim_engine_run like so: void sim_engine_run (SIM_DESC sd, int next_cpu_nr, /* ignore */ int nr_cpus, /* ignore */ int siggnal) /* ignore */ {=20 SIM_CPU *cpu; SIM_ASSERT (STATE_MAGIC (sd) =3D=3D SIM_MAGIC_NUMBER); cpu =3D STATE_CPU (sd, 0); while (1) {=20 step_once (cpu); if (sim_events_tick (sd)) sim_events_process (sd); } } then declare a step_once func and put all the cpu-specific logc in there > +int > +sim_write (sd, addr, buffer, size) > > +int > +sim_read (sd, addr, buffer, size) i think once you switch to nrun.c, you won't need these anymore > +int > +sim_store_register (sd, rn, memory, length) > > +int > +sim_fetch_register (sd, rn, memory, length) you should create a helper func that does the rn<->pointer lookup so that y= ou=20 can use it here. e.g. something like: static unsigned_word ft32_lookup_register (SIM_DESC sd, int nr) { SIM_CPU *cpu =3D STATE_CPU (sd, 0); switch (nr) { case 0: return cpu->registers[0]; ... default: return NULL; } } then the store/fetch funcs become much simpler ideally you'd switch to sim-reg.o in your Makefile's SIM_OBJS ... that'll=20 provide these entry points. that would require also enabling sim-model.o=20 & SIM_AC_OPTION_DEFAULT_MODEL support, but i don't think that'd be too hard. if you look at bfin/machs.c and start at "sim_machs", i think you should be= =20 able to track it down easily enough. > +int > +sim_trace (sd) once you switch to nrun.c, you can delete this > +void > +sim_stop_reason (sd, reason, sigrc) add sim-reason.o to your SIM_OBJS to get this for free > +int > +sim_stop (sd) add sim-stop.o to your SIM_OBJS to get this for free > +void > +sim_info (sd, verbose) once you switch to nrun.c/SIM_NEW_COMMON_OBJS, you get this for free > +SIM_DESC > +sim_open (kind, cb, abfd, argv) before this func, you should add: /* Cover function of sim_state_free to free the cpu buffers as well. */ static void free_state (SIM_DESC sd) { if (STATE_MODULES (sd) !=3D NULL) sim_module_uninstall (sd); sim_cpu_free_all (sd); sim_state_free (sd); } > +{ > + SIM_DESC sd =3D sim_state_alloc (kind, cb); > + SIM_ASSERT (STATE_MAGIC (sd) =3D=3D SIM_MAGIC_NUMBER); i don't think you need this assert after this alloc, you should do: /* The cpu data is kept in a separately allocated chunk of memory. */ if (sim_cpu_alloc_all (sd, 1, /*cgen_cpu_max_extra_bytes ()*/0) !=3D SIM_= RC_OK) {=20 free_state (sd); return 0; } you'll have to add sim-cpu.o to your SIM_OMJS > + if (sim_pre_argv_init (sd, argv[0]) !=3D SIM_RC_OK) > + return 0; you should call free_state in the error path here after this section, you should call: if (sim_pre_argv_init (sd, argv[0]) !=3D SIM_RC_OK) {=20 free_state (sd); return 0; } /* getopt will print the error message so we just have to exit if this fa= ils. FIXME: Hmmm... in the case of gdb we need getopt to call print_filtered. */ if (sim_parse_args (sd, argv) !=3D SIM_RC_OK) {=20 free_state (sd); return 0; } > + sim_do_command(sd," memory region 0x00000000,0x4000000") ; > + sim_do_command(sd," memory region 0xE0000000,0x10000") ; instead, do: /* Allocate external memory if none specified by user. Use address 4 here in case the user wanted address 0 unmapped. */ if (sim_core_read_buffer (sd, NULL, read_map, &c, 4, 1) =3D=3D 0) { sim_do_command(sd, "memory region 0x00000000,0x4000000"); sim_do_command(sd, "memory region 0xE0000000,0x10000"); } notice the style fixes too then you'll want to do: /* Check for/establish the a reference program image. */ if (sim_analyze_program (sd, (STATE_PROG_ARGV (sd) !=3D NULL ? *STATE_PROG_ARGV (sd) : NULL), abfd) !=3D SIM_RC_OK) { free_state (sd); return 0; } > + myname =3D argv[0]; won't be needed after you drop sim_load (see below) > + callback =3D cb; you shouldn't need this -- the call to sim_state_alloc above already assign= ed=20 the callback > + if (kind =3D=3D SIM_OPEN_STANDALONE) > + issue_messages =3D 1; this variable looks pointless in this whole codebase -> delete > + set_initial_gprs (); /* Reset the GPR registers. */ you should move this to just before the return, and walk all cpus: /* CPU specific initialization. */ for (i =3D 0; i < MAX_NR_PROCESSORS; ++i) { SIM_CPU *cpu =3D STATE_CPU (sd, i); set_initial_gprs (sd, cpu); } > + if (sim_config (sd) !=3D SIM_RC_OK) > + { > + sim_module_uninstall (sd); call your new free_state() helper instead > + if (sim_post_argv_init (sd) !=3D SIM_RC_OK) > + { > + /* Uninstall the modules to avoid memory leaks, > + file descriptor leaks, etc. */ > + sim_module_uninstall (sd); delete the comment & call your new free_state() helper instead > +void > +sim_close (sd, quitting) > + SIM_DESC sd; > + int quitting; > +{ > + /* nothing to do */ > +} you should do: sim_module_uninstall (sd); > +static void > +load_dtb (SIM_DESC sd, const char *filename) unused -> delete > +SIM_RC > +sim_load (sd, prog, abfd, from_tty) add sim-hload.o to your SIM_OBJS and you get this for free > +SIM_RC > +sim_create_inferior (sd, prog_bfd, argv, env) > +{ > + sim_cpu *scpu =3D STATE_CPU (sd, 0); /* FIXME */ SIM_CPU *cpu =3D STATE_CPU (sd, 0); > + /* Set the initial register set. */ > + l =3D issue_messages; > + issue_messages =3D 0; > + set_initial_gprs (); > + issue_messages =3D l; you shouldn't need to do this. you already handled this in sim_open. > + // printf("start address %#lx\n", bfd_get_start_address (prog_bfd)); > + cpu.pc =3D bfd_get_start_address (prog_bfd); > + cpu.regs[31] =3D 0x00000; > + cpu.num_i =3D 0; > + cpu.cycles =3D 0; > + cpu.next_tick_cycle =3D 100000; you should instead do: /* Set the PC. */ if (abfd !=3D NULL) addr =3D bfd_get_start_address (abfd); else addr =3D 0; sim_pc_set (cpu, addr); if (STATE_OPEN_KIND (sd) =3D=3D SIM_OPEN_DEBUG) {=20 freeargv (STATE_PROG_ARGV (sd)); STATE_PROG_ARGV (sd) =3D dupargv (argv); } > +void > +sim_kill (sd) you shouldn't need this -> delete looks like i should clean up the tree too > +void > +sim_do_command (sd, cmd) once you switch to nrun.c/SIM_NEW_COMMON_OBJS, you get this for free > +void > +sim_set_callbacks (ptr) once you switch to nrun.c, you can delete this > --- /dev/null > +++ b/sim/ft32/sim-main.h > > +#define SIM_HAVE_BIENDIAN once you switch to nrun.c, you can delete this > + /* The following are internal simulator state variables: */ > +#define CIA_GET(CPU) ((CPU)->registers[PCIDX] + 0) > +#define CIA_SET(CPU,CIA) ((CPU)->registers[PCIDX] =3D (CIA)) you should define these to CPU_PC_SET/CPU_PC_GET instead. we should probab= ly=20 make that the default too so ports don't have to do it. > +/* To keep this default simulator simple, and fast, we use a direct > + vector of registers. The internal simulator engine then uses > + manifests to access the correct slot. */ gnu style says to put two spaces after periods that said, i have a hard time believing the speed thing here when your curr= ent=20 store/fetch register functions are doing byte level memory accesses > + unsigned_word registers[19]; per above, you should probably create dedicated fields for each register. = this=20 would also make it easier to debug assuming you have named registers like $= pc=20 and $fp and $sp. > --- /dev/null > +++ b/sim/ft32/sysdep.h looks like you copied & pasted this from another port. fairly certain you = don't=20 need or want any of it. just delete the file entirely. -mike --Y4/B1uKDHmWlpzsz Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature Content-length: 819 -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJU5YtiAAoJEEFjO5/oN/WBEDkQAIrjZjDqjWBslz4x4F11nEGD 9DxACs36EKnyb4jerGtQqy/g9SJWnyJjUr2jnSLCdByMuyyePxbSPhi8dVxh8b2n hVumj2nEJS5t/TivnKiT1ZL8DXQo4UtnqKcLr4VZTScySe5/XdN4iKeSIK9YmGw4 4XV9x0Cbrj5Ryhe8xAJUa7kxRca/8F5+ZUNrnG9A4usV7fJEE1rFEdCNYNbrZEBl xbZPph5dr2HD+cFrGfFw6l4wWx9dhj/a1XWnPfdbaMkOkCnv15zNhPQMKde4h/j9 LCFe6uDCcuOyjpCu/Ict8WX2CsUVkzKYlewnITW37KzHKkkJWcVAR2TIeypMTAnb +TsAKT7Wizzqtu97fzJ9W2knyYV4rR8/S3Zk4y/ZeIIL5bNEGjFR6ltEnBTogT/1 hSwPP8m0KUPKz+HANkH7n8DkWG1xjHsk6e+YEn+5S0YJOy6k+ye5g3kw1ZtiNOAJ TRPyJ9QnldHMMY/H72jeA0eNYS4SqiXVTSBeau8LV1fOqISfXISwuH9d/IXFdYy5 wZP4jTXU5BkG0j/36x5ariwoFHxf5MJaMOyCP0v+HL68Q69NzGjCNhX1RnJS6clB TQhyc2kfqFj/CVw+L4Ut+kC/A61gY+ja1YF1dpZWPVBkf4x8aMcAyHsFXL/DL23H LxVSn9mKH0eiXm6O6DOl =xJg/ -----END PGP SIGNATURE----- --Y4/B1uKDHmWlpzsz--