GDB (s12z): Improve reliability of the stack unwinder.

  Previously, the stack unwinder searched through consecutive bytes for values
which it thought might be the start of a stack mutating operation.
This was error prone, because such bytes could also be the operands of other
instructions.  This change uses the opcodes api to interpret the code in each
frame.

gdb/ChangeLog:
	* s12z-tdep.c (push_pull_get_stack_adjustment): New function.
	(advance, posn, abstract_read_memory): New functions.
	[struct mem_read_abstraction]: New struct.
	(s12z_frame_cache): Use opcodes API to interpret stack frame code.
---
 gdb/s12z-tdep.c | 234 +++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 171 insertions(+), 63 deletions(-)
  

* John Darrington <john@darrington.wattle.id.au> [2019-05-09 12:22:19 +0200]:

> Previously, the stack unwinder searched through consecutive bytes for values
> which it thought might be the start of a stack mutating operation.
> This was error prone, because such bytes could also be the operands of other
> instructions.  This change uses the opcodes api to interpret the code in each
> frame.
> 
> gdb/ChangeLog:
> 	* s12z-tdep.c (push_pull_get_stack_adjustment): New function.
> 	(advance, posn, abstract_read_memory): New functions.
> 	[struct mem_read_abstraction]: New struct.
> 	(s12z_frame_cache): Use opcodes API to interpret stack frame
> code.

Looks good to me.

Thanks,
Andrew


