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

[Antoine Tremblay <antoine dot tremblay at ericsson dot com>]

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