[PATCH v2 0/5] Add support for AArch64 MOPS instructions

[PATCH v2 0/5] Add support for AArch64 MOPS instructions

[Thiago Jung Bauermann]

Hello,

I'm sending v2 because Christophe made a suggestion for the
gdb.arch/aarch64-mops-atomic-inst.exp testcase, so patch 4 incoroporates
it.

The other patches are unchanged from v1.

Here is the original cover letter for convenience:

This patch series implements GDB support for the new instructions in
AArch64's MOPS feature.  Patch 1 has a small overview.

What is needed from GDB is recognizing the MOPS sequences of instructions
as atomic so that they can be stepped over during instruction single
stepping, and also to avoid doing displaced stepping with them.  This is
done in patch 1.

Patch 2 adds support for the new instructions to the record an replay
target.

The other patches add testcases to test each of the aspects above, plus
one testcase to verify the interaction of the MOPS instructions with
watchpoints.

Tested on Ubuntu 23.10 aarch64-linux-gnu with no regressions, using the
Arm FVP emulator as well as QEMU v8.2.

Thiago Jung Bauermann (5):
  gdb/aarch64: Implement software single stepping for MOPS instructions
  gdb/aarch64: Add record support for MOPS instructions.
  gdb/testsuite: Add gdb.arch/aarch64-mops-watchpoint.exp
  gdb/testsuite: Add gdb.arch/aarch64-mops-atomic-inst.exp
  gdb/testsuite: Add gdb.reverse/aarch64-mops.exp

 gdb/aarch64-tdep.c                            | 191 +++++++++++++++++-
 .../gdb.arch/aarch64-mops-atomic-inst.c       |  69 +++++++
 .../gdb.arch/aarch64-mops-atomic-inst.exp     |  94 +++++++++
 .../gdb.arch/aarch64-mops-watchpoint.c        |  66 ++++++
 .../gdb.arch/aarch64-mops-watchpoint.exp      |  79 ++++++++
 gdb/testsuite/gdb.reverse/aarch64-mops.c      |  71 +++++++
 gdb/testsuite/gdb.reverse/aarch64-mops.exp    | 171 ++++++++++++++++
 gdb/testsuite/lib/gdb.exp                     |  61 ++++++
 8 files changed, 800 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-atomic-inst.c
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-atomic-inst.exp
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.c
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.exp
 create mode 100644 gdb/testsuite/gdb.reverse/aarch64-mops.c
 create mode 100644 gdb/testsuite/gdb.reverse/aarch64-mops.exp


base-commit: 84a069db6714ddcf444095ed09dbcd7404834694

[PATCH v2 1/5] gdb/aarch64: Implement software single stepping for MOPS instructions

[Thiago Jung Bauermann]

The AArch64 MOPS (Memory Operation) instructions provide a standardised
instruction sequence to perform a memset, memcpy or memmove.  A sequence is
always composed of three instructions: a prologue instruction, a main
instruction and an epilogue instruction.  As an illustration, here are the
implementations of these memory operations in glibc 2.39:

  (gdb) disassemble/r
  Dump of assembler code for function __memset_mops:
  => 0x0000fffff7e8d780 <+0>:     d503201f        nop
     0x0000fffff7e8d784 <+4>:     aa0003e3        mov     x3, x0
     0x0000fffff7e8d788 <+8>:     19c10443        setp    [x3]!, x2!, x1
     0x0000fffff7e8d78c <+12>:    19c14443        setm    [x3]!, x2!, x1
     0x0000fffff7e8d790 <+16>:    19c18443        sete    [x3]!, x2!, x1
     0x0000fffff7e8d794 <+20>:    d65f03c0        ret
  End of assembler dump.

  (gdb) disassemble/r
  Dump of assembler code for function __memcpy_mops:
  => 0x0000fffff7e8c580 <+0>:     d503201f        nop
     0x0000fffff7e8c584 <+4>:     aa0003e3        mov     x3, x0
     0x0000fffff7e8c588 <+8>:     19010443        cpyfp   [x3]!, [x1]!, x2!
     0x0000fffff7e8c58c <+12>:    19410443        cpyfm   [x3]!, [x1]!, x2!
     0x0000fffff7e8c590 <+16>:    19810443        cpyfe   [x3]!, [x1]!, x2!
     0x0000fffff7e8c594 <+20>:    d65f03c0        ret
  End of assembler dump.

  (gdb) disassemble/r
  Dump of assembler code for function __memmove_mops:
  => 0x0000fffff7e8d180 <+0>:     d503201f        nop
     0x0000fffff7e8d184 <+4>:     aa0003e3        mov     x3, x0
     0x0000fffff7e8d188 <+8>:     1d010443        cpyp    [x3]!, [x1]!, x2!
     0x0000fffff7e8d18c <+12>:    1d410443        cpym    [x3]!, [x1]!, x2!
     0x0000fffff7e8d190 <+16>:    1d810443        cpye    [x3]!, [x1]!, x2!
     0x0000fffff7e8d194 <+20>:    d65f03c0        ret
  End of assembler dump.

The Arm Architecture Reference Manual says that "the prologue, main, and
epilogue instructions are expected to be run in succession and to appear
consecutively in memory".  Therefore GDB needs to treat them as an atomic
instruction sequence, and also can't do displaced stepping with them.

This patch implements support for executing the sequence atomically, and
also disables displaced step on them.

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

No change in v2.

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 8d0553f3d7cd..e920cea49066 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -3444,6 +3444,104 @@ value_of_aarch64_user_reg (const frame_info_ptr &frame, const void *baton)
   return value_of_register (*reg_p, get_next_frame_sentinel_okay (frame));
 }
 
