> > >> On 3 Jul 2020, at 16:36, Luis Machado 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 . +# +# 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