From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 120848 invoked by alias); 16 Mar 2015 08:25:55 -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 120822 invoked by uid 89); 16 Mar 2015 08:25:53 -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; Mon, 16 Mar 2015 08:25:50 +0000 Received: from vapier (localhost [127.0.0.1]) by smtp.gentoo.org (Postfix) with SMTP id 7D2DE340925; Mon, 16 Mar 2015 08:25:48 +0000 (UTC) Date: Mon, 16 Mar 2015 08:25:00 -0000 From: Mike Frysinger To: James Bowman Cc: "gdb-patches@sourceware.org" Subject: Re: [PATCH, FT32] gdb and sim support Message-ID: <20150316082548.GC12926@vapier> Mail-Followup-To: James Bowman , "gdb-patches@sourceware.org" References: <20150224045154.GE13523@vapier> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="m51xatjYGsM+13rf" Content-Disposition: inline In-Reply-To: X-IsSubscribed: yes X-SW-Source: 2015-03/txt/msg00442.txt.bz2 --m51xatjYGsM+13rf Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Content-length: 9642 On 10 Mar 2015 16:56, James Bowman wrote: > gdb/: > 2015-03-10 James Bowman >=20 > * Mainfile.in: Add FT32 entries. > * configure.tgt: Add FT32 entry. > * ft32-tdep.c,h: New file, FT32 target-dependent code. the file names should be spelled out someone else will have to approve the gdb/ parts > --- /dev/null > +++ b/gdb/ft32-tdep.c > > +struct ft32_frame_cache > +{ > + /* Base address. */ > + CORE_ADDR base; > + CORE_ADDR pc; > + LONGEST framesize; > + CORE_ADDR saved_regs[FT32_NUM_REGS]; > + CORE_ADDR saved_sp; > + bfd_boolean established; /* Has the new frame been LINKed */ comment needs to be tweaked -- punctuation and two spaces at the end i'd personally prefer it to be above the member rather than inline, but tha= t's=20 me ... > +static const char * ft32_register_names[] =3D one more const: static const char * const ft32_register_names[] =3D > +static CORE_ADDR > +ft32_analyze_prologue (CORE_ADDR start_addr, CORE_ADDR end_addr, > + struct ft32_frame_cache *cache, > + struct gdbarch *gdbarch) > +{ > ... > + /* It is a LINK */ style needs tweaking > +static CORE_ADDR > +ft32_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc) > +{ > ... > + /* No function symbol -- just return the PC. */ > + return (CORE_ADDR) pc; why the cast ? pc is already of type CORE_ADDR ... > +static struct value * > +ft32_frame_prev_register (struct frame_info *this_frame, > + void **this_prologue_cache, int regnum) > +{ > ... > + 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]); documenting that constant would be nice > --- /dev/null > +++ b/sim/ft32/Makefile.in > > +SIM_OBJS =3D \ > + $(SIM_NEW_COMMON_OBJS) \ indent the entries in this var with tabs > +SIM_RUN_OBJS =3D nrun.o i flipped the default in the latest tree so you can delete this now > --- /dev/null > +++ b/sim/ft32/interp.c > > +static uint32_t safe_addr (uint32_t dw, uint32_t ea) > +{ > + ea &=3D 0x1ffff; > + switch (dw) > + { the braces on the switch should be indented by two spaces -- the same as th= e=20 case labels. should fix in the whole file. switch (foo) { case 0: ... break; ... } > +static uint32_t cpu_mem_read (SIM_DESC sd, uint32_t dw, uint32_t ea) > +{ > ... > + switch (ea) > + { > + case 0x10000: > + case 0x10020: > + return getchar (); err what's this for ? you trying to simulate a UART or something ? > + case 0x1fff4: > + return (uint32_t)(cpu->state.cycles / 100); no need for the cast > + default: > + sim_io_eprintf (sd, "Illegal IO read address %08x, pc %#x\n", ea= , cpu->state.pc); > + exit (0); the sim should never exit directly. you should use sim_engine_halt instead. also, we generally want code to wrap at 80 cols. i'm not super strict on t= his=20 (like in the code later on with the packed bit shifts / case statements), b= ut=20 you should wrap in simple cases like this printf. you should scan the whol= e=20 file for other long lines. > + r =3D cpu->state.ram[ea]; > + if (1 <=3D dw) > + { > + r +=3D (cpu->state.ram[ea + 1] << 8); > + if (2 <=3D dw) > + { > + r +=3D cpu->state.ram[ea + 2] << 16; > + r +=3D cpu->state.ram[ea + 3] << 24; > + } > + } you shouldn't have a state.ram member ... the sim core takes care of that f= or=20 you. when you called "memory region" in sim_open, that initialized a chunk= of=20 ram for you. you can access that by using the sim_core_write_buffer and=20 sim_core_read_buffer helpers. > +static void ft32_push (SIM_DESC sd, uint32_t v) > +{ > + sim_cpu *cpu =3D STATE_CPU (sd, 0); > + cpu->state.regs[31] -=3D 4; rather than hardcode constants, you probably want to use symbols like=20 FT32_SP_REGNUM. or maybe use a union in your state: union { uint32_t raw[32]; struct { uint32_t r0; uint32_t r1; ... uint32_t fp; uint32_t sp; uint32_t pc; }; } regs; i'm just guessing at the reg names ... i have no idea what's going on :) > + cpu->state.regs[31] &=3D 0xffff; the CPU itself will automatically wrap the SP to 16bits ? that's kind of w= eird. > +static int nunsigned (int siz, int bits) > +{ > + int mask =3D (1L << siz) - 1; > + return bits & mask; > +} for all these little utility functions, it'd be nice to have a one line sen= tence=20 above them explaining what they're for. > +static > +void step_once (SIM_DESC sd) > +{ > + sim_cpu *cpu =3D STATE_CPU (sd, 0); > + address_word cia =3D CIA_GET (cpu); > + const char *ram_char =3D (const char *)cpu->state.ram; > + > +#define ILLEGAL_INSTRUCTION() \ > + sim_engine_halt (sd, cpu, NULL, insnpc, sim_signalled, SIM_SIGILL) > + > + { > + uint32_t inst; what is with this indented block ? doesn't seem like you need this at all.= =20=20 delete the braces and unindent everything by one level. > + /* Handle "call 8" (which is FT32's "break" equivalent) here */ use GNU style comments > + if (inst =3D=3D 0x00340002) > + { > + goto escape; > + } drop the braces for single statements > + dw =3D (inst >> FT32_FLD_DW_BIT) & ((1U << FT32_FLD= _DW_SIZ) - 1); > + cb =3D (inst >> FT32_FLD_CB_BIT) & ((1U << FT32_FLD= _CB_SIZ) - 1); > + r_d =3D (inst >> FT32_FLD_R_D_BIT) & ((1U << FT32_FL= D_R_D_SIZ) - 1); > + cr =3D (inst >> FT32_FLD_CR_BIT) & ((1U << FT32_FLD= _CR_SIZ) - 1); > + cv =3D (inst >> FT32_FLD_CV_BIT) & ((1U << FT32_FLD= _CV_SIZ) - 1); > + bt =3D (inst >> FT32_FLD_BT_BIT) & ((1U << FT32_FLD= _BT_SIZ) - 1); > + r_1 =3D (inst >> FT32_FLD_R_1_BIT) & ((1U << FT32_FL= D_R_1_SIZ) - 1); looks like you were trying to align, but the =3D here is slightly off ;) > + cpu->state.pc +=3D 4; > + switch (upper) > + { > + case FT32_PAT_TOC: when you hit 8 spaces of indentation, you're supposed to replace with a tab > + if (upper =3D=3D FT32_PAT_ALUOP) > + { > + cpu->state.regs[r_d] =3D result; > + } drop the braces > + else incorrect indentation for this else -- delete two spaces this seems to come up a couple of times ... you should search & fix them all > +int > +sim_write (SIM_DESC sd, > + SIM_ADDR addr, > + const unsigned char *buffer, > + int size) > +{ > + sim_cpu *cpu =3D STATE_CPU (sd, 0); > + > + if (addr < 0x800000) > + { > + sim_core_write_buffer (sd, cpu, write_map, buffer, addr, size); > + } > + else > + { > + int i; > + > + addr -=3D 0x800000; > + for (i =3D 0; i < size; i++) > + cpu_mem_write (sd, 0, addr + i, buffer[i]); > + } > + return size; > +} > + > +int > +sim_read (SIM_DESC sd, > + SIM_ADDR addr, > + unsigned char *buffer, > + int size) > +{ > + sim_cpu *cpu =3D STATE_CPU (sd, 0); > + > + if (addr < 0x800000) > + { > + sim_core_read_buffer (sd, cpu, read_map, buffer, addr, size); > + } > + else > + { > + int i; > + > + addr -=3D 0x800000; > + for (i =3D 0; i < size; i++) > + buffer[i] =3D cpu_mem_read (sd, 0, addr + i); > + } > + > + return size; > +} what's going on here with the magic 0x800000 address ? > +static uint32_t* > +ft32_lookup_register (SIM_DESC sd, int nr) spae before the * > + switch (nr) > + { > + case 0: > + return &cpu->state.regs[29]; > + break; > + case 1: > + return &cpu->state.regs[31]; > + break; > + case 31: > + return &cpu->state.regs[30]; > + break; > + case 32: > + return &cpu->state.pc; > + break; > + default: > + return &cpu->state.regs[nr - 2]; you probably want to check the value of nr to make sure it's in range and=20 abort() when it isn't > +sim_store_register (SIM_DESC sd, > +sim_fetch_register (SIM_DESC sd, you should change these to ft32_reg_{store,fetch}, and at the end of sim_op= en,=20 do something like: /* CPU specific initialization. */ for (i =3D 0; i < MAX_NR_PROCESSORS; ++i) { SIM_CPU *cpu =3D STATE_CPU (sd, i); CPU_REG_FETCH (cpu) =3D ft32_reg_fetch; CPU_REG_STORE (cpu) =3D ft32_reg_store; } then add sim-reg.o to your Makefile.in you probably should also implement ft32_pc_{fetch,store} and assign them: CPU_PC_FETCH (cpu) =3D ft32_pc_fetch; CPU_PC_STORE (cpu) =3D ft32_pc_store; > +SIM_DESC > +sim_open (SIM_OPEN_KIND kind, > + host_callback *cb, > + struct bfd *abfd, > + char **argv) > +{ > + char c; > + SIM_DESC sd =3D sim_state_alloc (kind, cb); > + > + /* 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 S= IM_RC_OK) > + { > + free_state (sd); > + return 0; > + } > + > + if (sim_pre_argv_init (sd, argv[0]) !=3D SIM_RC_OK) > + { > + free_state (sd); > + return 0; > + } after this, you should do: /* 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; } > + sd->base.prog_argv =3D argv + 1; why do you need to do this ? -mike --m51xatjYGsM+13rf Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature Content-length: 819 -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJVBpOMAAoJEEFjO5/oN/WB5cEQANUxWCmB7vvzJJp3krfB9r+8 BLeENeUWU7ZGEiOgkIWeixcPB5tA2Z2W/BME1WT2VoZ/ewOq3pDltpqrlS5Ef+Gk ahqpMUO2e0b0k3xLewuMfm9QI6xgcmt7mq95KJDnoQowc2gSN5l/a3CXR8HmgIgr m55mbkKtpfrGjt/mCHPi5UXyOGO4XHrc8pIzdRqggw+dH7zPnzZ/T2z1JIm1Y30j 0VVguW07mQ9gF3R/t7fCP6eEE6EUiYfMOBmU1AKoQgVzvdDLrnjbFR8BLMu1gD2m 2GLrrfO5w1buhCr4dhg7TTML0hqvrGBE8bGdW+TSLpnogFulhqF12PNvbTy7qg0l xegTzF0j/vbxZSNQl9a6ua9cj3qkOb2BK1hOAiDeXeHZl7dxs5w+zbRW53guJdNW o4ztDneOEuNtITn1TddQckT26GESYn+HcRM/NA3imzGzI33ukXVwvyKWyhQW9STN YrSOm+AgFydlbffKSGJnlgRcXq7Z4eYXfCrovrg3broZYSzbVEeL3yqujlq6aof+ L1Af4mEG2hPigozIotMS+995eICdJ6UxKd24p62df+T738ZfS5ijn3rk4Ol7cSLa 6UltLvVxz+9jqgwG8WDopvHTeuyi8c+rYTECopDp5b3mEUhOHv8uLZoPpJZPDjO6 KhwoS6ebwcySoa7KC1TX =GG9h -----END PGP SIGNATURE----- --m51xatjYGsM+13rf--