Re: [patch, nios2] clean up prologue/epilogue detection code

[Yao Qi <yao at codesourcery dot com>]

Sandra Loosemore <sandra@codesourcery.com> writes:

Hi, Sandra,

> In this patch, all explicit matching of opcodes and masks in
> nios2_in_epilogue_p, nios2_analyze_prologue, and nios2_get_net_pc is
> eliminated in favor of invoking the disassembler (via
> nios2_find_opcode_hash) to do that part.  A new set of helper
> functions are introduced to extract instruction operands according to
> the format of the matched instruction opcode.  Future ISA extensions
> are likely to include multiple encodings of some logical operations,
> such as add or subtract, so this organization will allow those details
> to be consolidated in the new helper functions instead of handled
> inline at call sites for those functions.  Also, organizing the
> analysis by what the instructions being examined conceptually do
> instead of by their format and encoding makes it easier to understand.

Great, these helpers look very clear to me.

>
> This patch also fixes an outstanding bug in the code.  Formerly, there
> were assumptions that the prologue and epilogue could only include one
> stack adjustment each.  This used to be true of code emitted by GCC
> except for functions with stack frame too large to be addressed via a
> 16-bit offset.  A change made to GCC earlier this year (r208472) to
> correct an ABI conformance issue also means that many functions with
> frame pointers now have an additional stack pointer adjustment too.
> In lifting the restriction, I made the prologue analyzer a little
> smarter in differentiating valid prologue stack adjustments
> (decrementing the SP, setting the FP from the SP) from those that can
> only appear in epilogues (incrementing the SP, setting the SP from the
> FP).

These fixes should be in separate patches.  I'd like to see this patch
is split to a series which includes the following patches,

 1. add new helpers and rewrite existing functions with these new
 helpers,
 2. permit more than one stack adjustment instruction to appear in
 epilogue
 3. detect some additional prologue instruction patterns,

> diff --git a/gdb/nios2-tdep.c b/gdb/nios2-tdep.c
> index 1b647ac..816b17b 100644
> --- a/gdb/nios2-tdep.c
> +++ b/gdb/nios2-tdep.c
> @@ -275,46 +275,398 @@ nios2_init_cache (struct nios2_unwind_cache *cache, CORE_ADDR pc)
>    nios2_setup_default (cache);
>  }
>  
> +/* Read and identify an instruction at PC.  If INSNP is non-null,
> +   store the instruction word into that location.  Return the opcode
> +   pointer or NULL if the memory couldn't be read or disassembled.  */

There should be an empty line between the end of comment and the function.

> +static const struct nios2_opcode *
> +nios2_fetch_insn (struct gdbarch *gdbarch, CORE_ADDR pc,
> +		  unsigned int *insnp)
> +{
> +  LONGEST memword;
> +  unsigned long mach = gdbarch_bfd_arch_info (gdbarch)->mach;
> +  unsigned int insn;
> +
> +  if (!safe_read_memory_integer (pc, NIOS2_OPCODE_SIZE,
> +				 gdbarch_byte_order (gdbarch), &memword))
> +    return NULL;
> +
> +  insn = (unsigned int) memword;
> +  if (insnp)
> +    *insnp = insn;
> +  return nios2_find_opcode_hash (insn, mach);
> +}
> +
> +
> +/* Match and disassemble an ADD-type instruction, with 3 register operands.
> +   Returns true on success, and fills in the operand pointers.  */
> +static int
> +nios2_match_add (uint32_t insn,
> +		 const struct nios2_opcode *op,
> +		 unsigned long mach,
> +		 int *ra, int *rb, int *rc)

Is there any reason you put these parameters in different lines?  We can
place them on the same line like:

static int
nios2_match_add (uint32_t insn, const struct nios2_opcode *op,
		 unsigned long mach, int *ra, int *rb, int *rc)

then the function can be shorter.

