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: 2096 bytes --]

> 
> Two minor, but important, nits to fix:
> 
> Missing 2x Changelogs.
> 
...
> 
> Missing copyright file header.
> 
> 


Apologies, the ChangeLog is below, and I've added the copyright header that has
gone in aarch64-disp-stepping.s inline so you can check it easily.

The patch (created from `git format-patch`) is attached.


I don't have commit rights to GDB, so if this is OK could you apply the patch?


Thanks,
Matthew


ChangeLogs to apply:
------

gdb/ChangeLog:
2020-07-23  Matthew Malcomson  <matthew.malcomson@arm.com>

	* aarch64-tdep.c (aarch64_displaced_step_others): Account for
	BLR and BR instructions.
	* arch/aarch64-insn.h (enum aarch64_opcodes): Add BR opcode.
	(enum aarch64_masks): New.


gdb/testsuite/ChangeLog:
2020-07-23  Matthew Malcomson  <matthew.malcomson@arm.com>

	* gdb.arch/aarch64-disp-stepping.exp: New test runner.
	* gdb.arch/aarch64-disp-stepping.s: New test.


-------
Additional Copyright header added to aarch64-disp-stepping.s:


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..c7a67a12f250abee4250a1edfca411f25bc3a113
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.s
@@ -0,0 +1,111 @@
+/* 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 AArch64 instructions.  */
+


[-- Attachment #2: 0001-Patch-GDB-aarch64-Add-ability-to-displaced-step-over.patch --]
[-- Type: text/plain, Size: 12145 bytes --]

From 872cec9f53482429fbd547e13ac552d472bd002b Mon Sep 17 00:00:00 2001
From: Matthew Malcomson <matthew.malcomson@arm.com>
Date: Sun, 28 Jun 2020 13:06:36 +0100
Subject: [PATCH] [Patch] GDB: aarch64: Add ability to displaced step over a
 BR/BLR instruction

Enable displaced stepping over a BR/BLR instruction

Displaced stepping over an instruction executes a instruction in a
scratch area and then manually fixes up the PC address to leave
execution where it would have been if the instruction were in its
original location.

The BR instruction does not need modification in order to run correctly
at a different address, but the displaced step fixup method should not
manually adjust the PC since the BR instruction sets that value already.

The BLR instruction should also avoid such a fixup, but must also have
the link register modified to point to just after the original code
location rather than back to the scratch location.

This patch adds the above functionality.
We add this functionality by modifying aarch64_displaced_step_others
rather than by adding a new visitor method to aarch64_insn_visitor.
We choose this since it seems that visitor approach is designed
specifically for PC relative instructions (which must always be modified
when executed in a different location).

It seems that the BR and BLR instructions are more like the RET
instruction which is already handled specially in
aarch64_displaced_step_others.

This also means the gdbserver code to relocate an instruction when
creating a fast tracepoint does not need to be modified, since nothing
special is needed for the BR and BLR instructions there.

Manually tested on AArch64 and ensured new testcase passes.
Regression tests showed nothing untoward.

------#####
Observed (mis)behaviour before was that displaced stepping over a BR or
BLR instruction would not execute the function they called.  Most easily
seen by putting a breakpoint with a condition on such an instruction and
a print statement in the functions they called.
When run with the breakpoint enabled the function is not called and
"numargs called" is not printed.
When run with the breakpoint disabled the function is called and the
message is printed.

--- GDB Session
hw-a20-10:gcc-source [15:57:14] % gdb ../using-blr
Reading symbols from ../using-blr...done.
(gdb) disassemble blr_call_value
Dump of assembler code for function blr_call_value:
...
   0x0000000000400560 <+28>:    blr     x2
...
   0x00000000004005b8 <+116>:   ret
End of assembler dump.
(gdb) break *0x0000000000400560
Breakpoint 1 at 0x400560: file ../using-blr.c, line 22.
(gdb) condition 1 10 == 0
(gdb) run
Starting program: /home/matmal01/using-blr
[Inferior 1 (process 33279) exited with code 012]
(gdb) disable 1
(gdb) run
Starting program: /home/matmal01/using-blr
numargs called
[Inferior 1 (process 33289) exited with code 012]
(gdb)

Test program:
---- using-blr ----
\#include <stdio.h>
typedef int (foo) (int, int);
typedef void (bar) (int, int);
struct sls_testclass {
    foo *x;
    bar *y;
    int left;
    int right;
};

__attribute__ ((noinline))
int blr_call_value (struct sls_testclass x)
{
  int retval = x.x(x.left, x.right);
  if (retval % 10)
    return 100;
  return 9;
}

__attribute__ ((noinline))
int blr_call (struct sls_testclass x)
{
  x.y(x.left, x.right);
  if (x.left % 10)
    return 100;
  return 9;
}

int
numargs (__attribute__ ((unused)) int left, __attribute__ ((unused)) int right)
{
        printf("numargs called\n");
        return 10;
}

void
altfunc (__attribute__ ((unused)) int left, __attribute__ ((unused)) int right)
{
        printf("altfunc called\n");
}

int main(int argc, char **argv)
{
  struct sls_testclass x = { .x = numargs, .y = altfunc, .left = 1, .right = 2 };
  if (argc > 2)
  {
        blr_call (x);
  }
  else
        blr_call_value (x);
  return 10;
}

------

gdb/ChangeLog:
2020-07-23  Matthew Malcomson  <matthew.malcomson@arm.com>

	* aarch64-tdep.c (aarch64_displaced_step_others): Account for
	BLR and BR instructions.
	* arch/aarch64-insn.h (enum aarch64_opcodes): Add BR opcode.
	(enum aarch64_masks): New.

gdb/testsuite/ChangeLog:
2020-07-23  Matthew Malcomson  <matthew.malcomson@arm.com>

	* gdb.arch/aarch64-disp-stepping.exp: New test runner.
	* gdb.arch/aarch64-disp-stepping.s: New test.
---
 gdb/aarch64-tdep.c                               |  19 ++--
 gdb/arch/aarch64-insn.h                          |  10 ++
 gdb/testsuite/gdb.arch/aarch64-disp-stepping.exp |  65 +++++++++++++
 gdb/testsuite/gdb.arch/aarch64-disp-stepping.s   | 111 +++++++++++++++++++++++
 4 files changed, 199 insertions(+), 6 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-disp-stepping.exp
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-disp-stepping.s

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 5e7d0d0..d247108 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 6a63ce9..f261363 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 0000000..54eae61
--- /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 0000000..c7a67a1
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.s
@@ -0,0 +1,111 @@
+/* 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 AArch64 instructions.  */
+
+// 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
-- 
2.7.4