[PATCH v3 1/2] Record ARM THUMB2 PLD/PLI cache instructions

[PATCH v3 1/2] Record ARM THUMB2 PLD/PLI cache instructions

[Trent Piepho]

These weren't decoded correctly and trigger an unknown instruction error
when recording.  The ARM format was handled, but not the 32-bit THUMB2
format.

Since they are only hints that may affect cache state, there is nothing
to record.

gdb/ChangeLog
2018-09-28  Trent Piepho  <tpiepho@impinj.com>

        PR gdb/23725
        * gdb/arm-tdep.c (thumb2_record_ld_mem_hints): Decode thumb2 PLD/PLI
---
 gdb/arm-tdep.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index c3280ee211..90936ada8e 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -12683,6 +12683,14 @@ thumb2_record_ld_mem_hints (insn_decode_record *thumb2_insn_r)
                 record_buf);
       return ARM_RECORD_SUCCESS;
     }
+  else
+    {
+      if (bits (thumb2_insn_r->arm_insn, 20, 22) == 0x1)
+	{
+	  /* Handle PLD, PLI affect only caches, so nothing to record */
+	  return ARM_RECORD_SUCCESS;
+	}
+    }
 
   return ARM_RECORD_FAILURE;
 }
-- 
2.14.4

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

[Trent Piepho]

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 */
+
+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 */
+    }
+}
+
 /* 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 */
   if (ARM_PC_REGNUM != reg_rt)
     {
-      record_buf[0] = reg_rt;
-      record_buf[1] = reg_rn;
-      record_buf[2] = ARM_PS_REGNUM;
-      thumb2_insn_r->reg_rec_count = 3;
+      if (mode != -1)
+        {
+	  record_buf[0] = reg_rt;
+	  record_buf[1] = reg_rn;
+	  record_buf[2] = ARM_PS_REGNUM;
+	  thumb2_insn_r->reg_rec_count = 3;
 
-      REG_ALLOC (thumb2_insn_r->arm_regs, thumb2_insn_r->reg_rec_count,
-                record_buf);
-      return ARM_RECORD_SUCCESS;
+	  REG_ALLOC (thumb2_insn_r->arm_regs, thumb2_insn_r->reg_rec_count,
+		    record_buf);
+	  return ARM_RECORD_SUCCESS;
+	}
     }
   else
     {
       if (bits (thumb2_insn_r->arm_insn, 20, 22) == 0x1)
 	{
 	  /* Handle PLD, PLI affect only caches, so nothing to record */
-	  return ARM_RECORD_SUCCESS;
+	  if (mode == 1 || mode == 2 || mode == 3 || mode == 7)
+	    return ARM_RECORD_SUCCESS;
 	}
     }
 
-- 
2.14.4

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

[Simon Marchi]

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

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

[Trent Piepho]

On Tue, 2018-10-02 at 13:39 -0400, Simon Marchi wrote:
> 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?

Most useful reference I've found is a file named Thumb-
2SupplementReferenceManual.pdf, easily located via google.  Section
3.3.3 describes all possible "Load and store single data item, and
memory hints" thumb2 instructions.  The figure there made it easy to
come up with a decision tree to arrive at which valid encoding, if any,
was in use.

> > +
> > +  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?

No official names that I know of.  The numbers appear in a table
listing all possible instruction formats in the document I described
above.  E.g. "LDR, LDRB, ..., (immediate offset)" must use format 2. 
"PLI, PLD" may use 1, 2, 3, or 7.  And so on.  The comments I added by
each return statement are how the format is described.

I could condense the logic in this function to one that returns a
boolean, based on if the current version would return 1,2,3,7 or not. 
Basically, make it check solely for a valid PLD/PLI instruction.  But
since the format being decoded here is common to the other
instructions, this lets me enhance the decoding of them too.  And the
code is easier to follow if it's more directly analogous to the
documentation, which is clearly part of the design of the existing
recording code.  It's not a coincidence that thumb2_record_ld_mem_hints
records every instruction appearing in exactly one section of the
reference manual.