Re: [Patch] GDB: aarch64: Add ability to step over a BR/BLR instruction

[Matthew Malcomson <matthew.malcomson@arm.com>]

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

> 
> 
>> On 3 Jul 2020, at 16:36, Luis Machado <luis.machado@linaro.org> wrote:
> >
>> On 7/3/20 11:55 AM, Matthew Malcomson wrote:
> >> Manually tested on AArch64 (it doesn't look like there are tests for
> >> displaced stepping on the other instructions that are manually handled,
> >> so I figured adding a testcase for BR and BLR would be out of place).
> >
>> Not out of place, but those just did not get added. A test that exercises displaced stepping over those two instructions would be a good addition. That or some unit testing code to make sure the function handled the instruction in the expected way.
> 
> +1. Was going to write a very similar comment. A test for all the cases would be great.
> 

I've added tests for the BR and BLR instructions rather than for all
instructions due to time constraints ... I hope that's OK?

> >> ###############     Attachment also inlined for ease of reply    ###############
> >> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> >> index 5e7d0d0b8682af04ce4f01fd999d26c9eb459932..640a3e302f8e2b5fac3575e2f37212d40441d318 100644
> >> --- a/gdb/aarch64-tdep.c
> >> +++ b/gdb/aarch64-tdep.c
> >> @@ -2974,15 +2974,22 @@ aarch64_displaced_step_others (const uint32_t insn,
> >>    struct aarch64_displaced_step_data *dsd
> >>      = (struct aarch64_displaced_step_data *) data;
> >>  -  aarch64_emit_insn (dsd->insn_buf, insn);
> >> -  dsd->insn_count = 1;
> >> -
> >> -  if ((insn & 0xfffffc1f) == 0xd65f0000)
> 
> Maybe the 0xfffffc1f mask belongs in aarch64-insn.h
> 
> >> +  uint32_t masked_insn = (insn & 0xfffffc1f);

Done.




I'm not 100% confident on the approach of the testcase.
I tried to make it robust and clear, but may have missed a better approach.
(I used an assembly file to have easy control over the exact sequence of
instructions, and I think this is the nicest approach even though there are
only a few assembly files in the testsuite).


###############     Attachment also inlined for ease of reply    ###############


diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 5e7d0d0b8682af04ce4f01fd999d26c9eb459932..d247108f53bf045a018b2bf85284088563868ae0 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -2974,15 +2974,22 @@ aarch64_displaced_step_others (const uint32_t insn,
   struct aarch64_displaced_step_data *dsd
     = (struct aarch64_displaced_step_data *) data;
 
-  aarch64_emit_insn (dsd->insn_buf, insn);
-  dsd->insn_count = 1;
-
-  if ((insn & 0xfffffc1f) == 0xd65f0000)
+  uint32_t masked_insn = (insn & CLEAR_Rn_MASK);
+  if (masked_insn == BLR)
     {
-      /* RET */
-      dsd->dsc->pc_adjust = 0;
+      /* Emit a BR to the same register and then update LR to the original
+	 address (similar to aarch64_displaced_step_b).  */
+      aarch64_emit_insn (dsd->insn_buf, insn & 0xffdfffff);
+      regcache_cooked_write_unsigned (dsd->regs, AARCH64_LR_REGNUM,
+				      data->insn_addr + 4);
     }
   else
+    aarch64_emit_insn (dsd->insn_buf, insn);
+  dsd->insn_count = 1;
+
+  if (masked_insn == RET || masked_insn == BR || masked_insn == BLR)
+    dsd->dsc->pc_adjust = 0;
+  else
     dsd->dsc->pc_adjust = 4;
 }
 
diff --git a/gdb/arch/aarch64-insn.h b/gdb/arch/aarch64-insn.h
index 6a63ce9c2005acd6fe018a12c640f1be01751d6b..f261363feefe4e93e155434ba6d3df8e4b994c9f 100644
--- a/gdb/arch/aarch64-insn.h
+++ b/gdb/arch/aarch64-insn.h
@@ -40,7 +40,9 @@ enum aarch64_opcodes
   CBNZ            = 0x21000000 | B,
   TBZ             = 0x36000000 | B,
   TBNZ            = 0x37000000 | B,
+  /* BR             1101 0110 0001 1111 0000 00rr rrr0 0000 */
   /* BLR            1101 0110 0011 1111 0000 00rr rrr0 0000 */
+  BR              = 0xd61f0000,
   BLR             = 0xd63f0000,
   /* RET            1101 0110 0101 1111 0000 00rr rrr0 0000 */
   RET             = 0xd65f0000,
@@ -107,6 +109,14 @@ enum aarch64_opcodes
   NOP             = (0 << 5) | HINT,
 };
 
