Re: [PATCH v3 2/2] Check thumb2 load/store and cache hit addressing mode

[Simon Marchi <simon dot marchi at polymtl dot ca>]

This is a bit out of my comfort zone.  I'm reading the code with the 
reference manual on the side, and don't see the mapping.  I'd prefer if 
somebody from ARM took a look.  In the mean time, could you point me to 
the relevant sections in the manual for this patch?

I'll just note some formatting comments for now.

On 2018-10-01 18:26, Trent Piepho wrote:
> There are a number of different addressing forms available for these
> thumb2 instructions.  However, not all modes are valid for all
> instructions, nor is every possible bit pattern a valid mode.
>
> PLD/PLI are not that complex so verify that one of the valid modes for
> those instructions was used.
>
> Other instructions are checked for a valid mode encoding, but not
> necessary that the particular mode is valid for the full instruction.
>
> gdb/ChangeLog:
>
> 2018-10-01  Trent Piepho  <tpiepho@impinj.com>
>
>         * arm-tdep.c (thumb2_ld_mem_hint_mode): Decode addressing mode.
>         (thumb2_record_ld_mem_hints): Check addressing mode.
> ---
>
> Changes from v2:
> * Fix logic flaw that allowed invalid non PLI/D instructions to be
>   considered PLI/D instructions.
>
>  gdb/arm-tdep.c | 69 
> +++++++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 61 insertions(+), 8 deletions(-)
>
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 90936ada8e..f7b51d4805 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -12661,6 +12661,51 @@ thumb2_record_str_single_data
> (insn_decode_record *thumb2_insn_r)
>    return ARM_RECORD_SUCCESS;
>  }
>
> +
> +/* Decode addressing mode of thumb2 load and store single data item,
> +   and memory hints */

End the comment with a dot and two spaces.

> +
> +static int
> +thumb2_ld_mem_hint_mode (insn_decode_record *thumb2_insn_r)
> +{
> +  /* Check Rn = 0b1111 */
> +  if (bits (thumb2_insn_r->arm_insn, 16, 19) == 0xf)
> +    {
> +      if (bit (thumb2_insn_r->arm_insn, 20) == 1)
> +	return 1; /* PC +/- imm12 */
> +      else
> +	return -1; /* reserved */
> +    }
> +
> +  /* Check U = 1 */
> +  if (bit (thumb2_insn_r->arm_insn, 23) == 1)
> +    return 2; /* Rn + imm2 */
> +
> +  /* Check op2[5] = 0 */
> +  if (bit (thumb2_insn_r->arm_insn, 11) == 0)
> +    {
> +      if (bits (thumb2_insn_r->arm_insn, 6, 10) == 0)
> +	return 7; /* Rn + shifted register */
> +      return -1; /* reserved */
> +    }
> +
> +  switch (bits (thumb2_insn_r->arm_insn, 8, 10))
> +    {
> +      case 0x4:
> +	return 3; /* Rn - imm8 */
> +      case 0x6:
> +	return 4; /* Rn + imm8, User privilege */
> +      case 0x1:
> +      case 0x3:
> +	return 5; /* Rn post-indexed by +/- imm8 */
> +      case 0x5:
> +      case 0x7:
> +	return 6; /* Rn pre-indexed by +/- imm8 */
> +      default:
> +	return -1; /* reserved */

Do these modes have names?  Could we define an enum instead of using 
magic numbers?

> +    }
> +}
> +
>  /* Handler for thumb2 load memory hints instructions.  */
>
>  static int
> @@ -12668,27 +12713,35 @@ thumb2_record_ld_mem_hints
> (insn_decode_record *thumb2_insn_r)
>  {
>    uint32_t record_buf[8];
>    uint32_t reg_rt, reg_rn;
> +  uint32_t mode;
>
>    reg_rt = bits (thumb2_insn_r->arm_insn, 12, 15);
>    reg_rn = bits (thumb2_insn_r->arm_insn, 16, 19);
> +  mode = thumb2_ld_mem_hint_mode(thumb2_insn_r);
>
> +  /* This does not check every possible addressing mode + data size
> +   * combination for validity */

Remove the asterisk at the beginning of the second line and finish the 
sentence with a period and two spaces:

/* This does not check every possible addressing mode + data size
   combination for validity.  */

Thanks,

Simon