diff mbox series

[v3,2/5] gdb/aarch64: Add record support for MOPS instructions.

Message ID 20240510052408.2173579-3-thiago.bauermann@linaro.org
State New
Headers show
Series Add support for AArch64 MOPS instructions | expand

Commit Message

Thiago Jung Bauermann May 10, 2024, 5:24 a.m. UTC 
There are two kinds of MOPS instructions: set instructions and copy
instructions.  Within each group there are variants with minor
differences in how they read or write to memory — e.g., non-temporal
read and/or write, unprivileged read and/or write and permutations of
those — but they work in the same way in terms of the registers and
regions of memory that they modify.

PR tdep/31666
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31666
---
 gdb/aarch64-tdep.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

No change since v1.

Comments

Luis Machado May 10, 2024, 12:59 p.m. UTC | #1 
Hi,

Just a minor suggestion. Otherwise the patch looks OK.

On 5/10/24 06:24, Thiago Jung Bauermann wrote:
> There are two kinds of MOPS instructions: set instructions and copy
> instructions.  Within each group there are variants with minor
> differences in how they read or write to memory — e.g., non-temporal
> read and/or write, unprivileged read and/or write and permutations of
> those — but they work in the same way in terms of the registers and
> regions of memory that they modify.
> 
> PR tdep/31666
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31666
> ---
>  gdb/aarch64-tdep.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
> 
> No change since v1.
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 05ecd421cd0e..a9a67107675c 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -5188,6 +5188,86 @@ aarch64_record_asimd_load_store (aarch64_insn_decode_record *aarch64_insn_r)
>    return AARCH64_RECORD_SUCCESS;
>  }
>  
> +/* Record handler for Memory Copy and Memory Set instructions.  */
> +
> +static unsigned int
> +aarch64_record_memcopy_memset (aarch64_insn_decode_record *aarch64_insn_r)
> +{
> +  if (record_debug)
> +    debug_printf ("Process record: memory copy and memory set\n");
> +
> +  uint8_t op1 = bits (aarch64_insn_r->aarch64_insn, 22, 23);
> +  uint8_t op2 = bits (aarch64_insn_r->aarch64_insn, 12, 15);
> +  uint32_t reg_rd = bits (aarch64_insn_r->aarch64_insn, 0, 4);
> +  uint32_t reg_rn = bits (aarch64_insn_r->aarch64_insn, 5, 9);
> +  uint32_t record_buf[3];
> +  uint64_t record_buf_mem[4];
> +
> +  if (op1 != 3)
> +    {
> +      /* Copy instructions.  */
> +      uint32_t reg_rs = bits (aarch64_insn_r->aarch64_insn, 16, 20);
> +
> +      record_buf[0] = reg_rd;
> +      record_buf[1] = reg_rn;
> +      record_buf[2] = reg_rs;
> +      aarch64_insn_r->reg_rec_count = 3;
> +
> +      ULONGEST dest_addr;
> +      regcache_raw_read_unsigned (aarch64_insn_r->regcache, reg_rd,
> +				  &dest_addr);
> +      ULONGEST source_addr;
> +      regcache_raw_read_unsigned (aarch64_insn_r->regcache, reg_rs,
> +				  &source_addr);
> +      LONGEST length;
> +      regcache_raw_read_signed (aarch64_insn_r->regcache, reg_rn,
> +				&length);
> +
> +      /* In a processor using algorithm option A, the length in Rn has an
> +	 inverted sign.  */
> +      if (length < 0)
> +	length *= -1;
> +
> +      record_buf_mem[0] = length;
> +      record_buf_mem[1] = dest_addr;
> +      record_buf_mem[2] = length;
> +      record_buf_mem[3] = source_addr;
> +      aarch64_insn_r->mem_rec_count = 2;
> +    }
> +  else if ((op1 == 3 && op2 < 12) || (op1 == 3 && op2 < 12))
> +    {
> +      /* Set instructions.  */
> +      record_buf[0] = reg_rd;
> +      record_buf[1] = reg_rn;
> +      aarch64_insn_r->reg_rec_count = 2;
> +
> +      ULONGEST address;
> +      regcache_raw_read_unsigned (aarch64_insn_r->regcache, reg_rd,
> +				  &address);
> +
> +      LONGEST length;
> +      regcache_raw_read_signed (aarch64_insn_r->regcache, reg_rn,
> +				&length);
> +
> +      /* In a processor using algorithm option B, the length in Rn has an
> +	 inverted sign.  */
> +      if (length < 0)
> +	length *= -1;
> +
> +      record_buf_mem[0] = length;
> +      record_buf_mem[1] = address;
> +      aarch64_insn_r->mem_rec_count = 1;
> +    }

Looks like both cases above could potentially share some code in a common path. Would be nice
to do so if it doesn't make the code all awkward. If it does, then this is fine.

> +  else
> +    return AARCH64_RECORD_UNKNOWN;
> +
> +  MEM_ALLOC (aarch64_insn_r->aarch64_mems, aarch64_insn_r->mem_rec_count,
> +	     record_buf_mem);
> +  REG_ALLOC (aarch64_insn_r->aarch64_regs, aarch64_insn_r->reg_rec_count,
> +	     record_buf);
> +  return AARCH64_RECORD_SUCCESS;
> +}
> +
>  /* Record handler for load and store instructions.  */
>  
>  static unsigned int
> @@ -5465,6 +5545,10 @@ aarch64_record_load_store (aarch64_insn_decode_record *aarch64_insn_r)
>        if (insn_bits10_11 == 0x01 || insn_bits10_11 == 0x03)
>  	record_buf[aarch64_insn_r->reg_rec_count++] = reg_rn;
>      }
> +  /* Memory Copy and Memory Set instructions.  */
> +  else if ((insn_bits24_27 & 1) == 1 && insn_bits28_29 == 1
> +	   && insn_bits10_11 == 1 && !insn_bit21)
> +    return aarch64_record_memcopy_memset (aarch64_insn_r);
>    /* Advanced SIMD load/store instructions.  */
>    else
>      return aarch64_record_asimd_load_store (aarch64_insn_r);
Thiago Jung Bauermann May 23, 2024, 2:04 a.m. UTC | #2 
Hello Luis,

