Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping

[Yao Qi <qiyaoltc@gmail.com>]

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:

>> In software single step, we calculate the next pcs, and select
>> breakpoint kinds of them, according to current pc.  If current
>> pc is not within IT block (!InITBlock ()) or the last instruction
>> in IT block (LastInITBlock ()), we can safely use 16-bit thumb
>> breakpoint for any thumb instruction.  That is, in
>> gdbserver/linux-aarch32-low.c:arm_breakpoint_kind_from_current_state,
>> we can return ARM_BP_KIND_THUMB if (!InITBlock () || LastInITBlock ()).
>
> This is not entirely true since we have to check if the next pcs are in
> an IT block rather then only the current one, so there's multiple
> scenarios.

In fact, we need to know whether the next pcs will be conditionally
executed or not, right?  If they won't be conditionally executed, we can
use 16-bit thumb breakpoint instruction.  By checking CPSR, if
current PC is not in IT block or on the last instruction in IT block,
the next pcs can't be conditionally executed.  I've already had a patch
below to implement what I described.

The problem of this patch is that we end up inserting different
kinds of breakpoints on the same instruction.  For a given 32-bit thumb
instruction, GDB and GDBserver knows 32-bit thumb breakpoint instruction
is used for GDB breakpoint, but only GDBserver knows 16-bit thumb
breakpoint is used for GDBserver single-step breakpoint, so GDB will be
confused on this.  I stopped here, and start to do something else.

>
> Consider if current PC is the IT instruction for example, then there's
> at least 2 next pcs inside the IT block where we will need to install an THUMB2
> breakpoint and get_next_pcs will return that.

-- 
Yao (齐尧)

From fad6162c9365535a09ca1072f15469e6007c0fd2 Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Fri, 24 Feb 2017 09:25:43 +0000
Subject: [PATCH] Only use 32-bit thumb-2 breakpoint instruction on necessary

It takes two PTRACE_POKETEXT calls to write 32-bit thumb-2 breakpoint to
a 2-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.

32-bit thumb-2 breakpoint was invented for single step 32-bit thumb-2
instruction in IT block,
http://lists.infradead.org/pipermail/linux-arm-kernel/2010-January/007488.html
but we can use 16-bit thumb breakpoint instruction anywhere else.
That is what this patch does.  Change in set_breakpoint_type_at is similar
to breakpoint.c:breakpoint_kind.

This patch fixes fails in gdb.threads/schedlock.exp.

Even with 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 21169.

gdb/gdbserver:

2017-02-24  Yao Qi  <yao.qi@linaro.org>

	PR server/21169
	* linux-aarch32-low.c (arm_breakpoint_kind_from_current_state):
	Set kind to ARM_BP_KIND_THUMB if program is out of IT block or
	on the last instruction of IT block.
	* mem-break.c (set_breakpoint_type_at): Call
	target_breakpoint_kind_from_current_state to get breakpoint kind
	for single_step breakpoint.

diff --git a/gdb/gdbserver/linux-aarch32-low.c b/gdb/gdbserver/linux-aarch32-low.c
index 2b710ba..7409050 100644
--- a/gdb/gdbserver/linux-aarch32-low.c
+++ b/gdb/gdbserver/linux-aarch32-low.c
@@ -288,7 +288,30 @@ arm_breakpoint_kind_from_current_state (CORE_ADDR *pcptr)
   if (arm_is_thumb_mode ())
     {
       *pcptr = MAKE_THUMB_ADDR (*pcptr);
-      return arm_breakpoint_kind_from_pc (pcptr);
+      int kind = arm_breakpoint_kind_from_pc (pcptr);
+
+      if (kind == ARM_BP_KIND_THUMB2)
+	{
+	    unsigned long cpsr;
+	    struct regcache *regcache
+	      = get_thread_regcache (current_thread, 1);
+
+	    collect_register_by_name (regcache, "cpsr", &cpsr);
+	    /* Only use 32-bit thumb-2 breakpoint if *PCPTR is within
+	       IT block, because it takes two PTRACE_POKETEXT calls to
+	       write 32-bit thumb-2 breakpoint to a 2-byte aligned
+	       address, and other threads can observe the partially
+	       modified instruction in the memory between two
+	       PTRACE_POKETEXT calls.
+
+	       Don't use 32-bit thumb-2 breakpoint if program is not
+	       in IT block or on the last instruction of IT block,
+	       (ITSTATE.IT<2:0> == 000).  These bits are from CPSR bit
+	       10, 25, and 26.  */
+	    if ((cpsr & 0x06000400) == 0)
+	      kind = ARM_BP_KIND_THUMB;
+	}
+      return kind;
     }
   else
     {
diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c
index 6e6926a..f3845cf 100644
--- a/gdb/gdbserver/mem-break.c
+++ b/gdb/gdbserver/mem-break.c
@@ -855,7 +855,21 @@ 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,