| Message ID | 1491936739-14649-1-git-send-email-antoine.tremblay@ericsson.com |
|---|---|
| State | New |
| Headers | show |
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 Qi writes: > 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, OK. > >> >> +/* 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."? Yes. I did not want to modify existing code in the move... But I can make it a small patch before. > >> 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? > OK. >> +{ >> + /* Next PC. */ >> + CORE_ADDR nextpc; > > s/nextpc/addr/ ? > OK. >> + /* 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. Right. Do you see any possible solution to this ? The only thing I would see is to accept inconsistent breakpoint sizes GDBServer. I'm not sure anyway why that size/check is there assuming GDB/GDBServer is the only one writing breakpoints in the program ? > >> >> @@ -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. > Yes indeed will fix. >> 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/? > I had it in threads was we had discussed it before since it uses threads, but I'm ok with 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. > Right. will fix. > 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. > OK. Thanks, Antoine
Antoine Tremblay <antoine.tremblay@ericsson.com> writes: >> 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. > > Right. Do you see any possible solution to this ? > I don't have any solutions off hand. Nowadays, GDB chooses the breakpoint kind by address and the instruction on that address in arm_breakpoint_kind_from_pc, if (target_read_memory (*pcptr, buf, 2) == 0) { unsigned short inst1; inst1 = extract_unsigned_integer (buf, 2, byte_order_for_code); if (thumb_insn_size (inst1) == 4) return ARM_BP_KIND_THUMB2; } we can change it to show breakpoints in target_read_memory, /* Make sure we see the memory breakpoints. */ cleanup = make_show_memory_breakpoints_cleanup (1); if (target_read_memory (*pcptr, buf, 2) == 0) { do_cleanups (cleanup); /* The remote stub may choose 16-bit thumb breakpoint for 32-bit thumb instruction if the instruction is out side of IT block. */ if (memcmp (buf, tdep->thumb_breakpoint) == 0) return ARM_BP_KIND_THUMB; unsigned short inst1; inst1 = extract_unsigned_integer (buf, 2, byte_order_for_code); // Suppose thumb_insn_size returns 4 for the first half of 32-bit // thumb-2 breakpoint. if (thumb_insn_size (inst1) == 4) return ARM_BP_KIND_THUMB2; } do_cleanups (cleanup); > The only thing I would see is to accept inconsistent breakpoint > sizes GDBServer. In GDBserver, all breakpoints (gdb breakpoints, single-step breakpoints) are organized via raw_breakpoint, is it possible that 32-bit thumb breakpoint is use for gdb breakpoint and 16-bit thumb breakpoint is used for single-step breakpoint for the same address? It makes troubles to raw_breakpoint management, as we find a raw_breakpoint for a given kind on a given address. I don't know what GDBserver should do, so I can't give much help here, sorry. I would like to change GDBserver code, and fix the regressions caused by the change. > > I'm not sure anyway why that size/check is there assuming > GDB/GDBServer is the only one writing breakpoints in the program ? Because so far, arm is the only target in which we can use different kind breakpoint instructions for a given address.
Yao Qi writes: > Antoine Tremblay <antoine.tremblay@ericsson.com> writes: > >>> 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. >> >> Right. Do you see any possible solution to this ? >> > > I don't have any solutions off hand. Nowadays, GDB chooses the > breakpoint kind by address and the instruction on that address > in arm_breakpoint_kind_from_pc, > > if (target_read_memory (*pcptr, buf, 2) == 0) > { > unsigned short inst1; > > inst1 = extract_unsigned_integer (buf, 2, byte_order_for_code); > if (thumb_insn_size (inst1) == 4) > return ARM_BP_KIND_THUMB2; > } > > we can change it to show breakpoints in target_read_memory, > > /* Make sure we see the memory breakpoints. */ > cleanup = make_show_memory_breakpoints_cleanup (1); > if (target_read_memory (*pcptr, buf, 2) == 0) > { > do_cleanups (cleanup); > > /* The remote stub may choose 16-bit thumb breakpoint for > 32-bit thumb instruction if the instruction is out side > of IT block. */ > if (memcmp (buf, tdep->thumb_breakpoint) == 0) > return ARM_BP_KIND_THUMB; > > unsigned short inst1; > > inst1 = extract_unsigned_integer (buf, 2, byte_order_for_code); > // Suppose thumb_insn_size returns 4 for the first half of 32-bit > // thumb-2 breakpoint. > if (thumb_insn_size (inst1) == 4) > return ARM_BP_KIND_THUMB2; > } > do_cleanups (cleanup); > Yes that would help... >> The only thing I would see is to accept inconsistent breakpoint >> sizes GDBServer. > > In GDBserver, all breakpoints (gdb breakpoints, single-step breakpoints) > are organized via raw_breakpoint, is it possible that 32-bit thumb > breakpoint is use for gdb breakpoint and 16-bit thumb breakpoint is used > for single-step breakpoint for the same address? Yes, tests reveal that this is an issue and fail with the breakpoint removed because of its inconsistent kind. > It makes troubles to > raw_breakpoint management, as we find a raw_breakpoint for a given kind > on a given address.I don't know what GDBserver should do, so I can't > give much help here, sorry. Yes at first I though what's the harm in allowing different breakpoint kinds on an address ? But looking at the code that introduced the check there seems to be complex reasons for that. >I would like to change GDBserver code, and fix the regressions caused by the change. I'm not sure what you mean ? You want to take over this issue ? (not a problem with me) I think we may forget this change for 8.0 however... ?
Antoine Tremblay <antoine.tremblay@ericsson.com> writes: > Yes at first I though what's the harm in allowing different breakpoint > kinds on an address ? There is nothing harmful, IMO. It is a matter of complexity. > > But looking at the code that introduced the check there seems to be complex > reasons for that. > >>I would like to change GDBserver code, and fix the regressions caused >> by the change. > > I'm not sure what you mean ? You want to take over this issue ? (not a > problem with me) No, I have no plan to take this issue over. I mean, if I were you, I'd start from change GDBserver code, fix the regressions, etc. > > I think we may forget this change for 8.0 however... ? I feel it is difficult to catch 8.0 release, as there are still some open problems that we don't have ideas yet.
Yao Qi writes: > Antoine Tremblay <antoine.tremblay@ericsson.com> writes: > >> Yes at first I though what's the harm in allowing different breakpoint >> kinds on an address ? > > There is nothing harmful, IMO. It is a matter of complexity. > >> >> But looking at the code that introduced the check there seems to be complex >> reasons for that. >> >>>I would like to change GDBserver code, and fix the regressions caused >>> by the change. >> >> I'm not sure what you mean ? You want to take over this issue ? (not a >> problem with me) > > No, I have no plan to take this issue over. I mean, if I were you, I'd > start from change GDBserver code, fix the regressions, etc. > >> >> I think we may forget this change for 8.0 however... ? > > I feel it is difficult to catch 8.0 release, as there are still some open > problems that we don't have ideas yet. Since this won't be fixed for 8.0 and that I don't know when I will rework on this, would it make sense to revert the software single step in GDBServer ? Basically as it is now software single step has problems in GDB and GDBServer where as before 7.11 iirc at least GDBServer was working correctly. Regards, Antoine
diff --git a/gdb/arch/arm-get-next-pcs.c b/gdb/arch/arm-get-next-pcs.c index 398dd59a..c9be7e0 100644 --- a/gdb/arch/arm-get-next-pcs.c +++ b/gdb/arch/arm-get-next-pcs.c @@ -258,6 +258,125 @@ arm_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self) return next_pcs; } +/* 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. */ + } + else + { + int cond_negated; + + /* There are conditional instructions after this one. + If this instruction modifies the flags, then we can + not predict what the next executed instruction will + be. Fortunately, this instruction is architecturally + forbidden to branch; we know it will fall through. + Start by skipping past it. */ + pc += thumb_insn_size (inst1); + itstate = thumb_advance_itstate (itstate); + + /* Set a breakpoint on the following instruction. */ + gdb_assert ((itstate & 0x0f) != 0); + next_pcs.push_back (nextpc_itblock { MAKE_THUMB_ADDR (pc), true }); + + cond_negated = (itstate >> 4) & 1; + + /* Skip all following instructions with the same + condition. If there is a later instruction in the IT + block with the opposite condition, set the other + breakpoint there. If not, then set a breakpoint on + the instruction after the IT block. */ + do + { + inst1 = read_mem_uint (pc, 2, byte_order_for_code); + pc += thumb_insn_size (inst1); + itstate = thumb_advance_itstate (itstate); + } + while (itstate != 0 && ((itstate >> 4) & 1) == cond_negated); + + /* The instruction is after the itblock if itstate != 0. */ + next_pcs.push_back + (nextpc_itblock { MAKE_THUMB_ADDR (pc), itstate != 0 }); + + return next_pcs; + } + } + + return next_pcs; +} + /* Find the next possible PCs for thumb mode. */ static VEC (CORE_ADDR) * @@ -300,89 +419,14 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self) if (self->has_thumb2_breakpoint) { - 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); + auto itblock_next_pcs = thumb_get_next_pcs_raw_itblock + (regcache, self->ops->read_mem_uint, byte_order_for_code); - while (itstate != 0 && ! condition_true (itstate >> 4, status)) - { - inst1 = self->ops->read_mem_uint (pc, 2,byte_order_for_code); - pc += thumb_insn_size (inst1); - itstate = thumb_advance_itstate (itstate); - } + for (const auto &it_nextpc : itblock_next_pcs) + VEC_safe_push (CORE_ADDR, next_pcs, it_nextpc.nextpc); - VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc)); - 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 = self->ops->read_mem_uint (pc, 2, byte_order_for_code); - - pc += thumb_insn_size (inst1); - itstate = thumb_advance_itstate (itstate); - } - - VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc)); - 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. */ - } - else - { - int cond_negated; - - /* There are conditional instructions after this one. - If this instruction modifies the flags, then we can - not predict what the next executed instruction will - be. Fortunately, this instruction is architecturally - forbidden to branch; we know it will fall through. - Start by skipping past it. */ - pc += thumb_insn_size (inst1); - itstate = thumb_advance_itstate (itstate); - - /* Set a breakpoint on the following instruction. */ - gdb_assert ((itstate & 0x0f) != 0); - VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc)); - - cond_negated = (itstate >> 4) & 1; - - /* Skip all following instructions with the same - condition. If there is a later instruction in the IT - block with the opposite condition, set the other - breakpoint there. If not, then set a breakpoint on - the instruction after the IT block. */ - do - { - inst1 = self->ops->read_mem_uint (pc, 2, byte_order_for_code); - pc += thumb_insn_size (inst1); - itstate = thumb_advance_itstate (itstate); - } - while (itstate != 0 && ((itstate >> 4) & 1) == cond_negated); - - VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc)); - - return next_pcs; - } - } + if (!itblock_next_pcs.empty ()) + return next_pcs; } else if (itstate & 0x0f) { 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 +{ + /* Next PC. */ + CORE_ADDR nextpc; + /* 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) { gdb_byte buf[2]; @@ -9592,7 +9599,14 @@ arm_dump_tdep (struct gdbarch *gdbarch, struct ui_file *file) namespace selftests { static void arm_record_test (void); -} +namespace arm_get_next_pcs_tests +{ +namespace thumb_it_block_test +{ +static void test (void); +} /* namespace thumb_it_block_test */ +} /* namespace arm_get_next_pcs_tests */ +} /* namespace selftests */ extern initialize_file_ftype _initialize_arm_tdep; /* -Wmissing-prototypes */ @@ -9733,6 +9747,8 @@ vfp - VFP co-processor."), #if GDB_SELF_TEST register_self_test (selftests::arm_record_test); + register_self_test + (selftests::arm_get_next_pcs_tests::thumb_it_block_test::test); #endif } @@ -13211,7 +13227,235 @@ arm_record_test (void) SELF_CHECK (arm_record.arm_regs[0] == 7); } } -} // namespace selftests + + +/* Namespace for arm_get_gext_pcs test series. */ +namespace arm_get_next_pcs_tests +{ +namespace thumb_it_block_test +{ +/* Pointer to instruction array to test. */ +static const gdb_byte *insns; +/* Size of the instruction array. */ +static int insns_size; + +/* Mockup read function for the test function. */ +static ULONGEST +read_mem_uint (CORE_ADDR memaddr, int len, int byte_order) +{ + gdb_byte buf[sizeof (ULONGEST)]; + + if (memaddr + len > insns_size || len > sizeof (ULONGEST)) + return 0; + + memcpy (buf, insns + memaddr, len); + return extract_unsigned_integer (buf, len, (enum bfd_endian) byte_order); +} + +/* Test getting the next PCs when dealing with an IT block. */ + +static void +test () +{ + /* Instructions with an IT block and an instruction after the IT block. */ + const gdb_byte itte_insns[] = { + /* itte eq */ + 0x06, 0xbf, + /* nopeq */ + 0x00, 0xbf, + /* nopeq */ + 0x00, 0xbf, + /* nopne */ + 0x00, 0xbf, + /* nop */ + 0x00, 0xbf, + }; + + insns = itte_insns; + insns_size = sizeof (itte_insns); + + /* Test that if PC is at IT instruction the next PC is the first one + that validates the condition in the IT block. */ + { + CORE_ADDR pc = 0; + /* Simulate CMP that is Equal. */ + ULONGEST status = FLAG_Z; + ULONGEST itstate = insns[0] & 0x00ff; + auto next_pcs = thumb_get_next_pcs_raw_itblock_1 + (pc, status, itstate, read_mem_uint, 1); + + SELF_CHECK (next_pcs.size () == 1); + SELF_CHECK (next_pcs.back ().nextpc == MAKE_THUMB_ADDR (0x02)); + SELF_CHECK (next_pcs.back ().in_itblock); + } + + /* Test that if PC is in an IT block and the instruction at PC does not + validate the condition that the next PC is the one that does validate + the condition. */ + { + CORE_ADDR pc = 0x2; + /* Simulate CMP that Not Equal. */ + ULONGEST status = ~FLAG_Z; + ULONGEST itstate = insns[0] & 0x00ff; + auto next_pcs = thumb_get_next_pcs_raw_itblock_1 + (pc, status, itstate, read_mem_uint, 1); + + SELF_CHECK (next_pcs.size () == 1); + SELF_CHECK (next_pcs.back ().nextpc == MAKE_THUMB_ADDR (0x06)); + SELF_CHECK (next_pcs.back ().in_itblock); + } + + /* Test that if PC is at the last instruction of an IT block and it is + executed. No next PC is returned from + thumb_get_next_pcs_raw_itblock_1. */ + { + CORE_ADDR pc = 0x6; + /* Simulate CMP that is Not Equal. */ + ULONGEST status = ~FLAG_Z; + ULONGEST itstate = insns[0] & 0x00ff; + + /* Get the itstate for the last instruction. */ + for (int i = 0; i < 2; i++) + itstate = thumb_advance_itstate (itstate); + + auto next_pcs = thumb_get_next_pcs_raw_itblock_1 + (pc, status, itstate, read_mem_uint, 1); + + SELF_CHECK (next_pcs.empty ()); + } + + /* Test that if PC is at the last instruction of an IT block and it is + not executed. The next PC is after the itblock. */ + { + CORE_ADDR pc = 0x6; + /* Simulate CMP that is Equal. */ + ULONGEST status = FLAG_Z; + ULONGEST itstate = insns[0] & 0x00ff; + + /* Get the itstate for the last instruction. */ + for (int i = 0; i < 2; i++) + itstate = thumb_advance_itstate (itstate); + + auto next_pcs = thumb_get_next_pcs_raw_itblock_1 + (pc, status, itstate, read_mem_uint, 1); + + SELF_CHECK (next_pcs.size () == 1); + SELF_CHECK (next_pcs.back ().nextpc == MAKE_THUMB_ADDR (0x8)); + SELF_CHECK (!next_pcs.back ().in_itblock); + } + + /* Test that if PC is at the last instruction of an IT block and it is + executed. No next_pc is returned from + thumb_get_next_pcs_raw_itblock_1. */ + { + CORE_ADDR pc = 0x6; + /* Simulate CMP that Not Equal. */ + ULONGEST status = ~FLAG_Z; + ULONGEST itstate = insns[0] & 0x00ff; + + /* Get the itstate for the last instruction. */ + for (int i = 0; i < 2; i++) + itstate = thumb_advance_itstate (itstate); + + auto next_pcs = thumb_get_next_pcs_raw_itblock_1 + (pc, status, itstate, read_mem_uint, 1); + + SELF_CHECK (next_pcs.empty ()); + } + + /* Test that if PC is stopped at an instruction that validates the + condition, the next PCs returned are the next instruction and the next + instruction that does not validate the condition, since the + instruction at PC could modify the flags. */ + { + CORE_ADDR pc = 0x2; + /* Simulate CMP that Equal. */ + ULONGEST status = FLAG_Z; + ULONGEST itstate = insns[0] & 0x00ff; + + auto next_pcs = thumb_get_next_pcs_raw_itblock_1 + (0x2, status, itstate, read_mem_uint, 1); + + SELF_CHECK (next_pcs.size () == 2); + SELF_CHECK (next_pcs.front ().nextpc == MAKE_THUMB_ADDR (0x4)); + SELF_CHECK (next_pcs.front ().in_itblock); + SELF_CHECK (next_pcs.back ().nextpc == MAKE_THUMB_ADDR (0x6)); + SELF_CHECK (next_pcs.back ().in_itblock); + } + + /* Instructions with IT block created by an itt instruction and an + instruction after the IT block. */ + const gdb_byte itt_insns[] = { + /* itt eq */ + 0x04, 0xbf, + /* nopeq */ + 0x00, 0xbf, + /* nopeq */ + 0x00, 0xbf, + /* nop */ + 0x00, 0xbf, + }; + + insns = itt_insns; + insns_size = sizeof (itt_insns); + + /* Test that if PC is at IT instruction and there's no instruction that + validates the condition in the IT block then the next PC is outside + of the it block. */ + { + CORE_ADDR pc = 0; + /* Simulate CMP that is Not Equal. */ + ULONGEST status = ~FLAG_Z; + ULONGEST itstate = insns[0] & 0x00ff; + auto next_pcs = thumb_get_next_pcs_raw_itblock_1 + (pc, status, itstate, read_mem_uint, 1); + + SELF_CHECK (next_pcs.size () == 1); + SELF_CHECK (next_pcs.back ().nextpc == MAKE_THUMB_ADDR (0x06)); + SELF_CHECK (!next_pcs.back ().in_itblock); + } + + /* Test that if PC is in an IT block and the instruction at PC does not + validate the condition and that there is no instruction that does in + the IT block that the next instruction is outside of the IT + block. */ + { + CORE_ADDR pc = 0x2; + /* Simulate CMP that Not Equal. */ + ULONGEST status = ~FLAG_Z; + ULONGEST itstate = insns[0] & 0x00ff; + auto next_pcs = thumb_get_next_pcs_raw_itblock_1 + (pc, status, itstate, read_mem_uint, 1); + + SELF_CHECK (next_pcs.size () == 1); + SELF_CHECK (next_pcs.back ().nextpc == MAKE_THUMB_ADDR (0x06)); + SELF_CHECK (!next_pcs.back ().in_itblock); + } + + /* Test that if PC is stopped at an instruction that validates the + condition the next PCs returned are the next instruction and the + instruction after the IT block since in this case there's no + instruction that would not validate the condition in the IT + block. */ + { + + CORE_ADDR pc = 0x2; + /* Simulate CMP that is Equal. */ + ULONGEST status = FLAG_Z; + ULONGEST itstate = insns[0] & 0x00ff; + auto next_pcs = thumb_get_next_pcs_raw_itblock_1 + (pc, status, itstate, read_mem_uint, 1); + + SELF_CHECK (next_pcs.size () == 2); + SELF_CHECK (next_pcs.front ().nextpc == MAKE_THUMB_ADDR (0x4)); + SELF_CHECK (next_pcs.front ().in_itblock); + SELF_CHECK (next_pcs.back ().nextpc == MAKE_THUMB_ADDR (0x6)); + SELF_CHECK (!next_pcs.back ().in_itblock); + } +} +} /* namespace thumb_it_block_test */ +} /* namespace arm_thumb_get_pcs_tests */ +} /* namespace selftests */ #endif /* GDB_SELF_TEST */ /* Cleans up local record registers and memory allocations. */ diff --git a/gdb/gdbserver/linux-aarch32-low.c b/gdb/gdbserver/linux-aarch32-low.c index 2b710ba..30b7d1a 100644 --- a/gdb/gdbserver/linux-aarch32-low.c +++ b/gdb/gdbserver/linux-aarch32-low.c @@ -18,6 +18,7 @@ #include "server.h" #include "arch/arm.h" #include "arch/arm-linux.h" +#include "arch/arm-get-next-pcs.h" #include "linux-low.h" #include "linux-aarch32-low.h" @@ -28,6 +29,8 @@ #include <elf.h> #endif +#include <vector> + /* Correct in either endianness. */ #define arm_abi_breakpoint 0xef9f0001UL @@ -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); + } + + *pcptr = UNMAKE_THUMB_ADDR (*pcptr); + return ARM_BP_KIND_THUMB; } else - { - return arm_breakpoint_kind_from_pc (pcptr); - } + return arm_breakpoint_kind_from_pc (pcptr); +} + +/* Read memory from the inferior. + BYTE_ORDER is ignored and there to keep compatiblity with GDB's + read_memory_unsigned_integer. */ +ULONGEST +get_next_pcs_read_memory_unsigned_integer (CORE_ADDR memaddr, + int len, + int byte_order) +{ + ULONGEST res; + + res = 0; + target_read_memory (memaddr, (unsigned char *) &res, len); + + return res; } void diff --git a/gdb/gdbserver/linux-aarch32-low.h b/gdb/gdbserver/linux-aarch32-low.h index fecfcbe..77fca32 100644 --- a/gdb/gdbserver/linux-aarch32-low.h +++ b/gdb/gdbserver/linux-aarch32-low.h @@ -27,6 +27,9 @@ int arm_breakpoint_kind_from_pc (CORE_ADDR *pcptr); const gdb_byte *arm_sw_breakpoint_from_kind (int kind , int *size); int arm_breakpoint_kind_from_current_state (CORE_ADDR *pcptr); int arm_breakpoint_at (CORE_ADDR where); +ULONGEST get_next_pcs_read_memory_unsigned_integer (CORE_ADDR memaddr, + int len, + int byte_order); void initialize_low_arch_aarch32 (void); diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c index fc2b348..fc6b5cc 100644 --- a/gdb/gdbserver/linux-arm-low.c +++ b/gdb/gdbserver/linux-arm-low.c @@ -139,11 +139,6 @@ static int arm_regmap[] = { 64 }; -/* Forward declarations needed for get_next_pcs ops. */ -static ULONGEST get_next_pcs_read_memory_unsigned_integer (CORE_ADDR memaddr, - int len, - int byte_order); - static CORE_ADDR get_next_pcs_addr_bits_remove (struct arm_get_next_pcs *self, CORE_ADDR val); @@ -252,22 +247,6 @@ get_next_pcs_is_thumb (struct arm_get_next_pcs *self) return arm_is_thumb_mode (); } -/* Read memory from the inferiror. - BYTE_ORDER is ignored and there to keep compatiblity with GDB's - read_memory_unsigned_integer. */ -static ULONGEST -get_next_pcs_read_memory_unsigned_integer (CORE_ADDR memaddr, - int len, - int byte_order) -{ - ULONGEST res; - - res = 0; - target_read_memory (memaddr, (unsigned char *) &res, len); - - return res; -} - /* Fetch the thread-local storage pointer for libthread_db. */ ps_err_e diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c index 6e6926a..1e04063 100644 --- a/gdb/gdbserver/mem-break.c +++ b/gdb/gdbserver/mem-break.c @@ -855,7 +855,16 @@ set_breakpoint_type_at (enum bkpt_type type, CORE_ADDR where, { int err_ignored; CORE_ADDR placed_address = where; - int breakpoint_kind = target_breakpoint_kind_from_pc (&placed_address); + int breakpoint_kind; + + /* Get the kind of breakpoint to PLACED_ADDRESS except single-step + breakpoint. Get the kind of single-step breakpoint according to + the current register state. */ + if (type == single_step_breakpoint) + breakpoint_kind + = target_breakpoint_kind_from_current_state (&placed_address); + else + breakpoint_kind = target_breakpoint_kind_from_pc (&placed_address); return set_breakpoint (type, raw_bkpt_type_sw, placed_address, breakpoint_kind, handler, diff --git a/gdb/testsuite/gdb.threads/arm-single-step.c b/gdb/testsuite/gdb.threads/arm-single-step.c new file mode 100644 index 0000000..9783f33 --- /dev/null +++ b/gdb/testsuite/gdb.threads/arm-single-step.c @@ -0,0 +1,106 @@ +/* This testcase is part of GDB, the GNU debugger. + + 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/>. */ + +#include <pthread.h> +#include <unistd.h> +#include <stdlib.h> +#include <signal.h> + +#define NUM_THREADS 2 +#define TIMEOUT 5 + +static pthread_t child_thread[NUM_THREADS]; +static volatile int run = 1; + +static void +handler (int signo) +{ + run = 0; +} + +/* Misalign a 4 bytes instruction (the 2nd nop) on purpose. Force it to + be at an address that is a multiple of 2, but not 4. */ +#define THUMB2_INST \ + asm (" .align 4 \n" \ + " nop.n \n" \ + " nop.w \n" \ + ) \ + +#define ITBLOCK \ + asm (" .align 4 \n" \ + " nop\n" \ + " cmp r0, #0\n" \ + " itte eq\n" \ + " nop.w \n" \ + " nop.w \n" \ + " nop.w \n" \ + " nop \n" \ + ) \ + +#define LOOP(macro) \ + while (run) \ + macro; \ + +static void +out_of_loop () +{ + return; +} + +static void * +thumb2_function (void *arg) +{ + LOOP (THUMB2_INST); /* break thumb2 */ + out_of_loop (); + pthread_exit (NULL); +} + +void * +itblock_function (void *arg) +{ + LOOP (ITBLOCK); /* break itblock */ + out_of_loop (); + pthread_exit (NULL); +} + +int +main (void) +{ + int res; + int i, j; + void *(*functions[2]) (void *); + + functions[0] = thumb2_function; + functions[1] = itblock_function; + + signal (SIGALRM, handler); + + for (i = 0; i < sizeof (functions); i++) + { + alarm (TIMEOUT); + + for (j = 0; j < NUM_THREADS; j++) + res = pthread_create (&child_thread[j], NULL, functions[i], NULL); + + for (j = 0; j < NUM_THREADS; j++) + res = pthread_join (child_thread[j], NULL); + + run = 1; + } + + exit (EXIT_SUCCESS); +} 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 @@ -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" *-*-* + } + gdb_test "step" ".*out_of_loop.*" "stepping $inst" +}
It takes two PTRACE_POKETEXT calls to write a 32-bit thumb-2 breakpoint to a 2-byte aligned and non 4-byte aligned address, and other threads can observe the partially modified instruction in the memory between these two calls. This causes problems on single stepping multi-thread program in GDBserver and GDB. 32-bit thumb-2 breakpoint was invented for single stepping 32-bit thumb-2 instructions in an IT block see: http://lists.infradead.org/pipermail/linux-arm-kernel/2010-January/007488.html but we can use the 16-bit thumb breakpoint instruction anywhere else. That is what this patch does. The change in set_breakpoint_type_at is similar to breakpoint.c:breakpoint_kind. Note also that this patch changes the breakpoint kinds used both in GDB and GDBServer not only to fix the multi-threading issue but also in order to keep them in sync. Otherwise if GDBServer needed to install a software single step on a GDB breakpoint the breakpoint kinds would not match and the breakpoint would be removed. This patch fixes fails in gdb.threads/schedlock.exp in thumb mode. Even with this patch, 32-bit thumb-2 breakpoint is still used for 32-bit thumb-2 instructions in IT block, so the problem is still there. This patch is a partial fix to PR server/21169. This is marked as a known fail in the new test called arm-single-step.exp. The fixed portion of this problem is also tested by that test and without the corresponding fix, it fails quickly consistently very quickly. No regressions, tested on ubuntu 14.04 ARMv7 and x86. With gdb, gdbserver-{native,extended} / { -marm -mthumb } 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 --- gdb/arch/arm-get-next-pcs.c | 206 +++++++++++++-------- gdb/arch/arm-get-next-pcs.h | 32 +++- gdb/arm-tdep.c | 254 +++++++++++++++++++++++++- gdb/gdbserver/linux-aarch32-low.c | 53 +++++- gdb/gdbserver/linux-aarch32-low.h | 3 + gdb/gdbserver/linux-arm-low.c | 21 --- gdb/gdbserver/mem-break.c | 11 +- gdb/testsuite/gdb.threads/arm-single-step.c | 106 +++++++++++ gdb/testsuite/gdb.threads/arm-single-step.exp | 53 ++++++ 9 files changed, 624 insertions(+), 115 deletions(-) create mode 100644 gdb/testsuite/gdb.threads/arm-single-step.c create mode 100644 gdb/testsuite/gdb.threads/arm-single-step.exp