Luis Machado <luis.machado@arm.com> writes:

> Just a minor suggestion. Otherwise the patch looks OK.

Great, thanks!

> On 5/10/24 06:24, Thiago Jung Bauermann wrote:
>> +static unsigned int
>> +aarch64_record_memcopy_memset (aarch64_insn_decode_record *aarch64_insn_r)
>> +{
>> +  if (record_debug)
>> +    debug_printf ("Process record: memory copy and memory set\n");
>> +
>> +  uint8_t op1 = bits (aarch64_insn_r->aarch64_insn, 22, 23);
>> +  uint8_t op2 = bits (aarch64_insn_r->aarch64_insn, 12, 15);
>> +  uint32_t reg_rd = bits (aarch64_insn_r->aarch64_insn, 0, 4);
>> +  uint32_t reg_rn = bits (aarch64_insn_r->aarch64_insn, 5, 9);
>> +  uint32_t record_buf[3];
>> +  uint64_t record_buf_mem[4];
>> +
>> +  if (op1 != 3)
>> +    {
>> +      /* Copy instructions.  */
>> +      uint32_t reg_rs = bits (aarch64_insn_r->aarch64_insn, 16, 20);
>> +
>> +      record_buf[0] = reg_rd;
>> +      record_buf[1] = reg_rn;
>> +      record_buf[2] = reg_rs;
>> +      aarch64_insn_r->reg_rec_count = 3;
>> +
>> +      ULONGEST dest_addr;
>> +      regcache_raw_read_unsigned (aarch64_insn_r->regcache, reg_rd,
>> +				  &dest_addr);
>> +      ULONGEST source_addr;
>> +      regcache_raw_read_unsigned (aarch64_insn_r->regcache, reg_rs,
>> +				  &source_addr);
>> +      LONGEST length;
>> +      regcache_raw_read_signed (aarch64_insn_r->regcache, reg_rn,
>> +				&length);
>> +
>> +      /* In a processor using algorithm option A, the length in Rn has an
>> +	 inverted sign.  */
>> +      if (length < 0)
>> +	length *= -1;
>> +
>> +      record_buf_mem[0] = length;
>> +      record_buf_mem[1] = dest_addr;
>> +      record_buf_mem[2] = length;
>> +      record_buf_mem[3] = source_addr;
>> +      aarch64_insn_r->mem_rec_count = 2;
>> +    }
>> +  else if ((op1 == 3 && op2 < 12) || (op1 == 3 && op2 < 12))

The or operands are identical. Fixed in v4.

>> +    {
>> +      /* Set instructions.  */
>> +      record_buf[0] = reg_rd;
>> +      record_buf[1] = reg_rn;
>> +      aarch64_insn_r->reg_rec_count = 2;
>> +
>> +      ULONGEST address;
>> +      regcache_raw_read_unsigned (aarch64_insn_r->regcache, reg_rd,
>> +				  &address);
>> +
>> +      LONGEST length;
>> +      regcache_raw_read_signed (aarch64_insn_r->regcache, reg_rn,
>> +				&length);
>> +
>> +      /* In a processor using algorithm option B, the length in Rn has an
>> +	 inverted sign.  */
>> +      if (length < 0)
>> +	length *= -1;
>> +
>> +      record_buf_mem[0] = length;
>> +      record_buf_mem[1] = address;
>> +      aarch64_insn_r->mem_rec_count = 1;
>> +    }
>
> Looks like both cases above could potentially share some code in a common path. Would be
> nice
> to do so if it doesn't make the code all awkward. If it does, then this is fine.

Well spotted. It turns out that for the copy instructions we need to
record the same information as for the set instructions, plus an
additional register and memory region. Using a common path improves the
function. Thanks for the suggestion. Fixed in v4.

>> +  else
>> +    return AARCH64_RECORD_UNKNOWN;
>> +
>> +  MEM_ALLOC (aarch64_insn_r->aarch64_mems, aarch64_insn_r->mem_rec_count,
>> +	     record_buf_mem);
>> +  REG_ALLOC (aarch64_insn_r->aarch64_regs, aarch64_insn_r->reg_rec_count,
>> +	     record_buf);
>> +  return AARCH64_RECORD_SUCCESS;
>> +}
diff mbox series

