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.
	(s12z_frame_cache): Use opcodes API to interpret stack frame code.
---
 gdb/s12z-tdep.c | 227 ++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 164 insertions(+), 63 deletions(-)
  

On Thu, Apr 25, 2019 at 03:45:15PM +0200, John Darrington suggested:
     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.
     	(s12z_frame_cache): Use opcodes API to interpret stack frame code.
     ---
      gdb/s12z-tdep.c | 227 ++++++++++++++++++++++++++++++++++++++++----------------
      1 file changed, 164 insertions(+), 63 deletions(-)
     
     diff --git a/gdb/s12z-tdep.c b/gdb/s12z-tdep.c
     index cef92d8774..b80509f33e 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,97 @@ s12z_disassemble_info (struct gdbarch *gdbarch)
        return di;
      }
      
     +
     +struct mem_read_abstraction
     +{
     +  struct mem_read_abstraction_base base;
     +  bfd_vma memaddr;
     +  struct disassemble_info* info;
     +};
     +
     +static void
     +advance (struct mem_read_abstraction_base *b)
     +{
     +  struct mem_read_abstraction *mra = (struct mem_read_abstraction *) b;
     +  mra->memaddr++;
     +}
     +
     +static bfd_vma
     +posn (struct mem_read_abstraction_base *b)
     +{
     +  struct mem_read_abstraction *mra = (struct mem_read_abstraction *) b;
     +  return mra->memaddr;
     +}
     +
     +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  */
     +  else if (operands[0]->cl == OPND_CL_REGISTER_ALL16)
     +    stack_adjustment = 4 * 2; /* All 4 16 bit regs */
     +  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 (0); /* This cannot happen.  */
     +	  }
     +      }
     +  return stack_adjustment;
     +}
     +
      /* Initialize a prologue cache.  */
      
      static struct trad_frame_cache *
     @@ -234,73 +326,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
  

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

> On Thu, Apr 25, 2019 at 03:45:15PM +0200, John Darrington suggested:
>      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.

This sounds like a good change, and the core of the patch looks
reasonable, there's a few style issues that need to be addressed
though.

>      
>      gdb/ChangeLog:
>      	* s12z-tdep.c (push_pull_get_stack_adjustment): New function.
>      	(s12z_frame_cache): Use opcodes API to interpret stack frame
>      code.

It looks like most of the changes in this patch are missing from the
ChangeLog.  You need to document each new struct and function.

>      ---
>       gdb/s12z-tdep.c | 227 ++++++++++++++++++++++++++++++++++++++++----------------
>       1 file changed, 164 insertions(+), 63 deletions(-)
>      
>      diff --git a/gdb/s12z-tdep.c b/gdb/s12z-tdep.c
>      index cef92d8774..b80509f33e 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,97 @@ s12z_disassemble_info (struct gdbarch *gdbarch)
>         return di;
>       }
>       
>      +
>      +struct mem_read_abstraction
>      +{
>      +  struct mem_read_abstraction_base base;
>      +  bfd_vma memaddr;
>      +  struct disassemble_info* info;
>      +};

The structure needs a descriptive comment at the top, and ideally each
field documented with a comment.

>      +
>      +static void
>      +advance (struct mem_read_abstraction_base *b)
>      +{
>      +  struct mem_read_abstraction *mra = (struct mem_read_abstraction *) b;
>      +  mra->memaddr++;
>      +}

All functions should have a header comment.

>      +
>      +static bfd_vma
>      +posn (struct mem_read_abstraction_base *b)
>      +{
>      +  struct mem_read_abstraction *mra = (struct mem_read_abstraction *) b;
>      +  return mra->memaddr;
>      +}
>      +
>      +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  */

Missing a '.' at the end of this comment, and in a few other comments too.

>      +  else if (operands[0]->cl == OPND_CL_REGISTER_ALL16)
>      +    stack_adjustment = 4 * 2; /* All 4 16 bit regs */
>      +  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 (0); /* This cannot happen.  */

You can use gdb_assert_not_reached here.

>      +	  }
>      +      }
>      +  return stack_adjustment;
>      +}
>      +
>       /* Initialize a prologue cache.  */
>       
>       static struct trad_frame_cache *
>      @@ -234,73 +326,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

Thanks,
Andrew