Re: [PATCH v4] [gdb] Add basic Z80 CPU support

[Sergey Belyashov <sergey.belyashov@gmail.com>]

Hi Simon,
Thank you.

You can safely remove these comments, but keep last one - it is reserved
for GameBoy Z80 & Z80 Next CPUs.

Sergey Belyashov


вт, 13 июля 2021 г., 0:37 Simon Marchi <simon.marchi@polymtl.ca>:

> Hi Sergey,
>
> I'm fine with merging this pretty much as-is.  I certainly won't have
> the time to do an in-depth / functional review, but I trust that if it
> is useful to you, it can be for others.  I can take care of rebasing and
> doing the fixups to make sure it compiles, as well as make a few style
> changes.
>
> I'm just wondering about these, if you could help me:
>
> > diff --git a/gdb/features/z80-cpu.xml b/gdb/features/z80-cpu.xml
> > new file mode 100644
> > index 0000000000..d8093d68b9
> > --- /dev/null
> > +++ b/gdb/features/z80-cpu.xml
> > @@ -0,0 +1,34 @@
> > +<?xml version="1.0"?>
> > +<!-- Copyright (C) 2020 Free Software Foundation, Inc.
> > +
> > +     Copying and distribution of this file, with or without
> modification,
> > +     are permitted in any medium without royalty provided the copyright
> > +     notice and this notice are preserved.  -->
> > +
> > +<!DOCTYPE feature SYSTEM "gdb-target.dtd">
> > +<feature name="org.gnu.gdb.z80.cpu">
> > +  <flags id="af_flags" size="2">
> > +    <field name="C" start="0" end="0"/>
> > +    <field name="N" start="1" end="1"/>
> > +    <field name="P/V" start="2" end="2"/>
> > +    <field name="F3" start="3" end="3"/>
> > +    <field name="H" start="4" end="4"/>
> > +    <field name="F5" start="5" end="5"/>
> > +    <field name="Z" start="6" end="6"/>
> > +    <field name="S" start="7" end="7"/>
> > +  </flags>
> > +  <reg name="af" bitsize="16" type="af_flags"/>
> > +  <reg name="bc" bitsize="16" type="uint16"/>
> > +  <reg name="de" bitsize="16" type="data_ptr"/>
> > +  <reg name="hl" bitsize="16" type="data_ptr"/>
> > +  <reg name="sp" bitsize="16" type="data_ptr" />
> > +  <reg name="pc" bitsize="32" type="code_ptr" />
> > +  <reg name="ix" bitsize="16" type="data_ptr"/>
> > +  <reg name="iy" bitsize="16" type="data_ptr"/>
> > +  <reg name="af'" bitsize="16" type="af_flags"/>
> > +  <reg name="bc'" bitsize="16" type="uint16"/>
> > +  <reg name="de'" bitsize="16" type="data_ptr"/>
> > +  <reg name="hl'" bitsize="16" type="data_ptr"/>
> > +  <reg name="ir" bitsize="16" type="uint16"/>
> > +<!--  <reg name="sps" bitsize="16" type="uint16"/> -->
>
> Is this commented out on purpose, can we remove it?
>
> > +static int
> > +z80_scan_prologue (struct gdbarch *gdbarch, CORE_ADDR pc_beg, CORE_ADDR
> pc_end,
> > +                struct z80_unwind_cache *info)
> > +{
> > +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> > +  int addr_len = gdbarch_tdep (gdbarch)->addr_length;
> > +  gdb_byte prologue[32]; /* max prologue is 24 bytes: __interrupt with
> local array */
> > +  int pos = 0;
> > +  int len;
> > +  int reg;
> > +  CORE_ADDR value;
> > +
> > +  len = pc_end - pc_beg;
> > +  if (len > (int)sizeof(prologue))
> > +    len = sizeof(prologue);
> > +
> > +  read_memory (pc_beg, prologue, len);
> > +
> > +  /* stage0: check for series of POPs and then PUSHs */
> > +  if ((reg = z80_is_pop_rr(prologue, &pos)))
> > +    {
> > +      int i;
> > +      int size = pos;
> > +      gdb_byte regs[8]; /* Z80 have only 6 register pairs */
> > +      regs[0] = reg & 0xff;
> > +      for (i = 1; i < 8 && (regs[i] = z80_is_pop_rr (&prologue[pos],
> &size));
> > +        ++i, pos += size);
> > +      /* now we expect series of PUSHs in reverse order */
> > +      for (--i; i >= 0 && regs[i] == z80_is_push_rr (&prologue[pos],
> &size);
> > +        --i, pos += size);
> > +      if (i == -1 && pos > 0)
> > +     info->prologue_type.load_args = 1;
> > +      else
> > +     pos = 0;
> > +    }
> > +  /* stage1: check for __interrupt handlers and __critical functions */
> > +  else if (!memcmp (&prologue[pos], "\355\127\363\365", 4))
> > +    { /* ld a, i; di; push af */
> > +      info->prologue_type.critical = 1;
> > +      pos += 4;
> > +      info->state_size += addr_len;
> > +    }
> > +  else if (!memcmp (&prologue[pos], "\365\305\325\345\375\345", 6))
> > +    { /* push af; push bc; push de; push hl; push iy */
> > +      info->prologue_type.interrupt = 1;
> > +      pos += 6;
> > +      info->state_size += addr_len * 5;
> > +    }
> > +
> > +  /* stage2: check for FP saving scheme */
> > +  if (prologue[pos] == 0xcd) /* call nn */
> > +    {
> > +      struct bound_minimal_symbol msymbol;
> > +      msymbol = lookup_minimal_symbol ("__sdcc_enter_ix", NULL, NULL);
> > +      if (msymbol.minsym)
> > +     {
> > +       value = BMSYMBOL_VALUE_ADDRESS (msymbol);
> > +       if (value == extract_unsigned_integer (&prologue[pos+1],
> addr_len, byte_order))
> > +         {
> > +           pos += 1 + addr_len;
> > +           info->prologue_type.fp_sdcc = 1;
> > +         }
> > +     }
> > +    }
> > +  else if (!memcmp (&prologue[pos], "\335\345\335\041\000\000",
> 4+addr_len) &&
> > +        !memcmp (&prologue[pos+4+addr_len], "\335\071\335\371", 4))
> > +    { /* push ix; ld ix, #0; add ix, sp; ld sp, ix */
> > +      pos += 4 + addr_len + 4;
> > +      info->prologue_type.fp_sdcc = 1;
> > +    }
> > +  else if (!memcmp (&prologue[pos], "\335\345", 2))
> > +    { /* push ix */
> > +      pos += 2;
> > +      info->prologue_type.fp_sdcc = 1;
> > +    }
> > +
> > +  /* stage3: check for local variables allocation */
> > +  switch (prologue[pos])
> > +    {
> > +      case 0xf5: /* push af */
> > +     info->size = 0;
> > +     while (prologue[pos] == 0xf5)
> > +       {
> > +         info->size += addr_len;
> > +         pos++;
> > +       }
> > +     if (prologue[pos] == 0x3b) /* dec sp */
> > +       {
> > +         info->size++;
> > +         pos++;
> > +       }
> > +     break;
> > +      case 0x3b: /* dec sp */
> > +     info->size = 0;
> > +     while (prologue[pos] == 0x3b)
> > +       {
> > +         info->size++;
> > +         pos++;
> > +       }
> > +     break;
> > +      case 0x21: /*ld hl, -nn */
> > +     if (prologue[pos+addr_len] == 0x39 && prologue[pos+addr_len] >=
> 0x80 &&
> > +         prologue[pos+addr_len+1] == 0xf9)
> > +       { /* add hl, sp; ld sp, hl */
> > +         info->size = -extract_signed_integer(&prologue[pos+1],
> addr_len, byte_order);
> > +         pos += 1 + addr_len + 2;
> > +       }
> > +     break;
> > +      case 0xfd: /* ld iy, -nn */
> > +     if (prologue[pos+1] == 0x21 && prologue[pos+1+addr_len] >= 0x80 &&
> > +         !memcmp (&prologue[pos+2+addr_len], "\375\071\375\371", 4))
> > +       {
> > +         info->size = -extract_signed_integer(&prologue[pos+2],
> addr_len, byte_order);
> > +         pos += 2 + addr_len + 4;
> > +       }
> > +     break;
> > +      case 0xed: /* check for lea xx, ix - n */
> > +     switch (prologue[pos+1])
> > +       {
> > +       case 0x22: /* lea hl, ix - n */
> > +         if (prologue[pos+2] >= 0x80 && prologue[pos+3] == 0xf9)
> > +           { /* ld sp, hl */
> > +             info->size = -extract_signed_integer(&prologue[pos+2], 1,
> byte_order);
> > +             pos += 4;
> > +           }
> > +         break;
> > +       case 0x55: /* lea iy, ix - n */
> > +         if (prologue[pos+2] >= 0x80 && prologue[pos+3] == 0xfd &&
> > +             prologue[pos+4] == 0xf9)
> > +           { /* ld sp, iy */
> > +             info->size = -extract_signed_integer(&prologue[pos+2], 1,
> byte_order);
> > +             pos += 5;
> > +           }
> > +         break;
> > +       }
> > +       break;
> > +    }
> > +  len = 0;
> > +  //info->saved_regs[Z80_PC_REGNUM].addr = len++
>
> Is this commented out on purpose?  Can we remove it?
>
> > +/* returns pointer to instruction information structure corresponded to
> opcode
> > +   in buf */
> > +static const struct insn_info *
> > +z80_get_insn_info (struct gdbarch *gdbarch, const gdb_byte *buf, int
> *size)
> > +{
> > +  int code;
> > +  const struct insn_info *info;
> > +  unsigned long mach = gdbarch_bfd_arch_info (gdbarch)->mach;
> > +  *size = 0;
> > +  switch (mach)
> > +    {
> > +    case bfd_mach_ez80_z80:
> > +      info = &ez80_main_insn_table[4]; /* skip force_nops */
> > +      break;
> > +    case bfd_mach_ez80_adl:
> > +      info = &ez80_adl_main_insn_table[4]; /* skip force_nops */
> > +      break;
> > +/*
> > +    case bfd_mach_gbz80:
> > +      info = &gbz80_main_insn_table[0];
> > +      break;
> > +    case bfd_mach_z80n:
> > +      info = &z80n_main_insn_table[0];
> > +      break;
> > +*/
>
> Is this commented out on purpose, can we remove it?
>
> Thanks,
>
> Simon
>