+/* List of useful masks.  */
+enum aarch64_masks
+{
+  /* Used for masking out an Rn argument from an opcode.  */
+  CLEAR_Rn_MASK = 0xfffffc1f,
+};
+
+
 /* Representation of a general purpose register of the form xN or wN.
 
    This type is used by emitting functions that take registers as operands.  */
diff --git a/gdb/testsuite/gdb.arch/aarch64-disp-stepping.exp b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.exp
new file mode 100644
index 0000000000000000000000000000000000000000..54eae61358c084d9342318591b7dbc57aa265ee4
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.exp
@@ -0,0 +1,65 @@
+# Copyright 2020 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 displaced stepping over BR and BLR instructions.
+
+if {![is_aarch64_target]} {
+    verbose "Skipping ${gdb_test_file_name}."
+    return
+}
+
+standard_testfile ".s"
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+    return -1
+}
+
+gdb_breakpoint "*blr_teststart"
+gdb_breakpoint "*blr_testcheck"
+gdb_breakpoint "*br_teststart"
+gdb_breakpoint "*br_testcheck"
+
+
+# Test for displaced stepping over the BLR instruction.
+gdb_test "run" \
+  "Starting program.*Breakpoint $decimal.*" \
+  "Run until BLR test start"
+
+set expected_lr [get_hexadecimal_valueof "\$pc + 4" 0]
+gdb_test "print/x \$x0" \
+  ".. = 0x0" \
+  "Ensure x0 is 0 before BLR test."
+
+gdb_continue_to_breakpoint "BLR test check"
+
+gdb_test "print/x \$lr == $expected_lr" \
+  ".. = 0x1" \
+  "Ensure LR is set to just after BLR."
+gdb_test "print/x \$x0" \
+  ".. = 0x1" \
+  "Ensure x0 is 1 after BLR test."
+
+
+# Test for displaced stepping over the BR instruction.
+gdb_continue_to_breakpoint "BR test start"
+
+gdb_test "print/x \$x0" \
+  ".. = 0x0" \
+  "Ensure x0 is 0 before BR test."
+gdb_continue_to_breakpoint "BR test check"
+gdb_test "print/x \$x0" \
+  ".. = 0x1" \
+  "Ensure x0 is 1 after BR test."
diff --git a/gdb/testsuite/gdb.arch/aarch64-disp-stepping.s b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.s
new file mode 100644
index 0000000000000000000000000000000000000000..fb67333271e649388a613fb9558f39ff30297697
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.s
@@ -0,0 +1,92 @@
+// Instructions not yet tested.
+// -  B
+// -  BL
+// -  B.COND
+// -  CBZ
+// -  CBNZ
+// -  TBZ
+// -  TBNZ
+// -  ADR
+// -  ADRP
+// -  LDR (literal)
+// -  RET
+
+// Function testing stepping over BLR instruction.
+	.text
+	.align	2
+	.global	test_blr_stepping
+	.type	test_blr_stepping, %function
+test_blr_stepping:
+	.cfi_startproc
+	// x2 Stores the old LR.
+	mov	x2,x30
+	// x0 is the indicator value to show whether the jump happened.
+	mov	x0, #0
+	// Load the jump position into register x1
+	movz	x1, :abs_g3:.LJUMPPOS
+	movk	x1, :abs_g2_nc:.LJUMPPOS
+	movk	x1, :abs_g1_nc:.LJUMPPOS
+	movk	x1, :abs_g0_nc:.LJUMPPOS
+blr_teststart:
+	blr	x1
+	b	blr_testcheck
+.LJUMPPOS:
+	mov	x0, #1
+blr_testcheck:
+	// Put the old LR value back into the LR register.
+	// Do this for both successful jump and unsuccessful jump since the LR
+	// will have changed both times and we want the program to continue
+	// properly both times.
+	mov	x30, x2
+	ret
+	.cfi_endproc
+	.size	test_blr_stepping, .-test_blr_stepping
+
+
+// Function testing stepping over BR instruction.
+	.text
+	.align	2
+	.global	test_br_stepping
+	.type	test_br_stepping, %function
+test_br_stepping:
+	.cfi_startproc
+	// x0 is the indicator value to show whether the jump happened.
+	mov	x0, #0
+	// Load the jump position into register x1
+	movz	x1, :abs_g3:.LJUMPPOS2
+	movk	x1, :abs_g2_nc:.LJUMPPOS2
+	movk	x1, :abs_g1_nc:.LJUMPPOS2
+	movk	x1, :abs_g0_nc:.LJUMPPOS2
+br_teststart:
+	br	x1
+	b	br_testcheck
+.LJUMPPOS2:
+	mov	x0, #1
+br_testcheck:
+	ret
+	.cfi_endproc
+	.size	test_br_stepping, .-test_br_stepping
+
+
+
+// Main function calling all test functions above.
+	.text
+	.align	2
+	.global	main
+	.type	main, %function
+main:
+	.cfi_startproc
+	stp	x29, x30, [sp, -16]!
+	.cfi_def_cfa_offset 16
+	.cfi_offset 29, -16
+	.cfi_offset 30, -8
+	bl	test_blr_stepping
+	bl	test_br_stepping
+	ldp	x29, x30, [sp], 16
+	.cfi_restore 30
+	.cfi_restore 29
+	.cfi_def_cfa_offset 0
+	ret
+	.cfi_endproc
+	.size	main, .-main
+	.section	.note.GNU-stack,"",@progbits


