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

[Alan Hayward <Alan.Hayward@arm.com>]

> On 20 Jul 2020, at 12:13, Matthew Malcomson <Matthew.Malcomson@arm.com> wrote:
> 
>> 
>> 
>>> 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?

Sure, that’s fine.

> 
>>>> ###############     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).

Your approach ensures that the functions have the correct instructions, so I
think that’s ok. Alternatively asm in a C file might have worked too.


Two minor, but important, nits to fix:

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

Missing 2x Changelogs.

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

Missing copyright file header.


> @@ -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
> 
> <blr-patch.patch>