Re: [PATCH 2/2] [AArch64] Recognize STR instruction in prologue

[Pedro Alves <palves at redhat dot com>]

On 11/29/2016 02:12 PM, Yao Qi wrote:
> This patch teaches GDB AArch64 backend to recognize STR instructions
> in prologue, like 'str x19, [sp, #-48]!' or 'str w0, [sp, #44]'.
> The unit test is added too.
> 
> gdb:
> 
> 2016-11-28  Yao Qi  <yao.qi@linaro.org>
> 
> 	* aarch64-tdep.c (aarch64_analyze_prologue): Recognize STR
> 	instruction.
> 	(aarch64_analyze_prologue_test): More tests.
> ---
>  gdb/aarch64-tdep.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index b10002a..fc68b8a 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -397,6 +397,35 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
>  	    regs[rn] = pv_add_constant (regs[rn], imm);
>  
>  	}
> +      else if ((inst.opcode->iclass == ldst_imm9 /* Signed immediate.  */
> +		|| (inst.opcode->iclass == ldst_pos /* Unsigned immediate.  */
> +		    && (inst.opcode->op == OP_STR_POS
> +			|| inst.opcode->op == OP_STRF_POS)))
> +	       && inst.operands[1].addr.base_regno == AARCH64_SP_REGNUM
> +	       && strcmp ("str", inst.opcode->name) == 0)
> +	{
> +	  /* STR (immediate) */
> +	  unsigned int rt = inst.operands[0].reg.regno;
> +	  int32_t imm = inst.operands[1].addr.offset.imm;
> +	  unsigned rn = inst.operands[1].addr.base_regno;
> +	  int is64
> +	    = (aarch64_get_qualifier_esize (inst.operands[0].qualifier) == 8);
> +	  gdb_assert (inst.operands[0].type == AARCH64_OPND_Rt
> +		      || inst.operands[0].type == AARCH64_OPND_Ft);
> +
> +	  if (inst.operands[0].type == AARCH64_OPND_Ft)
> +	    {
> +	      /* Only bottom 64-bit of each V register (D register) need
> +		 to be preserved.  */
> +	      gdb_assert (inst.operands[0].qualifier == AARCH64_OPND_QLF_S_D);
> +	      rt += AARCH64_X_REGISTER_COUNT;
> +	    }
> +
> +	  pv_area_store (stack, pv_add_constant (regs[rn], imm),
> +			 is64 ? 8 : 4, regs[rt]);
> +	  if (inst.operands[1].addr.writeback)
> +	    regs[rn] = pv_add_constant (regs[rn], imm);
> +	}
>        else if (inst.opcode->iclass == testbranch)
>  	{
>  	  /* Stop analysis on branch.  */
> @@ -540,6 +569,45 @@ aarch64_analyze_prologue_test (void)
>  
>        SELF_CHECK (cache.saved_regs[i + regnum + AARCH64_D0_REGNUM].addr == -1);
>      }
> +
> +  /* Second test.  */

I'd suggest putting each test in its own scope, to avoid accidental
local variable reuse (or unintentionally reusing internal state of
internal variables).  Like

 /* Second test.  */
 {
   /* test code here */
 }

Also, I think you should expand that "second test" comment.  It should
inform the reader about what is important about the test.  In this
case, it should probably say something about "STR".

Likewise for the first test.

> +  cache.saved_regs = trad_frame_alloc_saved_regs (gdbarch);
> +
> +  instruction_reader_test test2 {
> +      0xf81d0ff3, /* str	x19, [sp, #-48]! */
> +      0xb9002fe0, /* str	w0, [sp, #44] */
> +      0xf90013e1, /* str	x1, [sp, #32]*/
> +      0xfd000fe0, /* str	d0, [sp, #24] */
> +      0xaa0203f3, /* mov	x19, x2 */
> +      0xf94013e0, /* ldr	x0, [sp, #32] */
> +   };

Indentation looks odd here too.

Thanks,
Pedro Alves