Patch

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 05ecd421cd0e..a9a67107675c 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -5188,6 +5188,86 @@  aarch64_record_asimd_load_store (aarch64_insn_decode_record *aarch64_insn_r)
   return AARCH64_RECORD_SUCCESS;
 }
 
+/* Record handler for Memory Copy and Memory Set instructions.  */
+
+static unsigned int
+aarch64_record_memcopy_memset (aarch64_insn_decode_record *aarch64_insn_r)
+{
+  if (record_debug)
+    debug_printf ("Process record: memory copy and memory set\n");
+
+  uint8_t op1 = bits (aarch64_insn_r->aarch64_insn, 22, 23);
+  uint8_t op2 = bits (aarch64_insn_r->aarch64_insn, 12, 15);
+  uint32_t reg_rd = bits (aarch64_insn_r->aarch64_insn, 0, 4);
+  uint32_t reg_rn = bits (aarch64_insn_r->aarch64_insn, 5, 9);
+  uint32_t record_buf[3];
+  uint64_t record_buf_mem[4];
+
+  if (op1 != 3)
+    {
+      /* Copy instructions.  */
+      uint32_t reg_rs = bits (aarch64_insn_r->aarch64_insn, 16, 20);
+
+      record_buf[0] = reg_rd;
+      record_buf[1] = reg_rn;
+      record_buf[2] = reg_rs;
+      aarch64_insn_r->reg_rec_count = 3;
+
+      ULONGEST dest_addr;
+      regcache_raw_read_unsigned (aarch64_insn_r->regcache, reg_rd,
+				  &dest_addr);
+      ULONGEST source_addr;
+      regcache_raw_read_unsigned (aarch64_insn_r->regcache, reg_rs,
+				  &source_addr);
+      LONGEST length;
+      regcache_raw_read_signed (aarch64_insn_r->regcache, reg_rn,
+				&length);
+
+      /* In a processor using algorithm option A, the length in Rn has an
+	 inverted sign.  */
+      if (length < 0)
+	length *= -1;
+
+      record_buf_mem[0] = length;
+      record_buf_mem[1] = dest_addr;
+      record_buf_mem[2] = length;
+      record_buf_mem[3] = source_addr;
+      aarch64_insn_r->mem_rec_count = 2;
+    }
+  else if ((op1 == 3 && op2 < 12) || (op1 == 3 && op2 < 12))
+    {
+      /* Set instructions.  */
+      record_buf[0] = reg_rd;
+      record_buf[1] = reg_rn;
+      aarch64_insn_r->reg_rec_count = 2;
+
+      ULONGEST address;
+      regcache_raw_read_unsigned (aarch64_insn_r->regcache, reg_rd,
+				  &address);
+
+      LONGEST length;
+      regcache_raw_read_signed (aarch64_insn_r->regcache, reg_rn,
+				&length);
+
+      /* In a processor using algorithm option B, the length in Rn has an
+	 inverted sign.  */
+      if (length < 0)
+	length *= -1;
+
+      record_buf_mem[0] = length;
+      record_buf_mem[1] = address;
+      aarch64_insn_r->mem_rec_count = 1;
+    }
+  else
+    return AARCH64_RECORD_UNKNOWN;
+
+  MEM_ALLOC (aarch64_insn_r->aarch64_mems, aarch64_insn_r->mem_rec_count,
+	     record_buf_mem);
+  REG_ALLOC (aarch64_insn_r->aarch64_regs, aarch64_insn_r->reg_rec_count,
+	     record_buf);
+  return AARCH64_RECORD_SUCCESS;
+}
+
 /* Record handler for load and store instructions.  */
 
 static unsigned int
@@ -5465,6 +5545,10 @@  aarch64_record_load_store (aarch64_insn_decode_record *aarch64_insn_r)
       if (insn_bits10_11 == 0x01 || insn_bits10_11 == 0x03)
 	record_buf[aarch64_insn_r->reg_rec_count++] = reg_rn;
     }
+  /* Memory Copy and Memory Set instructions.  */
+  else if ((insn_bits24_27 & 1) == 1 && insn_bits28_29 == 1
+	   && insn_bits10_11 == 1 && !insn_bit21)
+    return aarch64_record_memcopy_memset (aarch64_insn_r);
   /* Advanced SIMD load/store instructions.  */
   else
     return aarch64_record_asimd_load_store (aarch64_insn_r);