[-- Attachment #2: blr-patch.patch --]
[-- Type: text/plain, Size: 6822 bytes --]

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 5e7d0d0b8682af04ce4f01fd999d26c9eb459932..d247108f53bf045a018b2bf85284088563868ae0 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -2974,15 +2974,22 @@ aarch64_displaced_step_others (const uint32_t insn,
   struct aarch64_displaced_step_data *dsd
     = (struct aarch64_displaced_step_data *) data;
 
-  aarch64_emit_insn (dsd->insn_buf, insn);
-  dsd->insn_count = 1;
-
-  if ((insn & 0xfffffc1f) == 0xd65f0000)
+  uint32_t masked_insn = (insn & CLEAR_Rn_MASK);
+  if (masked_insn == BLR)
     {
-      /* RET */
-      dsd->dsc->pc_adjust = 0;
+      /* Emit a BR to the same register and then update LR to the original
+	 address (similar to aarch64_displaced_step_b).  */
+      aarch64_emit_insn (dsd->insn_buf, insn & 0xffdfffff);
+      regcache_cooked_write_unsigned (dsd->regs, AARCH64_LR_REGNUM,
+				      data->insn_addr + 4);
     }
   else
+    aarch64_emit_insn (dsd->insn_buf, insn);
+  dsd->insn_count = 1;
+
+  if (masked_insn == RET || masked_insn == BR || masked_insn == BLR)
+    dsd->dsc->pc_adjust = 0;
+  else
     dsd->dsc->pc_adjust = 4;
 }
 
diff --git a/gdb/arch/aarch64-insn.h b/gdb/arch/aarch64-insn.h
index 6a63ce9c2005acd6fe018a12c640f1be01751d6b..f261363feefe4e93e155434ba6d3df8e4b994c9f 100644
--- a/gdb/arch/aarch64-insn.h
+++ b/gdb/arch/aarch64-insn.h
@@ -40,7 +40,9 @@ enum aarch64_opcodes
   CBNZ            = 0x21000000 | B,
   TBZ             = 0x36000000 | B,
   TBNZ            = 0x37000000 | B,
+  /* BR             1101 0110 0001 1111 0000 00rr rrr0 0000 */
   /* BLR            1101 0110 0011 1111 0000 00rr rrr0 0000 */
+  BR              = 0xd61f0000,
   BLR             = 0xd63f0000,
   /* RET            1101 0110 0101 1111 0000 00rr rrr0 0000 */
   RET             = 0xd65f0000,
@@ -107,6 +109,14 @@ enum aarch64_opcodes
   NOP             = (0 << 5) | HINT,
 };
 
+/* List of useful masks.  */
+enum aarch64_masks
+{
+  /* Used for masking out an Rn argument from an opcode.  */
+  CLEAR_Rn_MASK = 0xfffffc1f,
+};
+
+
 /* Representation of a general purpose register of the form xN or wN.
 
    This type is used by emitting functions that take registers as operands.  */
