Re: [PATCH 1/5] Support for recording arm/thumb mode coprocessor instructions

[Omair Javaid <omair dot javaid at linaro dot org>]

On Wed 09 Apr 2014 03:18:21 PM PKT, Will Newton wrote:
> On 22 February 2014 16:48, Omair Javaid <omair.javaid@linaro.org> wrote:
>> gdb:
>>
>> 2013-02-22  Omair Javaid  <omair.javaid@linaro.org>
>>
>>         * arm-tdep.c (arm_record_coproc_data_proc): Updated.
>>         (arm_record_asimd_vfp_coproc): New function.
>>         (thumb2_record_coproc_insn): New function.
>>         (thumb2_record_decode_insn_handler): Updated.
>>         (decode_insn): Updated.
>
> The changelog here could be improved I think, e.g. something line
> "Handle VFP, SIMD and coprocessor instructions." rather than
> "Updated,"
>
> With that and the nit below this looks ok to me.
>
>> ---
>>  gdb/arm-tdep.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 112 insertions(+), 8 deletions(-)
>>
>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
>> index 12254ec..5eb7b12 100644
>> --- a/gdb/arm-tdep.c
>> +++ b/gdb/arm-tdep.c
>> @@ -11915,20 +11915,81 @@ arm_record_unsupported_insn (insn_decode_record *arm_insn_r)
>>    return -1;
>>  }
>>
>> +/* Handling opcode 110 insns.  */
>> +
>> +static int
>> +arm_record_asimd_vfp_coproc (insn_decode_record *arm_insn_r)
>> +{
>> +  uint32_t op, op1, op1_sbit, op1_ebit, coproc;
>> +
>> +  coproc = bits (arm_insn_r->arm_insn, 8, 11);
>> +  op1 = bits (arm_insn_r->arm_insn, 20, 25);
>> +  op1_sbit = bit (arm_insn_r->arm_insn, 24);
>> +  op1_ebit = bit (arm_insn_r->arm_insn, 20);
>> +  op = bit (arm_insn_r->arm_insn, 4);
>> +
>> +  if ((coproc & 0x0e) == 0x0a)
>> +    {
>> +      /* Handle extension register ld/st instructions.  */
>> +      if (!(op1 & 0x20))
>> +        return arm_record_unsupported_insn (arm_insn_r);
>> +
>> +      /* 64-bit transfers between arm core and extension registers.  */
>> +      if ((op1 & 0x3e) == 0x04)
>> +        return arm_record_unsupported_insn (arm_insn_r);
>> +    }
>> +  else
>> +    {
>> +      /* Handle coprocessor ld/st instructions.  */
>> +      if (!(op1 & 0x3a))
>> +        {
>> +          /* Store.  */
>> +          if (!op1_ebit)
>> +            return arm_record_unsupported_insn (arm_insn_r);
>> +          else
>> +            /* Load.  */
>> +            return arm_record_unsupported_insn (arm_insn_r);
>> +        }
>> +
>> +      /* Move to coprocessor from two arm core registers.  */
>> +      if (op1 == 0x4)
>> +        return arm_record_unsupported_insn (arm_insn_r);
>> +
>> +      /* Move to two arm core registers from coprocessor.  */
>> +      if (op1 == 0x5)
>> +        {
>> +          uint32_t reg_t[2];
>> +
>> +          reg_t[0] = bits (arm_insn_r->arm_insn, 12, 15);
>> +          reg_t[1] = bits (arm_insn_r->arm_insn, 16, 19);
>> +          arm_insn_r->reg_rec_count = 2;
>> +
>> +          REG_ALLOC (arm_insn_r->arm_regs, arm_insn_r->reg_rec_count, reg_t);
>> +          return 0;
>> +       }
>> +    }
>> +  return arm_record_unsupported_insn (arm_insn_r);
>> +}
>> +
>>  /* Handling opcode 111 insns.  */
>>
>>  static int
>>  arm_record_coproc_data_proc (insn_decode_record *arm_insn_r)
>>  {
>> +  uint32_t op, op1_sbit, op1_ebit, coproc;
>>    struct gdbarch_tdep *tdep = gdbarch_tdep (arm_insn_r->gdbarch);
>>    struct regcache *reg_cache = arm_insn_r->regcache;
>>    uint32_t ret = 0; /* function return value: -1:record failure ;  0:success  */
>
> It looks like ret is now pretty much unused and can be removed.
>
>>    ULONGEST u_regval = 0;
>>
>>    arm_insn_r->opcode = bits (arm_insn_r->arm_insn, 24, 27);
>> +  coproc = bits (arm_insn_r->arm_insn, 8, 11);
>> +  op1_sbit = bit (arm_insn_r->arm_insn, 24);
>> +  op1_ebit = bit (arm_insn_r->arm_insn, 20);
>> +  op = bit (arm_insn_r->arm_insn, 4);
>>
>>    /* Handle arm SWI/SVC system call instructions.  */
>> -  if (15 == arm_insn_r->opcode)
>> +  if (op1_sbit)
>>      {
>>        if (tdep->arm_syscall_record != NULL)
>>          {
>> @@ -11942,20 +12003,52 @@ arm_record_coproc_data_proc (insn_decode_record *arm_insn_r)
>>              regcache_raw_read_unsigned (reg_cache, 7, &svc_number);
>>
>>            ret = tdep->arm_syscall_record (reg_cache, svc_number);
>> +          return ret;
>>          }
>>        else
>>          {
>>            printf_unfiltered (_("no syscall record support\n"));
>> -          ret = -1;
>> +          return -1;
>>          }
>>      }
>> +
>> +  if ((coproc & 0x0e) == 0x0a)
>> +    {
>> +      /* VFP data-processing instructions.  */
>> +      if (!op1_sbit && !op)
>> +        return arm_record_unsupported_insn (arm_insn_r);
>> +
>> +      /* Advanced SIMD, VFP instructions.  */
>> +      if (!op1_sbit && op)
>> +        return arm_record_unsupported_insn (arm_insn_r);
>> +    }
>>    else
>>      {
>> -      arm_record_unsupported_insn (arm_insn_r);
>> -      ret = -1;
>> +      /* Coprocessor data operations.  */
>> +      if (!op1_sbit && !op)
>> +        return arm_record_unsupported_insn (arm_insn_r);
>> +
>> +      /* Move to Coprocessor from ARM core register.  */
>> +      if (!op1_sbit && !op1_ebit && op)
>> +        return arm_record_unsupported_insn (arm_insn_r);
>> +
>> +      /* Move to arm core register from coprocessor.  */
>> +      if (!op1_sbit && op1_ebit && op)
>> +        {
>> +          uint32_t record_buf[1];
>> +
>> +          record_buf[0] = bits (arm_insn_r->arm_insn, 12, 15);
>> +          if (record_buf[0] == 15)
>> +            record_buf[0] = ARM_PS_REGNUM;
>> +
>> +          arm_insn_r->reg_rec_count = 1;
>> +          REG_ALLOC (arm_insn_r->arm_regs, arm_insn_r->reg_rec_count,
>> +                     record_buf);
>> +          return 0;
>> +        }
>>      }
>>
>> -  return ret;
>> +  return arm_record_unsupported_insn (arm_insn_r);
>>  }
>>
>>  /* Handling opcode 000 insns.  */
>> @@ -12871,6 +12964,17 @@ thumb2_record_lmul_lmla_div (insn_decode_record *thumb2_insn_r)
>>    return ARM_RECORD_SUCCESS;
>>  }
>>
>> +/* Record handler for thumb32 coprocessor instructions.  */
>> +
>> +static int
>> +thumb2_record_coproc_insn (insn_decode_record *thumb2_insn_r)
>> +{
>> +  if (bit (thumb2_insn_r->arm_insn, 25))
>> +    return arm_record_coproc_data_proc (thumb2_insn_r);
>> +  else
>> +    return arm_record_asimd_vfp_coproc (thumb2_insn_r);
>> +}
>> +
>>  /* Decodes thumb2 instruction type and invokes its record handler.  */
>>
>>  static unsigned int
>> @@ -12902,7 +13006,7 @@ thumb2_record_decode_insn_handler (insn_decode_record *thumb2_insn_r)
>>        else if (op2 & 0x40)
>>          {
>>            /* Co-processor instructions.  */
>> -          arm_record_unsupported_insn (thumb2_insn_r);
>> +          return thumb2_record_coproc_insn (thumb2_insn_r);
>>          }
>>      }
>>    else if (op1 == 0x02)
>> @@ -12968,7 +13072,7 @@ thumb2_record_decode_insn_handler (insn_decode_record *thumb2_insn_r)
>>        else if (op2 & 0x40)
>>          {
>>            /* Co-processor instructions.  */
>> -          return arm_record_unsupported_insn (thumb2_insn_r);
>> +          return thumb2_record_coproc_insn (thumb2_insn_r);
>>          }
>>     }
>>
>> @@ -13012,7 +13116,7 @@ decode_insn (insn_decode_record *arm_record, record_type_t record_type,
>>      arm_record_ld_st_reg_offset,        /* 011.  */
>>      arm_record_ld_st_multiple,          /* 100.  */
>>      arm_record_b_bl,                    /* 101.  */
>> -    arm_record_unsupported_insn,        /* 110.  */
>> +    arm_record_asimd_vfp_coproc,        /* 110.  */
>>      arm_record_coproc_data_proc         /* 111.  */
>>    };
>>
>> --
>> 1.8.3.2
>>
>
>
>
ping! Are there any further comments on this patch?