+/* Single step through MOPS instruction sequences on AArch64.  */
+
+static std::vector<CORE_ADDR>
+aarch64_software_single_step_mops (struct regcache *regcache, CORE_ADDR loc,
+				   uint32_t insn)
+{
+  const int insn_size = 4;
+  struct gdbarch *gdbarch = regcache->arch ();
+  enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
+  uint8_t o0 = bit (insn, 21);
+  uint8_t op1 = bits (insn, 22, 23);
+  uint8_t op2 = bits (insn, 12, 15);
+
+  /* Look for the prologue instruction that begins the sequence.  */
+
+	/* CPYFP* */
+  if (!((o0 == 0 && op1 == 0)
+	/* SETP* */
+	|| (o0 == 0 && op1 == 3 && op2 < 4)
+	/* CPYP* */
+	|| (o0 == 1 && op1 == 0)
+	/* SETGP* */
+	|| (o0 == 1 && op1 == 3 && op2 < 4)))
+    /* Prologue instruction not found.  */
+    return {};
+
+  /* Now look for the main instruction in the middle of the sequence.  */
+
+  loc += insn_size;
+  ULONGEST insn_from_memory;
+  if (!safe_read_memory_unsigned_integer (loc, insn_size,
+					  byte_order_for_code,
+					  &insn_from_memory))
+    {
+      /* Assume we don't have a MOPS sequence, as we couldn't read the
+	 instruction in this location.  */
+      return {};
+    }
+
+  insn = insn_from_memory;
+  aarch64_inst inst;
+  if (aarch64_decode_insn (insn, &inst, 1, nullptr) != 0)
+    return {};
+  if (!AARCH64_CPU_HAS_FEATURE (*inst.opcode->avariant, MOPS))
+    return {};
+
+  o0 = bit (insn, 21);
+  op1 = bits (insn, 22, 23);
+  op2 = bits (insn, 12, 15);
+
+	/* CPYFM* */
+  if (!((o0 == 0 && op1 == 1)
+	/* SETM* */
+	|| (o0 == 0 && op1 == 3 && op2 >= 4 && op2 < 8)
+	/* CPYM* */
+	|| (o0 == 1 && op1 == 1)
+	/* SETGM* */
+	|| (o0 == 1 && op1 == 3 && op2 >= 4 && op2 < 8)))
+    /* Main instruction not found.  */
+    return {};
+
+  /* Now look for the epilogue instruction that ends the sequence.  */
+
+  loc += insn_size;
+  if (!safe_read_memory_unsigned_integer (loc, insn_size,
+					  byte_order_for_code,
+					  &insn_from_memory))
+    {
+      /* Assume we don't have a MOPS sequence, as we couldn't read the
+	 instruction in this location.  */
+      return {};
+    }
+
+  insn = insn_from_memory;
+  if (aarch64_decode_insn (insn, &inst, 1, nullptr) != 0)
+    return {};
+  if (!AARCH64_CPU_HAS_FEATURE (*inst.opcode->avariant, MOPS))
+    return {};
+
+  o0 = bit (insn, 21);
+  op1 = bits (insn, 22, 23);
+  op2 = bits (insn, 12, 15);
+
+	/* CPYFE* */
+  if (!((o0 == 0 && op1 == 2)
+	/* SETE* (op2 >= 12 is unallocated space) */
+	|| (o0 == 0 && op1 == 3 && op2 >= 8 && op2 < 12)
+	/* CPYE* */
+	|| (o0 == 1 && op1 == 2)
+	/* SETGE* (op2 >= 12 is unallocated space) */
+	|| (o0 == 1 && op1 == 3 && op2 >= 8 && op2 < 12)))
+    /* Epilogue instruction not found.  */
+    return {};
+
+  /* Insert breakpoint after the end of the atomic sequence.  */
+  return { loc + insn_size };
+}
+
 /* Implement the "software_single_step" gdbarch method, needed to
    single step through atomic sequences on AArch64.  */
 
@@ -3479,6 +3577,9 @@ aarch64_software_single_step (struct regcache *regcache)
   if (aarch64_decode_insn (insn, &inst, 1, NULL) != 0)
     return {};
 
+  if (AARCH64_CPU_HAS_FEATURE (*inst.opcode->avariant, MOPS))
+    return aarch64_software_single_step_mops (regcache, loc, insn);
+
   /* Look for a Load Exclusive instruction which begins the sequence.  */
   if (inst.opcode->iclass != ldstexcl || bit (insn, 22) == 0)
     return {};
