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

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

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