This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Only use 32-bit thumb-2 breakpoint instruction if necessary


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 (齐尧)


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]