@@ -3808,8 +3909,10 @@ aarch64_displaced_step_copy_insn (struct gdbarch *gdbarch,
   if (aarch64_decode_insn (insn, &inst, 1, NULL) != 0)
     return NULL;
 
-  /* Look for a Load Exclusive instruction which begins the sequence.  */
-  if (inst.opcode->iclass == ldstexcl && bit (insn, 22))
+  /* Look for a Load Exclusive instruction which begins the sequence,
+     or for a MOPS instruction.  */
+  if ((inst.opcode->iclass == ldstexcl && bit (insn, 22))
+      || AARCH64_CPU_HAS_FEATURE (*inst.opcode->avariant, MOPS))
     {
       /* We can't displaced step atomic sequences.  */
       return NULL;

Re: [PATCH v2 1/5] gdb/aarch64: Implement software single stepping for MOPS instructions

[Pedro Alves]

On 2024-05-07 03:22, Thiago Jung Bauermann wrote:

> The Arm Architecture Reference Manual says that "the prologue, main, and
> epilogue instructions are expected to be run in succession and to appear
> consecutively in memory".  Therefore GDB needs to treat them as an atomic
> instruction sequence, and also can't do displaced stepping with them.

Curious on this "can't".  GDB could copy the whole sequence to the displaced
stepping buffer, and execute then in one go, and it should work, right?

Not suggesting that you should do it as a requisite to the patch, just
trying to understand it, in case the issue comes up again in other contexts.

Re: [PATCH v2 1/5] gdb/aarch64: Implement software single stepping for MOPS instructions

[Thiago Jung Bauermann]

Pedro Alves <pedro@palves.net> writes:

> On 2024-05-07 03:22, Thiago Jung Bauermann wrote:
>
>> The Arm Architecture Reference Manual says that "the prologue, main, and
>> epilogue instructions are expected to be run in succession and to appear
>> consecutively in memory".  Therefore GDB needs to treat them as an atomic
>> instruction sequence, and also can't do displaced stepping with them.
>
> Curious on this "can't".  GDB could copy the whole sequence to the displaced
> stepping buffer, and execute then in one go, and it should work, right?
>
> Not suggesting that you should do it as a requisite to the patch, just
> trying to understand it, in case the issue comes up again in other contexts.

Yes. I hadn't thought of that, but Luis also gave that idea. He said¹
that "Regarding displaced stepping, we can disable displaced stepping
temporarily for these instructions. If we find a convenient way to
relocate the whole block of MOPS instructions, then we can add that
later on."

I'll work on it as a future improvement.

-- 
Thiago

¹ https://inbox.sourceware.org/gdb-patches/2c974745-b2d1-4981-bbc8-1b5985970fdb@arm.com/

Re: [PATCH v2 1/5] gdb/aarch64: Implement software single stepping for MOPS instructions

[Luis Machado]

On 5/21/24 22:40, Thiago Jung Bauermann wrote:
> Pedro Alves <pedro@palves.net> writes:
> 
>> On 2024-05-07 03:22, Thiago Jung Bauermann wrote:
>>
>>> The Arm Architecture Reference Manual says that "the prologue, main, and
>>> epilogue instructions are expected to be run in succession and to appear
>>> consecutively in memory".  Therefore GDB needs to treat them as an atomic
>>> instruction sequence, and also can't do displaced stepping with them.
>>
>> Curious on this "can't".  GDB could copy the whole sequence to the displaced
>> stepping buffer, and execute then in one go, and it should work, right?
>>
>> Not suggesting that you should do it as a requisite to the patch, just
>> trying to understand it, in case the issue comes up again in other contexts.
> 
> Yes. I hadn't thought of that, but Luis also gave that idea. He said¹
> that "Regarding displaced stepping, we can disable displaced stepping
> temporarily for these instructions. If we find a convenient way to
> relocate the whole block of MOPS instructions, then we can add that
> later on."
> 
> I'll work on it as a future improvement.
> 

FTR, I'm still happy with enabling this in an upcoming patch. We might have to
think about whether displaced-stepping the block and then getting interrupted
in the middle of executing it needs special handling.

[PATCH v2 2/5] gdb/aarch64: Add record support for MOPS instructions.

[Thiago Jung Bauermann]

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 in v2.

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index e920cea49066..c60c5d6e0ec2 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -5289,6 +5289,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
@@ -5566,6 +5646,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);

[PATCH v2 3/5] gdb/testsuite: Add gdb.arch/aarch64-mops-watchpoint.exp

[Thiago Jung Bauermann]

Test behaviour of watchpoints triggered by MOPS instructions.  This test
is similar to gdb.base/memops-watchpoint.exp, but specifically for MOPS
instructions rather than whatever instructions are used in the libc's
implementation of memset/memcpy/memmove.

There's a separate watched variable for each set of instructions so that
the testcase can test whether GDB correctly identified the watchpoint
that triggered in each case.
---
 .../gdb.arch/aarch64-mops-watchpoint.c        | 66 ++++++++++++++++
 .../gdb.arch/aarch64-mops-watchpoint.exp      | 79 +++++++++++++++++++
 gdb/testsuite/lib/gdb.exp                     | 61 ++++++++++++++
 3 files changed, 206 insertions(+)
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.c
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.exp

No change in v2.

diff --git a/gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.c b/gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.c
new file mode 100644
index 000000000000..b981f033d210
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.c
@@ -0,0 +1,66 @@
+/* This test program is part of GDB, the GNU debugger.
+
+   Copyright 2024 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+main (void)
+{
+  char source[40] __attribute__ ((aligned (8)))
+    = "This is a relatively long string...";
+  char a[40] __attribute__ ((aligned (8)))
+    = "String to be overwritten with zeroes";
+  char b[40] __attribute__ ((aligned (8)))
+    = "Another string to be memcopied...";
+  char c[40] __attribute__ ((aligned (8)))
+    = "Another string to be memmoved...";
+  char *p, *q;
+  long size, zero;
+
+  /* Break here.  */
+  p = a;
+  size = sizeof (a);
+  zero = 0;
+  /* memset implemented in MOPS instructions.  */
+  __asm__ volatile ("setp [%0]!, %1!, %2\n\t"
+		    "setm [%0]!, %1!, %2\n\t"
+		    "sete [%0]!, %1!, %2\n\t"
+		    : "+&r"(p), "+&r"(size)
+		    : "r"(zero)
+		    : "memory");
+
+  p = b;
+  q = source;
+  size = sizeof (b);
+  /* memmove implemented in MOPS instructions.  */
+  __asm__ volatile ("cpyp   [%0]!, [%1]!, %2!\n\t"
+		    "cpym   [%0]!, [%1]!, %2!\n\t"
+		    "cpye   [%0]!, [%1]!, %2!\n\t"
+		    : "+&r" (p), "+&r" (q), "+&r" (size)
+		    :
+		    : "memory");
+  p = c;
+  q = source;
+  size = sizeof (c);
+  /* memcpy implemented in MOPS instructions.  */
+  __asm__ volatile ("cpyfp   [%0]!, [%1]!, %2!\n\t"
+		    "cpyfm   [%0]!, [%1]!, %2!\n\t"
+		    "cpyfe   [%0]!, [%1]!, %2!\n\t"
+		    : "+&r" (p), "+&r" (q), "+&r" (size)
+		    :
+		    : "memory");
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.exp b/gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.exp
new file mode 100644
index 000000000000..9e210602d800
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.exp
@@ -0,0 +1,79 @@
+# Copyright 2024 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test a binary that uses MOPS (Memory Operations) instructions.
+# This test is similar to gdb.base/memops-watchpoint.exp, but specifically
+# tests MOPS instructions rather than whatever instructions are used in the
+# system libc's implementation of memset/memcpy/memmove.
+
+require allow_hw_watchpoint_tests allow_aarch64_mops_tests
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
+	  [list debug additional_flags=-march=armv9.3-a]] } {
+    return -1
+}
+
+set linespec ${srcfile}:[gdb_get_line_number "Break here"]
+if ![runto ${linespec}] {
+    return -1
+}
+
+gdb_test "watch -location a\[28\]" \
+    "(Hardware w|W)atchpoint ${decimal}: -location a\\\[28\\\]" \
+    "set watch on a"
+gdb_test "watch -location b\[28\]" \
+    "(Hardware w|W)atchpoint ${decimal}: -location b\\\[28\\\]" \
+    "set watchpoint on b"
+gdb_test "watch -location c\[28\]" \
+    "(Hardware w|W)atchpoint ${decimal}: -location c\\\[28\\\]" \
+    "set watchpoint on c"
+
+gdb_test "continue" \
+    [multi_line \
+	 "Continuing\\." \
+	 "" \
+	 "Hardware watchpoint ${decimal}: -location a\\\[28\\\]" \
+	 "" \
+	 "Old value = 104 'h'" \
+	 "New value = 0 '\\\\000'" \
+	 "$hex in main \\(\\) at .*aarch64-mops-watchpoint.c:$decimal" \
+	 "${decimal}\\s+__asm__ volatile \\(\"setp.*\\\\n\\\\t\""] \
+    "continue until set watchpoint hits"
+
+gdb_test "continue" \
+    [multi_line \
+	 "Continuing\\." \
+	 "" \
+	 "Hardware watchpoint ${decimal}: -location b\\\[28\\\]" \
+	 "" \
+	 "Old value = 101 'e'" \
+	 "New value = 114 'r'" \
+	 "$hex in main \\(\\) at .*aarch64-mops-watchpoint.c:$decimal" \
+	 "${decimal}\\s+__asm__ volatile \\(\"cpyp.*\\\\n\\\\t\""] \
+    "continue until cpy watchpoint hits"
+
+gdb_test "continue" \
+    [multi_line \
+	 "Continuing\\." \
+	 "" \
+	 "Hardware watchpoint ${decimal}: -location c\\\[28\\\]" \
+	 "" \
+	 "Old value = 100 'd'" \
+	 "New value = 114 'r'" \
+	 "$hex in main \\(\\) at .*aarch64-mops-watchpoint.c:$decimal" \
+	 "${decimal}\\s+__asm__ volatile \\(\"cpyfp.*\\\\n\\\\t\""] \
+    "continue until cpyf watchpoint hits"
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index fe3f05c18df9..78c926ac80b6 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -4497,6 +4497,67 @@ proc aarch64_supports_sme_svl { length } {
     return 1
 }
 