> ---
>  gdb/s12z-tdep.c | 234 +++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 171 insertions(+), 63 deletions(-)
> 
> diff --git a/gdb/s12z-tdep.c b/gdb/s12z-tdep.c
> index cef92d8774..b549862e84 100644
> --- a/gdb/s12z-tdep.c
> +++ b/gdb/s12z-tdep.c
> @@ -30,6 +30,7 @@
>  #include "opcode/s12z.h"
>  #include "trad-frame.h"
>  #include "remote.h"
> +#include "opcodes/s12z-opc.h"
>  
>  /* Two of the registers included in S12Z_N_REGISTERS are
>     the CCH and CCL "registers" which are just views into
> @@ -162,6 +163,104 @@ s12z_disassemble_info (struct gdbarch *gdbarch)
>    return di;
>  }
>  
> +
> +/* A struct (based on mem_read_abstraction_base) to read memory
> +   through the disassemble_info API.  */
> +struct mem_read_abstraction
> +{
> +  struct mem_read_abstraction_base base; /* The parent struct.  */
> +  bfd_vma memaddr;                  /* Where to read from.  */
> +  struct disassemble_info* info;  /* The disassember  to use for reading.  */
> +};
> +
> +/* Advance the reader by one byte.  */
> +static void
> +advance (struct mem_read_abstraction_base *b)
> +{
> +  struct mem_read_abstraction *mra = (struct mem_read_abstraction *) b;
> +  mra->memaddr++;
> +}
> +
> +/* Return the current position of the reader.  */
> +static bfd_vma
> +posn (struct mem_read_abstraction_base *b)
> +{
> +  struct mem_read_abstraction *mra = (struct mem_read_abstraction *) b;
> +  return mra->memaddr;
> +}
> +
> +/* Read the N bytes at OFFSET  using B.  The bytes read are stored in BYTES.
> +   It is the caller's responsibility to ensure that this is of at least N
> +   in size.  */
> +static int
> +abstract_read_memory (struct mem_read_abstraction_base *b,
> +		      int offset,
> +		      size_t n, bfd_byte *bytes)
> +{
> +  struct mem_read_abstraction *mra = (struct mem_read_abstraction *) b;
> +
> +  int status =
> +    (*mra->info->read_memory_func) (mra->memaddr + offset,
> +				    bytes, n, mra->info);
> +
> +  if (status != 0)
> +    {
> +      (*mra->info->memory_error_func) (status, mra->memaddr, mra->info);
> +      return -1;
> +    }
> +
> +  return 0;
> +}
> +
> +
> +/* Return the stack adjustment caused by a push or pull instruction.  */
> +static int
> +push_pull_get_stack_adjustment (int n_operands,
> +				struct operand *const *operands)
> +{
> +  int stack_adjustment = 0;
> +  gdb_assert (n_operands > 0);
> +  if (operands[0]->cl == OPND_CL_REGISTER_ALL)
> +    stack_adjustment = 26;  /* All the regs are involved.  */
> +  else if (operands[0]->cl == OPND_CL_REGISTER_ALL16)
> +    stack_adjustment = 4 * 2; /* All four 16 bit regs are involved.  */
> +  else
> +    for (int i = 0; i < n_operands; ++i)
> +      {
> +	if (operands[i]->cl != OPND_CL_REGISTER)
> +	  continue; /* I don't think this can ever happen.  */
> +	const struct register_operand *op
> +	  = (const struct register_operand *) operands[i];
> +	switch (op->reg)
> +	  {
> +	  case REG_X:
> +	  case REG_Y:
> +	    stack_adjustment += 3;
> +	    break;
> +	  case REG_D7:
> +	  case REG_D6:
> +	    stack_adjustment += 4;
> +	    break;
> +	  case REG_D2:
> +	  case REG_D3:
> +	  case REG_D4:
> +	  case REG_D5:
> +	    stack_adjustment += 2;
> +	    break;
> +	  case REG_D0:
> +	  case REG_D1:
> +	  case REG_CCL:
> +	  case REG_CCH:
> +	    stack_adjustment += 1;
> +	    break;
> +	  default:
> +	    gdb_assert_not_reached ("Invalid register in push/pull operation.");
> +	    break;
> +	  }
> +      }
> +  return stack_adjustment;
> +}
> +
>  /* Initialize a prologue cache.  */
>  
>  static struct trad_frame_cache *
> @@ -234,73 +333,82 @@ s12z_frame_cache (struct frame_info *this_frame, void **prologue_cache)
>    CORE_ADDR addr = start_addr; /* Where we have got to?  */
>    int frame_size = 0;
>    int saved_frame_size = 0;
> -  while (this_pc > addr)
> -    {
> -      struct disassemble_info di = s12z_disassemble_info (gdbarch);
> -
> -      /* No instruction can be more than 11 bytes long, I think.  */
> -      gdb_byte buf[11];
>  
> -      int nb = print_insn_s12z (addr, &di);
> -      gdb_assert (nb <= 11);
> +  struct disassemble_info di = s12z_disassemble_info (gdbarch);
>  
> -      if (0 != target_read_code (addr, buf, nb))
> -        memory_error (TARGET_XFER_E_IO, addr);
>  
> -      if (buf[0] == 0x05)        /* RTS */
> -        {
> -          frame_size = saved_frame_size;
> -        }
> -      /* Conditional Branches.   If any of these are encountered, then
> -         it is likely that a RTS will terminate it.  So we need to save
> -         the frame size so it can be restored.  */
> -      else if ( (buf[0] == 0x02)      /* BRSET */
> -                || (buf[0] == 0x0B)   /* DBcc / TBcc */
> -                || (buf[0] == 0x03))  /* BRCLR */
> -        {
> -          saved_frame_size = frame_size;
> -        }
> -      else if (buf[0] == 0x04)        /* PUL/ PSH .. */
> -        {
> -          bool pull = buf[1] & 0x80;
> -          int stack_adjustment = 0;
> -          if (buf[1] & 0x40)
> -            {
> -              if (buf[1] & 0x01) stack_adjustment += 3;  /* Y */
> -              if (buf[1] & 0x02) stack_adjustment += 3;  /* X */
> -              if (buf[1] & 0x04) stack_adjustment += 4;  /* D7 */
> -              if (buf[1] & 0x08) stack_adjustment += 4;  /* D6 */
> -              if (buf[1] & 0x10) stack_adjustment += 2;  /* D5 */
> -              if (buf[1] & 0x20) stack_adjustment += 2;  /* D4 */
> -            }
> -          else
> -            {
> -              if (buf[1] & 0x01) stack_adjustment += 2;  /* D3 */
> -              if (buf[1] & 0x02) stack_adjustment += 2;  /* D2 */
> -              if (buf[1] & 0x04) stack_adjustment += 1;  /* D1 */
> -              if (buf[1] & 0x08) stack_adjustment += 1;  /* D0 */
> -              if (buf[1] & 0x10) stack_adjustment += 1;  /* CCL */
> -              if (buf[1] & 0x20) stack_adjustment += 1;  /* CCH */
> -            }
> +  struct mem_read_abstraction mra;
> +  mra.base.read = (int (*)(mem_read_abstraction_base*,
> +			   int, size_t, bfd_byte*)) abstract_read_memory;
> +  mra.base.advance = advance ;
> +  mra.base.posn = posn;
> +  mra.info = &di;
>  
> -          if (!pull)
> -            stack_adjustment = -stack_adjustment;
> -          frame_size -= stack_adjustment;
> -        }
> -      else if (buf[0] == 0x0a)   /* LEA S, (xxx, S) */
> -        {
> -          if (0x06 == (buf[1] >> 4))
> -            {
> -              int simm = (signed char) (buf[1] & 0x0F);
> -              frame_size -= simm;
> -            }
> -        }
> -      else if (buf[0] == 0x1a)   /* LEA S, (S, xxxx) */
> -        {
> -	  int simm = (signed char) buf[1];
> -	  frame_size -= simm;
> -        }
> -      addr += nb;
> +  while (this_pc > addr)
> +    {
> +      enum optr optr = OP_INVALID;
> +      short osize;
> +      int n_operands = 0;
> +      struct operand *operands[6];
> +      mra.memaddr = addr;
> +      int n_bytes =
> +	decode_s12z (&optr, &osize, &n_operands, operands,
> +		     (mem_read_abstraction_base *) &mra);
> +
> +      switch (optr)
> +	{
> +	case OP_tbNE:
> +	case OP_tbPL:
> +	case OP_tbMI:
> +	case OP_tbGT:
> +	case OP_tbLE:
> +	case OP_dbNE:
> +	case OP_dbEQ:
> +	case OP_dbPL:
> +	case OP_dbMI:
> +	case OP_dbGT:
> +	case OP_dbLE:
> +	  /* Conditional Branches.   If any of these are encountered, then
> +	     it is likely that a RTS will terminate it.  So we need to save
> +	     the frame size so it can be restored.  */
> +	  saved_frame_size = frame_size;
> +	  break;
> +	case OP_rts:
> +	  /* Restore the frame size from a previously saved value.  */
> +	  frame_size = saved_frame_size;
> +	  break;
> +	case OP_push:
> +	  frame_size += push_pull_get_stack_adjustment (n_operands, operands);
> +	  break;
> +	case OP_pull:
> +	  frame_size -= push_pull_get_stack_adjustment (n_operands, operands);
> +	  break;
> +	case OP_lea:
> +	  if (operands[0]->cl == OPND_CL_REGISTER)
> +	    {
> +	      int reg = ((struct register_operand *) (operands[0]))->reg;
> +	      if ((reg == REG_S) && (operands[1]->cl == OPND_CL_MEMORY))
> +		{
> +		  const struct memory_operand *mo
> +		    = (const struct memory_operand * ) operands[1];
> +		  if (mo->n_regs == 1 && !mo->indirect
> +		      && mo->regs[0] == REG_S
> +		      && mo->mutation == OPND_RM_NONE)
> +		    {
> +		      /* LEA S, (xxx, S) -- Decrement the stack.   This is
> +			 almost certainly the start of a frame.  */
> +		      int simm = (signed char)  mo->base_offset;
> +		      frame_size -= simm;
> +		    }
> +		}
> +	    }
> +	  break;
> +	default:
> +	  break;
> +	}
> +      addr += n_bytes;
> +      for (int o = 0; o < n_operands; ++o)
> +	free (operands[o]);
>      }
>  
>    /* If the PC has not actually got to this point, then the frame
> -- 
> 2.11.0
>