This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Only use 32-bit thumb-2 breakpoint instruction if necessary
- From: Yao Qi <qiyaoltc at gmail dot com>
- To: Antoine Tremblay <antoine dot tremblay at ericsson dot com>
- Cc: <gdb-patches at sourceware dot org>
- Date: Wed, 19 Apr 2017 15:25:16 +0100
- Subject: Re: [PATCH] Only use 32-bit thumb-2 breakpoint instruction if necessary
- Authentication-results: sourceware.org; auth=none
- References: <1491936739-14649-1-git-send-email-antoine.tremblay@ericsson.com>
Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
Hi Antoine,
> gdb/ChangeLog:
>
> PR server/21169
> * arch/arm-get-next-pcs.c:(thumb_get_next_pcs_raw_itblock):
> New function.
> (thumb_get_next_pcs_raw_itblock_1): New function.
> (thumb_get_next_pcs_raw): Move thumb2 IT block next pc logic
> to thumb_get_next_pcs_raw_itblock_1.
> * arch/arm-get-next-pcs.h: Include vector.
> (read_mem_uint_ftype): New typedef.
> (struct arm_get_next_pcs_ops): Use read_mem_uint_ftype typedef.
> (struct nextpc_itblock): New struct.
> (thumb_get_next_pcs_raw_itblock_1): New declaration.
> (thumb_get_next_pcs_raw_itblock): New declaration.
> * arm-tdep.c (arm_breakpoint_kind_from_pc): Use 16-bit breakpoints
> unless the instruction is in an IT block.
> (sefltest::arm_get_next_pcs_tests): New namespace.
> (sefltest::arm_get_next_pcs_tests::thumb_it_block_test):
> New namespace.
> (thumb_it_block_test::test): New function.
> (_initialize_arm_tdep): Register
> selftests::arm_get_next_pcs_tests::thumb_it_block_test::test function.
> (thumb_it_block_test::insns, insns_size): New variables.
> (thumb_it_block_test::read_mem_uint): New function.
> (thumb_it_block_test::test): New function.
>
> gdb/gdbserver/ChangeLog:
>
> PR server/21169
> * linux-aarch32-low.c: Include arm-get-next-pcs.h and vector.
> (arm_breakpoint_kind_from_current_state): Use
> thumb_get_next_pcs_raw_itblock to install a thumb-2 breakpoint
> only in an IT block.
> (get_next_pcs_read_memory_unsigned_integer): Move from linux-arm-low.c.
> * linux-aarch32-low.h (get_next_pcs_read_memory_unsigned_integer):
> New declaration.
> * linux-arm-low.c (get_next_pcs_read_memory_unsigned_integer):
> Moved to linux-aarch32-low.c.
> * mem-break.c (set_breakpoint_type_at): Call
> target_breakpoint_kind_from_current_state to get breakpoint kind
> for single_step breakpoint.
>
> gdb/testsuite/ChangeLog:
>
> PR server/21169
> * gdb.threads/arm-single-step.c: New file
> * gdb.threads/arm-single-step.exp: New file
"New file."
We need to split this patch into a patch set,
- Changes in arch/arm-get-next-pcs.c and unit tests,
- Changes in gdbserver,
- Changes in gdb,
- New test case gdb.threads/arm-single-step.exp,
>
> +/* See arm-get-next-pcs.h. */
> +
> +std::vector<nextpc_itblock>
> +thumb_get_next_pcs_raw_itblock (struct regcache *regcache,
> + read_mem_uint_ftype read_mem_uint,
> + int byte_order_for_code)
> +{
> + CORE_ADDR pc = regcache_read_pc (regcache);
> + ULONGEST status = regcache_raw_get_unsigned (regcache, ARM_PS_REGNUM);
> + ULONGEST itstate = ((status >> 8) & 0xfc) | ((status >> 25) & 0x3);
> +
> + return thumb_get_next_pcs_raw_itblock_1 (pc, status, itstate,
> + read_mem_uint,
> + byte_order_for_code);
> +}
> +
> +/* See arm-get-next-pcs.h. */
> +
> +std::vector<nextpc_itblock>
> +thumb_get_next_pcs_raw_itblock_1 (CORE_ADDR pc, ULONGEST status,
> + ULONGEST itstate,
> + read_mem_uint_ftype read_mem_uint,
> + int byte_order_for_code)
> +{
> + std::vector<nextpc_itblock> next_pcs;
> + unsigned short inst1 = read_mem_uint (pc, 2, byte_order_for_code);
> +
> + if ((inst1 & 0xff00) == 0xbf00 && (inst1 & 0x000f) != 0)
> + {
> + /* An IT instruction. Because this instruction does not
> + modify the flags, we can accurately predict the next
> + executed instruction. */
> + itstate = inst1 & 0x00ff;
> + pc += thumb_insn_size (inst1);
> +
> + while (itstate != 0 && ! condition_true (itstate >> 4, status))
> + {
> + inst1 = read_mem_uint (pc, 2,byte_order_for_code);
> + pc += thumb_insn_size (inst1);
> + itstate = thumb_advance_itstate (itstate);
> + }
> + /* The instruction is after the itblock if itstate != 0. */
> + next_pcs.push_back
> + (nextpc_itblock { MAKE_THUMB_ADDR (pc), itstate != 0 });
> + return next_pcs;
> + }
> + else if (itstate != 0)
> + {
> + /* We are in a conditional block. Check the condition. */
> + if (! condition_true (itstate >> 4, status))
> + {
> + /* Advance to the next executed instruction. */
> + pc += thumb_insn_size (inst1);
> + itstate = thumb_advance_itstate (itstate);
> +
> + while (itstate != 0 && ! condition_true (itstate >> 4, status))
> + {
> + inst1 = read_mem_uint (pc, 2, byte_order_for_code);
> +
> + pc += thumb_insn_size (inst1);
> + itstate = thumb_advance_itstate (itstate);
> + }
> +
> + /* The instruction is after the itblock if itstate != 0. */
> + next_pcs.push_back
> + (nextpc_itblock { MAKE_THUMB_ADDR (pc), itstate != 0 });
> + return next_pcs;
> + }
> + else if ((itstate & 0x0f) == 0x08)
> + {
> + /* This is the last instruction of the conditional
> + block, and it is executed. We can handle it normally
> + because the following instruction is not conditional,
> + and we must handle it normally because it is
> + permitted to branch. Fall through. */
"Fall through" is no longer valid. How about "Handle the instruction
normally."?
> diff --git a/gdb/arch/arm-get-next-pcs.h b/gdb/arch/arm-get-next-pcs.h
> index 2300ac1..cc8a6a7 100644
> --- a/gdb/arch/arm-get-next-pcs.h
> +++ b/gdb/arch/arm-get-next-pcs.h
> @@ -20,14 +20,18 @@
> #ifndef ARM_GET_NEXT_PCS_H
> #define ARM_GET_NEXT_PCS_H 1
> #include "gdb_vecs.h"
> +#include <vector>
>
> /* Forward declaration. */
> struct arm_get_next_pcs;
>
> +typedef ULONGEST (*read_mem_uint_ftype) (CORE_ADDR memaddr,
> + int len, int byte_order);
> +
> /* get_next_pcs operations. */
> struct arm_get_next_pcs_ops
> {
> - ULONGEST (*read_mem_uint) (CORE_ADDR memaddr, int len, int byte_order);
> + read_mem_uint_ftype read_mem_uint;
> CORE_ADDR (*syscall_next_pc) (struct arm_get_next_pcs *self);
> CORE_ADDR (*addr_bits_remove) (struct arm_get_next_pcs *self, CORE_ADDR val);
> int (*is_thumb) (struct arm_get_next_pcs *self);
> @@ -52,6 +56,16 @@ struct arm_get_next_pcs
> struct regcache *regcache;
> };
>
> +/* Contains the CORE_ADDR and if it's in an IT block.
> + To be returned by thumb_get_next_pcs_raw_itblock. */
> +struct nextpc_itblock
struct addr_in_itblock?
> +{
> + /* Next PC. */
> + CORE_ADDR nextpc;
s/nextpc/addr/ ?
> + /* Is this PC in an IT block. */
> + bool in_itblock;
> +};
> +
> /* Initialize arm_get_next_pcs. */
> void arm_get_next_pcs_ctor (struct arm_get_next_pcs *self,
> struct arm_get_next_pcs_ops *ops,
> @@ -63,4 +77,20 @@ void arm_get_next_pcs_ctor (struct arm_get_next_pcs *self,
> /* Find the next possible PCs after the current instruction executes. */
> VEC (CORE_ADDR) *arm_get_next_pcs (struct arm_get_next_pcs *self);
>
> +/* This is a subfunction of thumb_get_next_pcs_raw_itblock it is used to
> + provide a unit testable interface to
> + thumb_get_next_pcs_raw_itblock. */
> +
> +std::vector<nextpc_itblock>
> +thumb_get_next_pcs_raw_itblock_1 (CORE_ADDR pc, ULONGEST status,
> + ULONGEST itstate,
> + read_mem_uint_ftype read_mem_uint,
> + int byte_order_for_code);
> +
> +/* Return the next possible PCs after and if those are in a thumb2 it
> + block. */
> +std::vector<nextpc_itblock> thumb_get_next_pcs_raw_itblock
> +(struct regcache *regcache, read_mem_uint_ftype read_mem_uint,
> + int byte_order_for_code);
> +
> #endif /* ARM_GET_NEXT_PCS_H */
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index b3c3705..6133b75 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -7837,11 +7837,18 @@ arm_breakpoint_kind_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr)
>
> if (arm_pc_is_thumb (gdbarch, *pcptr))
> {
> + /* Check if we are in an IT block. */
> + CORE_ADDR adjusted_addr
> + = arm_adjust_breakpoint_address(gdbarch, *pcptr);
> +
> *pcptr = UNMAKE_THUMB_ADDR (*pcptr);
>
> - /* If we have a separate 32-bit breakpoint instruction for Thumb-2,
> - check whether we are replacing a 32-bit instruction. */
> - if (tdep->thumb2_breakpoint != NULL)
> + /* If we have a separate 32-bit breakpoint instruction for Thumb-2
> + check whether we are replacing a 32-bit instruction.
> +
> + Also check that the instruction at PCPTR is in an IT block. This
> + is needed to keep GDB and GDBServer breakpoint kinds in sync. */
> + if (adjusted_addr != *pcptr && tdep->thumb2_breakpoint != NULL)
We can't keep GDB and GDBserver in sync because we may have old gdb
talks with new gdbserver or new gdb talks with old gdbserver. If an old
gdb talks with a new gdbserver, gdb checks program_breakpoint_here_p by
reading an instruction and see if it is a breakpoint instruction. If
the original instruction is a 32-bit thumb instruction, gdbserver uses
16-bit thumb breakpoint, but gdb expects to match 32-bit thumb
breakpoint. IOW, gdb and gdbserver may see/use different kinds of
breakpoint on the same address (instruction), but gdb and gdbserver
can't do that.
>
> @@ -285,15 +288,53 @@ arm_sw_breakpoint_from_kind (int kind , int *size)
> int
> arm_breakpoint_kind_from_current_state (CORE_ADDR *pcptr)
> {
> - if (arm_is_thumb_mode ())
> + struct regcache *regcache = get_thread_regcache (current_thread, 1);
> + CORE_ADDR pc = regcache_read_pc (regcache);
> +
> + /* Two cases here:
> +
> + - If GDBServer is not single stepping then the PC is the current PC
> + and the PC doesn't contain the THUMB bit, even if it is a THUMB
> + instruction.
> +
> + - If single stepping, PCPTR is the next expected PC. In this case
> + PCPTR contains the THUMB bit if needed. GDBServer should not rely on
> + arm_is_thumb_mode in that case but only on the THUMB bit in the
> + PCPTR. */
> + if (((pc == *pcptr) && arm_is_thumb_mode ()) || IS_THUMB_ADDR (*pcptr))
> {
> - *pcptr = MAKE_THUMB_ADDR (*pcptr);
> - return arm_breakpoint_kind_from_pc (pcptr);
> + auto itblock_next_pcs = thumb_get_next_pcs_raw_itblock
> + (regcache, get_next_pcs_read_memory_unsigned_integer, 0);
> +
> + /* If this PC is in an itblock let arm_breakpoint_kind_from_pc
> + decide the kind. Otherwise always use a 2 byte breakpoint. */
> + for (const auto &nextpc : itblock_next_pcs)
> + {
> + if (MAKE_THUMB_ADDR (*pcptr) == nextpc.nextpc && nextpc.in_itblock)
> + return arm_breakpoint_kind_from_pc (pcptr);
> + }
In case #1, the condition "MAKE_THUMB_ADDR (*pcptr) == nextpc.nextpc" is
always false, so we end up returning ARM_BP_KIND_THUMB. It is incorrect
if *PCPTR is an address of 32-bit thumb instruction, and we use 32-bit
thumb breakpoint instruction.
> diff --git a/gdb/testsuite/gdb.threads/arm-single-step.exp b/gdb/testsuite/gdb.threads/arm-single-step.exp
> new file mode 100644
> index 0000000..0e97184
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/arm-single-step.exp
This test is quite arch specific, why don't we move it to gdb.arch/?
> @@ -0,0 +1,53 @@
> +# Copyright 2017 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 test checks that GDBServer doesn't crash the inferior while writing
> +# a breakpoint at an address that is aligned on 2 bytes but not on 4
> +# bytes.
> +# This file tests the partial resolution of PR server/21169.
> +
> +if {![istarget arm*-*eabi*]} then {
> + verbose "Skipping Thumb-2 threaded arm single-step tests."
> + return
> +}
> +
> +standard_testfile
> +set executable ${testfile}
> +
> +if { [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
> + untested "failed to compile"
> + return -1
> +}
> +
> +clean_restart $executable
> +
> +if ![runto_main] {
> + return -1
> +}
> +
> +gdb_test_no_output "set scheduler-locking off"
> +
> +# thumb2 is a misaligned 4 bytes instruction (the 2nd nop) it's forced to
> +# be at an address that is a multiple of 2, but not 4.
> +# itblock is the same but in an itblock.
> +
> +foreach_with_prefix inst { "thumb2" "itblock" } {
> + gdb_breakpoint [gdb_get_line_number "break $inst"] "temporary"
> + gdb_continue_to_breakpoint "break thumb2" ".* break $inst .*"
> + if { $inst == "itblock" } {
> + setup_kfail "server/21169" *-*-*
Both GDB and GDBserver has such problem, so "server/21169" makes no
sense, "tdep/21169" is better.
We run the tests twice with one inferior. If program crashes with
"thumb2", test with "itblock" won't be run. Better to restart gdb and
inferior in the loop.
> + }
> + gdb_test "step" ".*out_of_loop.*" "stepping $inst"
> +}
--
Yao (齐尧)