> +{
> +  if (op->match == MATCH_R1_ADD || op->match == MATCH_R1_MOV)
> +    {
> +      *ra = GET_IW_R_A (insn);
> +      *rb = GET_IW_R_B (insn);
> +      *rc = GET_IW_R_C (insn);
> +      return 1;
> +    }
> +  return 0;
> +}
> +


> +
>  /* Helper function to identify when we're in a function epilogue;
>     that is, the part of the function from the point at which the
> -   stack adjustment is made, to the return or sibcall.  On Nios II,
> -   we want to check that the CURRENT_PC is a return-type instruction
> -   and that the previous instruction is a stack adjustment.
> -   START_PC is the beginning of the function in question.  */
> -
> +   stack adjustments are made, to the return or sibcall.
> +   Note that we may have several stack adjustment instructions, and
> +   this function needs to test whether the stack teardown has already
> +   started before current_pc, not whether it has completed.  */
>  static int
>  nios2_in_epilogue_p (struct gdbarch *gdbarch,
>  		     CORE_ADDR current_pc,
>  		     CORE_ADDR start_pc)
>  {
> -  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  unsigned long mach = gdbarch_bfd_arch_info (gdbarch)->mach;
> +  /* Maximum number of possibly-epilogue instructions to check.
> +     Note that this number should not be too large, else we can
> +     potentially end up iterating through unmapped memory.  */
> +  int ninsns, max_insns = 5;
> +  unsigned int insn;
> +  const struct nios2_opcode *op = NULL;
> +  unsigned int uimm;
> +  int imm;
> +  int ra, rb, rc;
> +  enum branch_condition cond;
> +  CORE_ADDR pc;
>  
>    /* There has to be a previous instruction in the function.  */
> -  if (current_pc > start_pc)
> -    {
> +  if (current_pc <= start_pc)
> +    return 0;
>  
> -      /* Check whether the previous instruction was a stack
> -	 adjustment.  */
> -      unsigned int insn
> -        = read_memory_unsigned_integer (current_pc - NIOS2_OPCODE_SIZE,
> -					NIOS2_OPCODE_SIZE, byte_order);
> +  /* Find the previous instruction before current_pc.  */
> +  op = nios2_fetch_insn (gdbarch, current_pc - NIOS2_OPCODE_SIZE, &insn);
> +  if (op == NULL)
> +    return 0;
>  
> -      if ((insn & 0xffc0003c) == 0xdec00004	/* ADDI sp, sp, */
> -	  || (insn & 0xffc1ffff) == 0xdec1883a	/* ADD  sp, sp, */
> -	  || (insn & 0xffc0003f) == 0xdec00017)	/* LDW  sp, constant(sp) */
> -	{
> -	  /* Then check if it's followed by a return or a tail
> -	     call.  */
> -          insn = read_memory_unsigned_integer (current_pc, NIOS2_OPCODE_SIZE,
> -					       byte_order);
> -
> -	  if (insn == 0xf800283a			/* RET */
> -	      || insn == 0xe800083a			/* ERET */
> -	      || (insn & 0x07ffffff) == 0x0000683a	/* JMP */
> -	      || (insn & 0xffc0003f) == 6)		/* BR */
> -	    return 1;
> -	}
> +  /* Beginning with the previous instruction we just located, check whether
> +     we are in a sequence of at least one stack adjustment instruction.
> +     Possible instructions here include:
> +	 ADDI sp, sp, n
> +	 ADD sp, sp, rn
> +	 LDW sp, n(sp)  */
> +  for (ninsns = 0, pc = current_pc; ninsns < max_insns; ninsns++)
> +    {
> +      int ok = 0;

An empty line is needed between declaration and statement.

> +      if (nios2_match_addi (insn, op, mach, &ra, &rb, &imm))
> +	ok = (rb == NIOS2_SP_REGNUM);
> +      else if (nios2_match_add (insn, op, mach, &ra, &rb, &rc))
> +	ok = (rc == NIOS2_SP_REGNUM);
> +      else if (nios2_match_ldw (insn, op, mach, &ra, &rb, &imm))
> +	ok = (rb == NIOS2_SP_REGNUM);
> +      if (!ok)
> +	break;
> +      /* Fetch the next insn.  */
> +      op = nios2_fetch_insn (gdbarch, pc, &insn);
> +      if (op == NULL)
> +	return 0;
> +      pc += op->size;
>      }
> +
> +  /* No stack adjustments found.  */
> +  if (ninsns == 0)
> +    return 0;
> +
> +  /* We found a whole lot of stack adjustments.  Be safe, tell GDB that
> +     the epilogue stack unwind is in progress even if we didn't see a
> +     return yet.  */
> +  if (ninsns == max_insns)
> +    return 1;

I am confused by the comments and code.  Infer from your code above that
GCC emits arbitrary number of sp-adjusting continuous instructions in
epilogue, is that true?

> +
> +  /* The next instruction following the stack adjustments must be a
> +     return, jump, or unconditional branch.  */
> +  if (nios2_match_jmpr (insn, op, mach, &ra)
> +      || nios2_match_jmpi (insn, op, mach, &uimm)
> +      || (nios2_match_branch (insn, op, mach, &ra, &rb, &imm, &cond)
> +	  && cond == branch_none))
> +    return 1;
> +
>    return 0;
>  }
>  

Looks the epilogue detection isn't precise nor accurate.  Supposing we
have an epilogue like this below,

   1. ADDI sp, sp, n
   2. RET

if pc points at insn #1, nios2_in_epilogue_p returns true, which isn't
precise, because the stack frame isn't destroyed at the moment pc points
at insn #1.  gdbarch in_function_epilogue_p's name is misleading, and
better to be renamed to stack_frame_destroyed_p (it's on my todo list).

if pc points at insn #2, nios2_in_epilogue_p returns false, which isn't
accurate.

Probably, we need do the forward scan first, skipping arbitrary number of
sp-adjusting instructions until a return or jump, and then do a backward
scan to match a sp-adjusting instruction.  In this case, return true,
otherwise, return false.  Hopefully, this fixes some sw watchpoint
related test fails.

>  nios2_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *bp_addr,
>  			  int *bp_size)
>  {
> -  /* break encoding: 31->27  26->22  21->17  16->11 10->6 5->0 */
> -  /*                 00000   00000   0x1d    0x2d   11111 0x3a */
> -  /*                 00000   00000   11101   101101 11111 111010 */
> -  /* In bytes:       00000000 00111011 01101111 11111010 */
> -  /*                 0x0       0x3b    0x6f     0xfa */
> -  static const gdb_byte breakpoint_le[] = {0xfa, 0x6f, 0x3b, 0x0};
> -  static const gdb_byte breakpoint_be[] = {0x0, 0x3b, 0x6f, 0xfa};
> -
> -  enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
> -
> -  *bp_size = 4;
> -  if (gdbarch_byte_order_for_code (gdbarch) == BFD_ENDIAN_BIG)
> -    return breakpoint_be;
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);

Why do we change from byte_order_for_code to byte_order?

> +  unsigned long mach = gdbarch_bfd_arch_info (gdbarch)->mach;
> +
> +  /* R1 break encoding:
> +     ((0x1e << 17) | (0x34 << 11) | (0x1f << 6) | (0x3a << 0))
> +     0x003da7fa */
> +  static const gdb_byte r1_breakpoint_le[] = {0xfa, 0xa7, 0x3d, 0x0};
> +  static const gdb_byte r1_breakpoint_be[] = {0x0, 0x3d, 0xa7, 0xfa};
> +  *bp_size = NIOS2_OPCODE_SIZE;
> +  if (byte_order == BFD_ENDIAN_BIG)
> +    return r1_breakpoint_be;
>    else
> -    return breakpoint_le;
> +    return r1_breakpoint_le;
>  }
>  

-- 
Yao (éå)