+# Run a test on the target to see if it supports Aarch64 MOPS (Memory
+# Operations) extensions.  Return 0 if so, 1 if it does not.  Note this causes
+# a restart of GDB.
+
+gdb_caching_proc allow_aarch64_mops_tests {} {
+    global srcdir subdir gdb_prompt inferior_exited_re
+
+    set me "allow_aarch64_mops_tests"
+
+    if { ![is_aarch64_target]} {
+	return 0
+    }
+
+    # ARMv9.3-A contains the MOPS extension.  The test program doesn't use it,
+    # but take the opportunity to check whether the toolchain knows about MOPS.
+    set compile_flags "{additional_flags=-march=armv9.3-a}"
+
+    # Compile a program that tests the MOPS feature.
+    set src {
+	#include <stdbool.h>
+	#include <sys/auxv.h>
+
+	#ifndef HWCAP2_MOPS
+	#define HWCAP2_MOPS (1UL << 43)
+	#endif
+
+	int main() {
+	    bool mops_supported = getauxval (AT_HWCAP2) & HWCAP2_MOPS;
+
+	    return !mops_supported;
+	}
+    }
+
+    if {![gdb_simple_compile $me $src executable $compile_flags]} {
+	return 0
+    }
+
+    # Compilation succeeded so now run it via gdb.
+    clean_restart $obj
+    gdb_run_cmd
+    gdb_expect {
+	-re ".*$inferior_exited_re with code 01.*${gdb_prompt} $" {
+	    verbose -log "\n$me mops support not detected"
+	    set allow_mops_tests 0
+	}
+	-re ".*$inferior_exited_re normally.*${gdb_prompt} $" {
+	    verbose -log "\n$me: mops support detected"
+	    set allow_mops_tests 1
+	}
+	default {
+	  warning "\n$me: default case taken"
+	    set allow_mops_tests 0
+	}
+    }
+    gdb_exit
+    remote_file build delete $obj
+
+    verbose "$me:  returning $allow_mops_tests" 2
+    return $allow_mops_tests
+}
+
 # A helper that compiles a test case to see if __int128 is supported.
 proc gdb_int128_helper {lang} {
     return [gdb_can_simple_compile "i128-for-$lang" {

Re: [PATCH v2 3/5] gdb/testsuite: Add gdb.arch/aarch64-mops-watchpoint.exp

[Pedro Alves]

On 2024-05-07 03:22, Thiago Jung Bauermann wrote:
> +# Run a test on the target to see if it supports Aarch64 MOPS (Memory
> +# Operations) extensions.  Return 0 if so, 1 if it does not.  

This

   "Return 0 if so, 1 if it does not." 

comment seems backwards.

> Note this causes
> +# a restart of GDB.

Might want to apply a similar logic to can_spawn_for_attach, wrt gdb_exit.
See the tail end of that function.  Maybe we can factor that out and
reuse it.

> +
> +gdb_caching_proc allow_aarch64_mops_tests {} {

Re: [PATCH v2 3/5] gdb/testsuite: Add gdb.arch/aarch64-mops-watchpoint.exp

[Thiago Jung Bauermann]

Hello Pedro,

Thank you for your review!

Pedro Alves <pedro@palves.net> writes:

> On 2024-05-07 03:22, Thiago Jung Bauermann wrote:
>> +# Run a test on the target to see if it supports Aarch64 MOPS (Memory
>> +# Operations) extensions.  Return 0 if so, 1 if it does not.  
>
> This
>
>    "Return 0 if so, 1 if it does not." 
>
> comment seems backwards.

Yes, indeed. Fixed the thinko for v4.

>> Note this causes
>> +# a restart of GDB.
>
> Might want to apply a similar logic to can_spawn_for_attach, wrt gdb_exit.
> See the tail end of that function.  Maybe we can factor that out and
> reuse it.

I looked into it, but I wasn't able to understand what is the problem
that gdb_exit causes. Does it mean that any require predicate that
restarts GDB needs to use that two-tier caching scheme?

Many of the predicates that test for hardware features restart GDB, so
this would be a refactor affecting many predicates. I don't mind doing
it but I'd suggest doing so as a separate patch. It would also be useful
to have a reproducer of the problem that the double-caching fixes to
check that I'm effectively solving the problem.

>> +
>> +gdb_caching_proc allow_aarch64_mops_tests {} {

-- 
Thiago

Re: [PATCH v2 3/5] gdb/testsuite: Add gdb.arch/aarch64-mops-watchpoint.exp

[Tom de Vries]

On 5/21/24 23:03, Thiago Jung Bauermann wrote:
> Hello Pedro,
> 
> Thank you for your review!
> 
> Pedro Alves <pedro@palves.net> writes:
> 
>> On 2024-05-07 03:22, Thiago Jung Bauermann wrote:
>>> +# Run a test on the target to see if it supports Aarch64 MOPS (Memory
>>> +# Operations) extensions.  Return 0 if so, 1 if it does not.
>>
>> This
>>
>>     "Return 0 if so, 1 if it does not."
>>
>> comment seems backwards.
> 
> Yes, indeed. Fixed the thinko for v4.
> 
>>> Note this causes
>>> +# a restart of GDB.
>>
>> Might want to apply a similar logic to can_spawn_for_attach, wrt gdb_exit.
>> See the tail end of that function.  Maybe we can factor that out and
>> reuse it.
> 
> I looked into it, but I wasn't able to understand what is the problem
> that gdb_exit causes. Does it mean that any require predicate that
> restarts GDB needs to use that two-tier caching scheme?
> 

Let me try to explain the idea.  Consider two demonstrator test-cases:
...
diff --git a/gdb/testsuite/gdb.testsuite/example-2.exp 
b/gdb/testsuite/gdb.testsuite/example-2.exp
new file mode 100644
index 00000000000..0efa36e8cec
--- /dev/null
+++ b/gdb/testsuite/gdb.testsuite/example-2.exp
@@ -0,0 +1,8 @@
+if { [foo] } {
+    verbose -log "foo!"
+}
+
+clean_restart
+
+gdb_test "print 1"
+
diff --git a/gdb/testsuite/gdb.testsuite/example.exp 
b/gdb/testsuite/gdb.testsuite/example.exp
new file mode 100644
index 00000000000..2e463a911b6
--- /dev/null
+++ b/gdb/testsuite/gdb.testsuite/example.exp
@@ -0,0 +1,8 @@
+clean_restart
+
+if { [foo] } {
+    verbose -log "foo!"
+}
+
+gdb_test "print 1"
+
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 87e6ee050f2..d176a584a21 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -10618,5 +10618,10 @@ gdb_caching_proc root_user {} {
      return [expr $uid == 0]
  }

+gdb_caching_proc foo {} {
+    gdb_exit
+    return 1
+}
+
  # Always load compatibility stuff.
  load_lib future.exp
...

This runs fine like this:
...
$ ( cd build/gdb/testsuite/; make check TESTS="gdb.testsuite/example*.exp" )
...

But after swapping the two test-cases:
...
$ swap.sh gdb/testsuite/gdb.testsuite/example-2.exp 
gdb/testsuite/gdb.testsuite/example.exp
...
it fails like this instead:
...
ERROR: no fileid for xerxes
...

So:
- obviously, using a gdb_exit in a proc results in trouble if the proc
   is called at a point in the test-case where we don't want a gdb_exit
- using a gdb_exit in a caching proc may or may not result in trouble if
   the proc is called at a point in the test-case where we don't want
   gdb_exit.  Whether we run into trouble depends on whether the result
   of the proc is already cached or not.  This behaviour is undesirable.

The simplest way of fixing this, is by splitting up the proc into an 
uncached and cached part, and moving the gdb_exit to the uncached part.

This makes sure we that run into trouble in each test-case, independent 
of whether the result is already cached or not.

That however is somewhat too restrictive in the case that there are more 
call sites in a single test-case.  We only need the gdb_exit in the 
first call site, so we add the second tier of caching to make sure that 
we don't call gdb_exit from the uncached part more than once in a test-case.

This two-tier scheme is convenient for can_spawn_for_attach, because 
it's a pre-existing proc that didn't use gdb_exit and consequently was 
used in random places in test-cases, and also multiple times in a single 
test-case.

For a proc like allow_aarch64_mops_tests, which is only used at the very 
start of a test-case, once, IMO it's not necessary, and not worth the 
effort of splitting up the proc.

However, if we factor this approach out into a special variant, say 
gdb_caching_proc_with_gdb_exit or some such, then it could be done 
relatively easily.

Thanks,
- Tom

> Many of the predicates that test for hardware features restart GDB, so
> this would be a refactor affecting many predicates. I don't mind doing
> it but I'd suggest doing so as a separate patch. It would also be useful
> to have a reproducer of the problem that the double-caching fixes to
> check that I'm effectively solving the problem.
> 
>>> +
>>> +gdb_caching_proc allow_aarch64_mops_tests {} {
> 

Re: [PATCH v2 3/5] gdb/testsuite: Add gdb.arch/aarch64-mops-watchpoint.exp

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> - obviously, using a gdb_exit in a proc results in trouble if the proc
Tom>   is called at a point in the test-case where we don't want a gdb_exit
Tom> - using a gdb_exit in a caching proc may or may not result in trouble if
Tom>   the proc is called at a point in the test-case where we don't want
Tom>   gdb_exit.  Whether we run into trouble depends on whether the result
Tom>   of the proc is already cached or not.  This behaviour is undesirable.

I didn't really follow this thread, but I tend to think that caching
procs should be in two categories: those that require gdb to already be
running, and those that are standalone.  Having a proc like this call
gdb_exit seems like a recipe for some confusion, to me.  Even having one
start a new gdb seems like trouble, since IIRC that's hard to do without
introducing new test results, but the nature of a caching proc is that
the test names would then depend on which process wins the race to run
it first.  I've thought sometimes about adding some mechanism to enforce
this split.

Tom

Re: [PATCH v2 3/5] gdb/testsuite: Add gdb.arch/aarch64-mops-watchpoint.exp

[Thiago Jung Bauermann]

Hello Tom,

Tom de Vries <tdevries@suse.de> writes:

> On 5/21/24 23:03, Thiago Jung Bauermann wrote:
>> Hello Pedro,
>> Thank you for your review!
>> Pedro Alves <pedro@palves.net> writes:
>> 
>>>> Note this causes
>>>> +# a restart of GDB.
>>>
>>> Might want to apply a similar logic to can_spawn_for_attach, wrt gdb_exit.
>>> See the tail end of that function.  Maybe we can factor that out and
>>> reuse it.
>>
>> I looked into it, but I wasn't able to understand what is the problem
>> that gdb_exit causes. Does it mean that any require predicate that
>> restarts GDB needs to use that two-tier caching scheme?
>> 
>
> Let me try to explain the idea.  Consider two demonstrator test-cases:

<snip>

Thank you for the explanation and clear example. I understand the
problem now.

> This two-tier scheme is convenient for can_spawn_for_attach, because it's a pre-existing
> proc that didn't use gdb_exit and consequently was used in random places in test-cases,
> and also multiple times in a single test-case.
>
> For a proc like allow_aarch64_mops_tests, which is only used at the very start of a
> test-case, once, IMO it's not necessary, and not worth the effort of splitting up the
> proc.

Yes, I agree that this issue doesn't affect the require predicate proc.

> However, if we factor this approach out into a special variant, say
> gdb_caching_proc_with_gdb_exit or some such, then it could be done relatively easily.

If there are other caching procs that exit or restart GDB that are used
in the middle of testcases, then that is indeed a good idea.
-- 
Thiago

[PATCH v2 4/5] gdb/testsuite: Add gdb.arch/aarch64-mops-atomic-inst.exp

[Thiago Jung Bauermann]

The testcase verifies that MOPS sequences are stepped over when stepping
the inferior instruction by instruction.
---
 .../gdb.arch/aarch64-mops-atomic-inst.c       | 69 ++++++++++++++
 .../gdb.arch/aarch64-mops-atomic-inst.exp     | 94 +++++++++++++++++++
 2 files changed, 163 insertions(+)
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-atomic-inst.c
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-atomic-inst.exp

Changes in v2:
- Add prfm instruction after each MOPS sequence and look for it in the
  testcase to verify that the sequence was stepped through (Suggested by
  Christophe).

diff --git a/gdb/testsuite/gdb.arch/aarch64-mops-atomic-inst.c b/gdb/testsuite/gdb.arch/aarch64-mops-atomic-inst.c
new file mode 100644
index 000000000000..e33ba6a5900b
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-mops-atomic-inst.c
@@ -0,0 +1,69 @@
+/* This file is part of GDB, the GNU debugger.
+
+   Copyright 2024 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#define TEST_STRING "Just a test string."
+#define BUF_SIZE sizeof(TEST_STRING)
+
+int main(void)
+{
+  const char source[BUF_SIZE] = TEST_STRING;
+  char dest[BUF_SIZE];
+  char *p = dest;
+  const char *q;
+  const long zero = 0;
+  long size = BUF_SIZE;
+
+  /* Note: The prfm instruction in the asm statements below is there just
+     to allow the testcase to recognize when the PC is at the instruction
+     right after the MOPS sequence.  */
+
+  /* Break memset.  */
+  /* memset implemented in MOPS instructions.  */
+  __asm__ volatile ("setp [%0]!, %1!, %2\n\t"
+		    "setm [%0]!, %1!, %2\n\t"
+		    "sete [%0]!, %1!, %2\n\t"
+		    "prfm pldl3keep, [%0, #0]\n\t"
+		    : "+&r"(p), "+&r"(size)
+		    : "r"(zero)
+		    : "memory");
+
+  p = dest;
+  q = source;
+  /* Break memcpy.  */
+  /* memcpy implemented in MOPS instructions.  */
+  __asm__ volatile ("cpyfp [%0]!, [%1]!, %2!\n\t"
+		    "cpyfm [%0]!, [%1]!, %2!\n\t"
+		    "cpyfe [%0]!, [%1]!, %2!\n\t"
+		    "prfm pldl3keep, [%0, #0]\n\t"
+		    : "+&r" (p), "+&r" (q), "+&r" (size)
+		    :
+		    : "memory");
+
+  p = dest;
+  q = source;
+  /* Break memmove.  */
+  /* memmove implemented in MOPS instructions.  */
+  __asm__ volatile ("cpyp [%0]!, [%1]!, %2!\n\t"
+		    "cpym [%0]!, [%1]!, %2!\n\t"
+		    "cpye [%0]!, [%1]!, %2!\n\t"
+		    "prfm pldl3keep, [%0, #0]\n\t"
+		    : "+&r" (p), "+&r" (q), "+&r" (size)
+		    :
+		    : "memory");
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.arch/aarch64-mops-atomic-inst.exp b/gdb/testsuite/gdb.arch/aarch64-mops-atomic-inst.exp
new file mode 100644
index 000000000000..f28da6c3321b
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-mops-atomic-inst.exp
@@ -0,0 +1,94 @@
+# Copyright 2024 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# This file is part of the GDB testsuite.
+
+# Test single stepping through MOPS (memory operations) atomic sequences.
+
+require allow_aarch64_mops_tests
+
+standard_testfile
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
+	  [list debug additional_flags=-march=armv9.3-a]] } {
+    return -1
+}
+
+proc is_at_instruction { instruction } {
+    global gdb_prompt hex
+
+    set test "pc points to $instruction"
+    gdb_test_multiple {x/i $pc} $test {
+	-re -wrap "=> $hex \[^\r\n\]+:\t$instruction\t\[^\r\n\]+" {
+	    return 1
+	}
+	-re "\r\n$gdb_prompt $" {
+	    return 0
+	}
+    }
+
+    return 0
+}
+
+proc arrive_at_instruction { instruction } {
+    set count 0
+
+    while { [is_at_instruction $instruction] != 1 } {
+	gdb_test -nopass "nexti" ".*__asm__ volatile.*" \
+	    "nexti #$count to reach $instruction"
+	incr count
+
+	if { $count > 50 } {
+	    fail "didn't reach $instruction"
+	    return 0
+	}
+    }
+
+    return 1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+gdb_breakpoint ${srcfile}:[gdb_get_line_number "Break memset"]
+gdb_breakpoint ${srcfile}:[gdb_get_line_number "Break memcpy"]
+gdb_breakpoint ${srcfile}:[gdb_get_line_number "Break memmove"]
+
+gdb_continue_to_breakpoint "memset breakpoint"
+
+if { [arrive_at_instruction setp] } {
+    # The nexti output isn't useful to detect whether we skipped the sequence.
+    gdb_test "nexti" "\[^\r\n\]+" "step through the memset sequence"
+    gdb_assert { [is_at_instruction prfm] == 1 } \
+	"stepped through the memset sequence"
+}
+
+gdb_continue_to_breakpoint "memcpy breakpoint"
+
+if { [arrive_at_instruction cpyfp] } {
+    # The nexti output isn't useful to detect whether we skipped the sequence.
+    gdb_test "nexti" "\[^\r\n\]+" "step through the memcpy sequence"
+    gdb_assert { [is_at_instruction prfm] == 1 } \
+	"stepped through the memcpy sequence"
+}
+
+gdb_continue_to_breakpoint "memmove breakpoint"
+
+if { [arrive_at_instruction cpyp] } {
+    # The nexti output isn't useful to detect whether we skipped the sequence.
+    gdb_test "nexti" "\[^\r\n\]+" "step through the memmove sequence"
+    gdb_assert { [is_at_instruction prfm] == 1 } \
+	"stepped through the memmove sequence"
+}

[PATCH v2 5/5] gdb/testsuite: Add gdb.reverse/aarch64-mops.exp

[Thiago Jung Bauermann]

The testcase verifies that MOPS instructions are recorded and correctly
reversed.  Not all variants of the copy and set instructions are tested,
since there are many and the record and replay target processes them in
the same way.
---
 gdb/testsuite/gdb.reverse/aarch64-mops.c   |  71 +++++++++
 gdb/testsuite/gdb.reverse/aarch64-mops.exp | 171 +++++++++++++++++++++
 2 files changed, 242 insertions(+)
 create mode 100644 gdb/testsuite/gdb.reverse/aarch64-mops.c
 create mode 100644 gdb/testsuite/gdb.reverse/aarch64-mops.exp

No change in v2.

diff --git a/gdb/testsuite/gdb.reverse/aarch64-mops.c b/gdb/testsuite/gdb.reverse/aarch64-mops.c
new file mode 100644
index 000000000000..513f324b9dd6
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/aarch64-mops.c
@@ -0,0 +1,71 @@
+/* This test program is part of GDB, the GNU debugger.
+
+   Copyright 2024 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#define TEST_STRING "Just a test string."
+#define BUF_SIZE sizeof(TEST_STRING)
+
+int
+main (void)
+{
+  char dest[BUF_SIZE];
+  char source[BUF_SIZE] = TEST_STRING;
+  register char *p asm ("x19");
+  register char *q asm ("x20");
+  register long size asm ("x21");
+  register long zero asm ("x22");
+
+  p = dest;
+  size = BUF_SIZE;
+  zero = 0;
+  /* Before setp.  */
+  /* memset implemented in MOPS instructions.  */
+  __asm__ volatile ("setp [%0]!, %1!, %2\n\t"
+                    "setm [%0]!, %1!, %2\n\t"
+                    "sete [%0]!, %1!, %2\n\t"
+                    : "+&r"(p), "+&r"(size)
+                    : "r"(zero)
+                    : "memory");
+
+  /* After sete.  */
+  p = dest;
+  q = source;
+  size = BUF_SIZE;
+  /* Before cpyp.  */
+  /* memmove implemented in MOPS instructions.  */
+  __asm__ volatile ("cpyp   [%0]!, [%1]!, %2!\n\t"
+		    "cpym   [%0]!, [%1]!, %2!\n\t"
+		    "cpye   [%0]!, [%1]!, %2!\n\t"
+		    : "+&r" (p), "+&r" (q), "+&r" (size)
+		    :
+		    : "memory");
+  /* After cpye.  */
+  p = dest;
+  q = source;
+  size = BUF_SIZE;
+  /* Before cpyfp.  */
+  /* memcpy implemented in MOPS instructions.  */
+  __asm__ volatile ("cpyfp   [%0]!, [%1]!, %2!\n\t"
+		    "cpyfm   [%0]!, [%1]!, %2!\n\t"
+		    "cpyfe   [%0]!, [%1]!, %2!\n\t"
+		    : "+&r" (p), "+&r" (q), "+&r" (size)
+		    :
+		    : "memory");
+  /* After cpyfe.  */
+  p = dest;
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.reverse/aarch64-mops.exp b/gdb/testsuite/gdb.reverse/aarch64-mops.exp
new file mode 100644
index 000000000000..f9c1257e0b11
--- /dev/null
+++ b/gdb/testsuite/gdb.reverse/aarch64-mops.exp
@@ -0,0 +1,171 @@
+# Copyright 2024 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test instruction record for AArch64 FEAT_MOPS instructions.
+# Based on gdb.reverse/ppc_record_test_isa_3_1.exp
+#
+# The basic flow of the record tests are:
+#    1) Stop before executing the instructions of interest.  Record
+#       the initial value of the registers that the instruction will
+#       change, i.e. the destination register.
+#    2) Execute the instructions.  Record the new value of the
+#       registers that changed.
+#    3) Reverse the direction of the execution and execute back to
+#       just before the instructions of interest.  Record the final
+#       value of the registers of interest.
+#    4) Check that the initial and new values of the registers are
+#       different, i.e. the instruction changed the registers as expected.
+#    5) Check that the initial and final values of the registers are
+#       the same, i.e. GDB record restored the registers to their
+#       original values.
+
+require allow_aarch64_mops_tests
+
+standard_testfile
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
+	  [list debug additional_flags=-march=armv9.3-a]] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+gdb_test_no_output "record full"
+
+proc do_test { insn_prefix } {
+    global decimal hex
+
+    set before_seq [gdb_get_line_number "Before ${insn_prefix}p"]
+    set after_seq [gdb_get_line_number "After ${insn_prefix}e"]
+
+    with_test_prefix $insn_prefix {
+	gdb_test "break $before_seq" \
+	    "Breakpoint $decimal at $hex: file .*/aarch64-mops.c, line $decimal\\." \
+	    "break before instruction sequence"
+	gdb_test "continue" \
+	    [multi_line \
+		 "Continuing\\." \
+		 "" \
+		 "Breakpoint $decimal, main \\(\\) at .*/aarch64-mops.c:$decimal" \
+		 "$decimal\[ \t\]+__asm__ volatile \\(\"${insn_prefix}p \[^\r\n\]+\""] \
+	    "about to execute instruction sequence"
+
+	# Record the initial register values.
+	set x19_initial [capture_command_output "info register x19" ""]
+	set x21_initial [capture_command_output "info register x21" ""]
+
+	# The set instructions use the ZERO variable, but not Q, and the other
+	# instructions are the opposite.
+	if {[string compare $insn_prefix "set"] == 0} {
+	    set x22_initial [capture_command_output "info register x22" ""]
+	} else {
+	    set x20_initial [capture_command_output "info register x20" ""]
+	}
+
+	gdb_test "break $after_seq" \
+	    "Breakpoint $decimal at $hex: file .*/aarch64-mops.c, line $decimal\\." \
+	    "break after instruction sequence"
+	gdb_test "continue" \
+	    [multi_line \
+		 "Continuing\\." \
+		 "" \
+		 "Breakpoint $decimal, main \\(\\) at .*/aarch64-mops.c:$decimal" \
+		 "$decimal\[ \t\]+p = dest;"] \
+	    "executed instruction sequence"
+
+	# Record the new register values.
+	set x19_new [capture_command_output "info register x19" ""]
+	set x21_new [capture_command_output "info register x21" ""]
+
+	if {[string compare $insn_prefix "set"] == 0} {
+	    set x22_new [capture_command_output "info register x22" ""]
+	} else {
+	    set x20_new [capture_command_output "info register x20" ""]
+	}
+
+	# Execute in reverse to before the instruction sequence.
+	gdb_test_no_output "set exec-direction reverse"
+
+	gdb_test "continue" \
+	    [multi_line \
+		 "Continuing\\." \
+		 "" \
+		 "Breakpoint $decimal, main \\(\\) at .*/aarch64-mops.c:$decimal" \
+		 "$decimal\[ \t\]+__asm__ volatile \\(\"${insn_prefix}p \[^\r\n\]+\""] \
+	    "reversed execution of instruction sequence"
+
+	# Record the final register values.
+	set x19_final [capture_command_output "info register x19" ""]
+	set x21_final [capture_command_output "info register x21" ""]
+
+	if {[string compare $insn_prefix "set"] == 0} {
+	    set x22_final [capture_command_output "info register x22" ""]
+	} else {
+	    set x20_final [capture_command_output "info register x20" ""]
+	}
+
+	# Check initial and new values of x19 are different.
+	gdb_assert [string compare $x19_initial $x19_new] \
+	    "check x19 initial value versus x19 new value"
+
+	# Check initial and new values of x21 are different.
+	gdb_assert [string compare $x21_initial $x21_new] \
+	    "check x21 initial value versus x21 new value"
+
+	if {[string compare $insn_prefix "set"] == 0} {
+	    # Check initial and new values of x22 are the same.
+	    # The register with the value to set shouldn't change.
+	    gdb_assert ![string compare $x22_initial $x22_new] \
+		"check x22 initial value versus x22 new value"
+	} else {
+	    # Check initial and new values of x20 are different.
+	    gdb_assert [string compare $x20_initial $x20_new] \
+		"check x20 initial value versus x20 new value"
+	}
+
+	# Check initial and final values of x19 are the same.
+	gdb_assert ![string compare $x19_initial $x19_final] \
+	    "check x19 initial value versus x19 final value"
+
+	# Check initial and final values of x21 are the same.
+	gdb_assert ![string compare $x21_initial $x21_final] \
+	    "check x21 initial value versus x21 final value"
+
+	if {[string compare $insn_prefix "set"] == 0} {
+	    # Check initial and final values of x22 are the same.
+	    # The register with the value to set shouldn't change.
+	    gdb_assert ![string compare $x22_initial $x22_final] \
+		"check x22 initial value versus x22 final value"
+	} else {
+	    # Check initial and final values of x20 are the same.
+	    gdb_assert ![string compare $x20_initial $x20_final] \
+		"check x20 initial value versus x20 final value"
+	}
+
+	# Restore forward execution and go to end of recording.
+	gdb_test_no_output "set exec-direction forward"
+	gdb_test "record goto end" \
+	    [multi_line \
+		"Go forward to insn number $decimal" \
+		"#0  main \\(\\) at .*/aarch64-mops.c:$decimal" \
+		"$decimal\[ \t\]+p = dest;"]
+    }
+}
+
+do_test "set"
+do_test "cpy"
+do_test "cpyf"

Re: [PATCH v2 0/5] Add support for AArch64 MOPS instructions

[Luis Machado]

Hi Thiago,

Thanks for the series!

I just wanted to clarify the approach being used for this feature. From what I noticed,
we are implementing motion through these sequences of MOPS instructions as atomic,
right? Similar to the atomic sequences.

Is there a reason for that? From what I gathered, the Linux Kernel implements the
ability to single-step through each of the MOPS instructions. The catch is that
if the M and E instructions get interrupted, the sequence will get restarted, and
the debugger will need to be aware of that.

Was the atomic block approach for MOPS instructions used as a simplification?

I suppose displaced-stepping these sequences would be a bit tricky, but doable if
they were relocated as a block. I'm wondering what the performance impact would be
of requiring serialization of execution in a non-stop debugging scenario with
multiple threads.

One last point, doesn't record/replay require single-stepping each instruction
individually? Does that go against the atomic block approach?

I'll go stare at the code.

Regards,
Luis

On 5/7/24 03:22, Thiago Jung Bauermann wrote:
> Hello,
> 
> I'm sending v2 because Christophe made a suggestion for the
> gdb.arch/aarch64-mops-atomic-inst.exp testcase, so patch 4 incoroporates
> it.
> 
> The other patches are unchanged from v1.
> 
> Here is the original cover letter for convenience:
> 
> This patch series implements GDB support for the new instructions in
> AArch64's MOPS feature.  Patch 1 has a small overview.
> 
> What is needed from GDB is recognizing the MOPS sequences of instructions
> as atomic so that they can be stepped over during instruction single
> stepping, and also to avoid doing displaced stepping with them.  This is
> done in patch 1.
> 
> Patch 2 adds support for the new instructions to the record an replay
> target.
> 
> The other patches add testcases to test each of the aspects above, plus
> one testcase to verify the interaction of the MOPS instructions with
> watchpoints.
> 
> Tested on Ubuntu 23.10 aarch64-linux-gnu with no regressions, using the
> Arm FVP emulator as well as QEMU v8.2.
> 
> Thiago Jung Bauermann (5):
>   gdb/aarch64: Implement software single stepping for MOPS instructions
>   gdb/aarch64: Add record support for MOPS instructions.
>   gdb/testsuite: Add gdb.arch/aarch64-mops-watchpoint.exp
>   gdb/testsuite: Add gdb.arch/aarch64-mops-atomic-inst.exp
>   gdb/testsuite: Add gdb.reverse/aarch64-mops.exp
> 
>  gdb/aarch64-tdep.c                            | 191 +++++++++++++++++-
>  .../gdb.arch/aarch64-mops-atomic-inst.c       |  69 +++++++
>  .../gdb.arch/aarch64-mops-atomic-inst.exp     |  94 +++++++++
>  .../gdb.arch/aarch64-mops-watchpoint.c        |  66 ++++++
>  .../gdb.arch/aarch64-mops-watchpoint.exp      |  79 ++++++++
>  gdb/testsuite/gdb.reverse/aarch64-mops.c      |  71 +++++++
>  gdb/testsuite/gdb.reverse/aarch64-mops.exp    | 171 ++++++++++++++++
>  gdb/testsuite/lib/gdb.exp                     |  61 ++++++
>  8 files changed, 800 insertions(+), 2 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-atomic-inst.c
>  create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-atomic-inst.exp
>  create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.c
>  create mode 100644 gdb/testsuite/gdb.arch/aarch64-mops-watchpoint.exp
>  create mode 100644 gdb/testsuite/gdb.reverse/aarch64-mops.c
>  create mode 100644 gdb/testsuite/gdb.reverse/aarch64-mops.exp
> 
> 
> base-commit: 84a069db6714ddcf444095ed09dbcd7404834694

Re: [PATCH v2 0/5] Add support for AArch64 MOPS instructions

[Thiago Jung Bauermann]

Hello Luis,

Thank you for your comments!

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

> Hi Thiago,
>
> Thanks for the series!
>
> I just wanted to clarify the approach being used for this feature. From what I noticed,
> we are implementing motion through these sequences of MOPS instructions as atomic,
> right? Similar to the atomic sequences.

Yes, patch 1 makes GDB consider MOPS sequences as atomic.

> Is there a reason for that? From what I gathered, the Linux Kernel implements the
> ability to single-step through each of the MOPS instructions. The catch is that
> if the M and E instructions get interrupted, the sequence will get restarted, and
> the debugger will need to be aware of that.
>
> Was the atomic block approach for MOPS instructions used as a simplification?

No, it was just my mistaken interpretation of the Arm ARM. It says that
"the prologue, main, and epilogue instructions are expected to be run in
succession and to appear consecutively in memory", and I interpreted
that to mean that they're atomic. I didn't know that the kernel could
restart the sequence if the execution gets interrupted.
  
I tried to test how GDB reacts to the sequence getting restarted by
forcing the inferior to be scheduled to another CPU (by changing its CPU
affinity set) while the MOPS instructions were single-stepped, but I
wasn't able to trigger that behaviour.

Though thinking about it, I don't believe it would affect GDB in any
way. When GDB resumes the inferior, it will figure things out from
scratch when the inferior stops again so it doesn't make a difference
whether it stops in the next instruction or in the prologue instruction.
So IIUC I don't think we need to do anything specific about
single-stepping MOPS instructions then.

Maybe one thing that GDB would need to handle is when the user requests
a breakpoint in the main or epilogue instruction of the sequence. In
that case I think GDB would have to put the trap instruction either in
the prologue instruction or the one after the epilogue instruction. WDYT?

> I suppose displaced-stepping these sequences would be a bit tricky, but doable if
> they were relocated as a block. I'm wondering what the performance impact would be
> of requiring serialization of execution in a non-stop debugging scenario with
> multiple threads.

I didn't think of the possibility of relocating the sequence as a block,
so patch 1 also disables displaced stepping for MOPS instructions. Thank
you for the suggestion. I will implement your idea, though next week we
have the Linaro Connect conference so I think I'll only have something
about two weeks from now.

> One last point, doesn't record/replay require single-stepping each instruction
> individually? Does that go against the atomic block approach?

That's a good question. I'm not sure how the record and replay backend
deals with that, to be honest. But it does work, and I added a testcase
specifically to test reversing the MOPS sequences, and it passes both on
QEMU and FVP.

> I'll go stare at the code.

Thank you! You can skip all of the first patch except for the last hunk,
which disables displaced stepping for the MOPS instructions.

In the next few days I'll post a v3 where patch 1 does only that, and
make the corresponding adjustments the testcase in patch 4 which tests
single stepping.

After Linaro Connect I'll work on the displaced stepping support and (if
necessary) adjusting breakpoints in the middle of MOPS sequences.

-- 
Thiago

Re: [PATCH v2 0/5] Add support for AArch64 MOPS instructions

[Thiago Jung Bauermann]

Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:

> Maybe one thing that GDB would need to handle is when the user requests
> a breakpoint in the main or epilogue instruction of the sequence. In
> that case I think GDB would have to put the trap instruction either in
> the prologue instruction or the one after the epilogue instruction. WDYT?

I did some tests with the setp/setm/sete, cpyp/cpym/cpye and
cpyfp/cpyfm/cpyfe sequences and they all work fine if a software
breakpoint is set on their main or epilogue instructions. Tested on
Arm FVP and QEMU 8.2.

So at least for the emulators, adjusting the breakpoint address isn't
necessary.

-- 
Thiago

Re: [PATCH v2 0/5] Add support for AArch64 MOPS instructions

[Luis Machado]

On 5/10/24 06:20, Thiago Jung Bauermann wrote:
> Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:
> 
>> Maybe one thing that GDB would need to handle is when the user requests
>> a breakpoint in the main or epilogue instruction of the sequence. In
>> that case I think GDB would have to put the trap instruction either in
>> the prologue instruction or the one after the epilogue instruction. WDYT?
> 
> I did some tests with the setp/setm/sete, cpyp/cpym/cpye and
> cpyfp/cpyfm/cpyfe sequences and they all work fine if a software
> breakpoint is set on their main or epilogue instructions. Tested on
> Arm FVP and QEMU 8.2.
> 
> So at least for the emulators, adjusting the breakpoint address isn't
> necessary.
> 

Thanks for investingating that. Looks like it will be simpler then.

Regarding displaced stepping, we can disable displaced stepping temporarily
for these instructions. If we find a convenient way to relocate the whole block
of MOPS instructions, then we can add that later on.

Re: [PATCH v2 0/5] Add support for AArch64 MOPS instructions

[Guinevere Larsen]

[-- Attachment #1: Type: text/plain, Size: 1575 bytes --]

On 5/9/24 00:24, Thiago Jung Bauermann wrote:
>> One last point, doesn't record/replay require single-stepping each instruction
>> individually? Does that go against the atomic block approach?
> That's a good question. I'm not sure how the record and replay backend
> deals with that, to be honest. But it does work, and I added a testcase
> specifically to test reversing the MOPS sequences, and it passes both on
> QEMU and FVP.

On x86 we require it. That's because we record the current value of all 
registers of interest, including PC, which means if you tried to 
disassemble many instructions and record them all, you'd be recording 
only the PC of the first instruction many times.  I haven't looked at 
the Arm tdep stuff for record but I can't imagine it would be too 
different. If I'm right, the test works because you don't try to 
reverse-stepi to the middle of the asm statement, you just look at the 
full state before and after.

If GDB will treat the block as always atomic, then I guess we don't need 
to save the PCs of every instruction, so this approach is fine, but if 
people are expected to be able to stepi in between the blocks, reverse 
should be able to do the same. If things are atomic, should we try to 
warn users who try to stepi/reverse-stepi through those blocks?


Sidenote, I haven't had time to look at the code yet, but personally, 
I'd like to see the test introduced at the same commit where support was 
added, because that would make it easier to bisect in the future, if 
necessary :)

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

Re: [PATCH v2 0/5] Add support for AArch64 MOPS instructions

[Thiago Jung Bauermann]

Hello Guinevere,

Guinevere Larsen <blarsen@redhat.com> writes:

> On 5/9/24 00:24, Thiago Jung Bauermann wrote:
>
>>>  One last point, doesn't record/replay require single-stepping each instruction
>>> individually? Does that go against the atomic block approach?
>>
>> That's a good question. I'm not sure how the record and replay backend
>> deals with that, to be honest. But it does work, and I added a testcase
>> specifically to test reversing the MOPS sequences, and it passes both on
>> QEMU and FVP.
>
> On x86 we require it. That's because we record the current value of all registers of interest,
> including PC, which means if you tried to disassemble many instructions and record them all, you'd
> be recording only the PC of the first instruction many times.  I haven't looked at the Arm tdep
> stuff for record but I can't imagine it would be too different. If I'm right, the test works because
> you don't try to reverse-stepi to the middle of the asm statement, you just look at the full state
> before and after.

Thank you for the explanation!

> If GDB will treat the block as always atomic, then I guess we don't need to save the PCs of every
> instruction, so this approach is fine, but if people are expected to be able to stepi in between the
> blocks, reverse should be able to do the same. If things are atomic, should we try to warn users
> who try to stepi/reverse-stepi through those blocks?

For MOPS instructions specifically in later versions of the patch series
we're not treating them as atomic so this issue doesn't apply
anymore. But for other atomic instructions (e.g., acquire/release) if
the user stepi's (steps-i?) on the first instruction in the atomic
sequence, GDB sets a breakpoint at the end of the sequence and continues
the inferior until there. It doesn't show any warning or notice, though
perhaps that would be friendlier.

> Sidenote, I haven't had time to look at the code yet, but personally, I'd like to see the test
> introduced at the same commit where support was added, because that would make it easier to
> bisect in the future, if necessary :)

Makes sense. Done for v4.

-- 
Thiago