From: Mike Frysinger <vapier@gentoo.org>
To: James Bowman <james.bowman@ftdichip.com>
Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: Re: [PATCH, FT32] gdb and sim support
Date: Mon, 16 Mar 2015 08:25:00 -0000 [thread overview]
Message-ID: <20150316082548.GC12926@vapier> (raw)
In-Reply-To: <CA9BBF0458F83C4F9051448B941B57D11722A513@glaexch1>
[-- Attachment #1: Type: text/plain, Size: 9823 bytes --]
On 10 Mar 2015 16:56, James Bowman wrote:
> gdb/:
> 2015-03-10 James Bowman <james.bowman@ftdichip.com>
>
> * 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 that's
me ...
> +static const char * ft32_register_names[] =
one more const:
static const char * const ft32_register_names[] =
> +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] != REG_UNAVAIL)
> + return frame_unwind_got_memory (this_frame, regnum,
> + 0x800000 | cache->saved_regs[regnum]);
documenting that constant would be nice
> --- /dev/null
> +++ b/sim/ft32/Makefile.in
>
> +SIM_OBJS = \
> + $(SIM_NEW_COMMON_OBJS) \
indent the entries in this var with tabs
> +SIM_RUN_OBJS = 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 &= 0x1ffff;
> + switch (dw)
> + {
the braces on the switch should be indented by two spaces -- the same as the
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 this
(like in the code later on with the packed bit shifts / case statements), but
you should wrap in simple cases like this printf. you should scan the whole
file for other long lines.
> + r = cpu->state.ram[ea];
> + if (1 <= dw)
> + {
> + r += (cpu->state.ram[ea + 1] << 8);
> + if (2 <= dw)
> + {
> + r += cpu->state.ram[ea + 2] << 16;
> + r += cpu->state.ram[ea + 3] << 24;
> + }
> + }
you shouldn't have a state.ram member ... the sim core takes care of that for
you. when you called "memory region" in sim_open, that initialized a chunk of
ram for you. you can access that by using the sim_core_write_buffer and
sim_core_read_buffer helpers.
> +static void ft32_push (SIM_DESC sd, uint32_t v)
> +{
> + sim_cpu *cpu = STATE_CPU (sd, 0);
> + cpu->state.regs[31] -= 4;
rather than hardcode constants, you probably want to use symbols like
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] &= 0xffff;
the CPU itself will automatically wrap the SP to 16bits ? that's kind of weird.
> +static int nunsigned (int siz, int bits)
> +{
> + int mask = (1L << siz) - 1;
> + return bits & mask;
> +}
for all these little utility functions, it'd be nice to have a one line sentence
above them explaining what they're for.
> +static
> +void step_once (SIM_DESC sd)
> +{
> + sim_cpu *cpu = STATE_CPU (sd, 0);
> + address_word cia = CIA_GET (cpu);
> + const char *ram_char = (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.
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 == 0x00340002)
> + {
> + goto escape;
> + }
drop the braces for single statements
> + dw = (inst >> FT32_FLD_DW_BIT) & ((1U << FT32_FLD_DW_SIZ) - 1);
> + cb = (inst >> FT32_FLD_CB_BIT) & ((1U << FT32_FLD_CB_SIZ) - 1);
> + r_d = (inst >> FT32_FLD_R_D_BIT) & ((1U << FT32_FLD_R_D_SIZ) - 1);
> + cr = (inst >> FT32_FLD_CR_BIT) & ((1U << FT32_FLD_CR_SIZ) - 1);
> + cv = (inst >> FT32_FLD_CV_BIT) & ((1U << FT32_FLD_CV_SIZ) - 1);
> + bt = (inst >> FT32_FLD_BT_BIT) & ((1U << FT32_FLD_BT_SIZ) - 1);
> + r_1 = (inst >> FT32_FLD_R_1_BIT) & ((1U << FT32_FLD_R_1_SIZ) - 1);
looks like you were trying to align, but the = here is slightly off ;)
> + cpu->state.pc += 4;
> + switch (upper)
> + {
> + case FT32_PAT_TOC:
when you hit 8 spaces of indentation, you're supposed to replace with a tab
> + if (upper == FT32_PAT_ALUOP)
> + {
> + cpu->state.regs[r_d] = 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 = STATE_CPU (sd, 0);
> +
> + if (addr < 0x800000)
> + {
> + sim_core_write_buffer (sd, cpu, write_map, buffer, addr, size);
> + }
> + else
> + {
> + int i;
> +
> + addr -= 0x800000;
> + for (i = 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 = STATE_CPU (sd, 0);
> +
> + if (addr < 0x800000)
> + {
> + sim_core_read_buffer (sd, cpu, read_map, buffer, addr, size);
> + }
> + else
> + {
> + int i;
> +
> + addr -= 0x800000;
> + for (i = 0; i < size; i++)
> + buffer[i] = 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
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_open,
do something like:
/* CPU specific initialization. */
for (i = 0; i < MAX_NR_PROCESSORS; ++i)
{
SIM_CPU *cpu = STATE_CPU (sd, i);
CPU_REG_FETCH (cpu) = ft32_reg_fetch;
CPU_REG_STORE (cpu) = 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) = ft32_pc_fetch;
CPU_PC_STORE (cpu) = ft32_pc_store;
> +SIM_DESC
> +sim_open (SIM_OPEN_KIND kind,
> + host_callback *cb,
> + struct bfd *abfd,
> + char **argv)
> +{
> + char c;
> + SIM_DESC sd = 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) != SIM_RC_OK)
> + {
> + free_state (sd);
> + return 0;
> + }
> +
> + if (sim_pre_argv_init (sd, argv[0]) != 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 fails.
FIXME: Hmmm... in the case of gdb we need getopt to call
print_filtered. */
if (sim_parse_args (sd, argv) != SIM_RC_OK)
{
free_state (sd);
return 0;
}
> + sd->base.prog_argv = argv + 1;
why do you need to do this ?
-mike
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
next prev parent reply other threads:[~2015-03-16 8:25 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-02-03 4:06 James Bowman
2015-02-10 6:27 ` James Bowman
2015-02-17 10:21 ` Mike Frysinger
2015-02-19 7:06 ` Mike Frysinger
[not found] ` <ORIGINAL-RELEASE-1424557036-20150219070610.GI544@vapier>
2015-02-23 10:40 ` James Bowman
2015-02-24 4:52 ` Mike Frysinger
2015-03-10 16:56 ` James Bowman
2015-03-16 8:25 ` Mike Frysinger [this message]
2015-03-19 15:26 ` James Bowman
2015-03-19 18:52 ` Mike Frysinger
2015-03-20 2:57 ` James Bowman
2015-03-20 5:18 ` Mike Frysinger
2015-03-20 5:19 ` Mike Frysinger
2015-03-23 19:21 ` James Bowman
2015-03-26 7:10 ` Mike Frysinger
2015-03-26 19:05 ` James Bowman
2015-03-16 16:38 ` Mike Frysinger
2015-03-17 17:36 ` Joel Brobecker
2015-03-19 15:21 ` James Bowman
2015-03-19 18:54 ` Mike Frysinger
2015-03-20 3:01 ` James Bowman
2015-03-20 3:47 ` Mike Frysinger
2015-03-20 12:46 ` Joel Brobecker
2015-03-20 15:47 ` Mike Frysinger
2015-03-20 15:50 ` Joel Brobecker
2015-03-22 22:33 ` Mike Frysinger
2015-03-23 12:36 ` Joel Brobecker
2015-03-23 19:15 ` James Bowman
2015-03-23 23:04 ` Joel Brobecker
2015-03-28 6:21 ` Mike Frysinger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150316082548.GC12926@vapier \
--to=vapier@gentoo.org \
--cc=gdb-patches@sourceware.org \
--cc=james.bowman@ftdichip.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).