diff --git a/gdb/testsuite/gdb.arch/aarch64-disp-stepping.exp b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.exp
new file mode 100644
index 0000000000000000000000000000000000000000..54eae61358c084d9342318591b7dbc57aa265ee4
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.exp
@@ -0,0 +1,65 @@
+# Copyright 2020 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 displaced stepping over BR and BLR instructions.
+
+if {![is_aarch64_target]} {
+    verbose "Skipping ${gdb_test_file_name}."
+    return
+}
+
+standard_testfile ".s"
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+    return -1
+}
+
+gdb_breakpoint "*blr_teststart"
+gdb_breakpoint "*blr_testcheck"
+gdb_breakpoint "*br_teststart"
+gdb_breakpoint "*br_testcheck"
+
+
+# Test for displaced stepping over the BLR instruction.
+gdb_test "run" \
+  "Starting program.*Breakpoint $decimal.*" \
+  "Run until BLR test start"
+
+set expected_lr [get_hexadecimal_valueof "\$pc + 4" 0]
+gdb_test "print/x \$x0" \
+  ".. = 0x0" \
+  "Ensure x0 is 0 before BLR test."
+
+gdb_continue_to_breakpoint "BLR test check"
+
+gdb_test "print/x \$lr == $expected_lr" \
+  ".. = 0x1" \
+  "Ensure LR is set to just after BLR."
+gdb_test "print/x \$x0" \
+  ".. = 0x1" \
+  "Ensure x0 is 1 after BLR test."
+
+
+# Test for displaced stepping over the BR instruction.
+gdb_continue_to_breakpoint "BR test start"
+
+gdb_test "print/x \$x0" \
+  ".. = 0x0" \
+  "Ensure x0 is 0 before BR test."
+gdb_continue_to_breakpoint "BR test check"
+gdb_test "print/x \$x0" \
+  ".. = 0x1" \
+  "Ensure x0 is 1 after BR test."
diff --git a/gdb/testsuite/gdb.arch/aarch64-disp-stepping.s b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.s
new file mode 100644
index 0000000000000000000000000000000000000000..fb67333271e649388a613fb9558f39ff30297697
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.s
@@ -0,0 +1,92 @@
+// Instructions not yet tested.
+// -  B
+// -  BL
+// -  B.COND
+// -  CBZ
+// -  CBNZ
+// -  TBZ
+// -  TBNZ
+// -  ADR
+// -  ADRP
+// -  LDR (literal)
+// -  RET
+
+// Function testing stepping over BLR instruction.
+	.text
+	.align	2
+	.global	test_blr_stepping
+	.type	test_blr_stepping, %function
+test_blr_stepping:
+	.cfi_startproc
+	// x2 Stores the old LR.
+	mov	x2,x30
+	// x0 is the indicator value to show whether the jump happened.
+	mov	x0, #0
+	// Load the jump position into register x1
+	movz	x1, :abs_g3:.LJUMPPOS
+	movk	x1, :abs_g2_nc:.LJUMPPOS
+	movk	x1, :abs_g1_nc:.LJUMPPOS
+	movk	x1, :abs_g0_nc:.LJUMPPOS
+blr_teststart:
+	blr	x1
+	b	blr_testcheck
+.LJUMPPOS:
+	mov	x0, #1
+blr_testcheck:
+	// Put the old LR value back into the LR register.
+	// Do this for both successful jump and unsuccessful jump since the LR
+	// will have changed both times and we want the program to continue
+	// properly both times.
+	mov	x30, x2
+	ret
+	.cfi_endproc
+	.size	test_blr_stepping, .-test_blr_stepping
+
+
+// Function testing stepping over BR instruction.
+	.text
+	.align	2
+	.global	test_br_stepping
+	.type	test_br_stepping, %function
+test_br_stepping:
+	.cfi_startproc
+	// x0 is the indicator value to show whether the jump happened.
+	mov	x0, #0
+	// Load the jump position into register x1
+	movz	x1, :abs_g3:.LJUMPPOS2
+	movk	x1, :abs_g2_nc:.LJUMPPOS2
+	movk	x1, :abs_g1_nc:.LJUMPPOS2
+	movk	x1, :abs_g0_nc:.LJUMPPOS2
+br_teststart:
+	br	x1
+	b	br_testcheck
+.LJUMPPOS2:
+	mov	x0, #1
+br_testcheck:
+	ret
+	.cfi_endproc
+	.size	test_br_stepping, .-test_br_stepping
+
+
+
+// Main function calling all test functions above.
+	.text
+	.align	2
+	.global	main
+	.type	main, %function
+main:
+	.cfi_startproc
+	stp	x29, x30, [sp, -16]!
+	.cfi_def_cfa_offset 16
+	.cfi_offset 29, -16
+	.cfi_offset 30, -8
+	bl	test_blr_stepping
+	bl	test_br_stepping
+	ldp	x29, x30, [sp], 16
+	.cfi_restore 30
+	.cfi_restore 29
+	.cfi_def_cfa_offset 0
+	ret
+	.cfi_endproc
+	.size	main, .-main
+	.section	.note.GNU-stack,"",@progbits