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 1/2] This patch fixes GDBServer's run control for single stepping


Yao Qi writes:

> 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.
>

You have to add if the current instruction is an IT instruction in wich
case the next instruction will be in an IT block.

Also if you have a conditional instruction that would evalutate to
true and is not the last one, get_next_pcs may return an instruction
after the IT block, arm_breakpoint_kind_from_current_state will be
called from the IT block with that PC and return a THUMB2_KIND when it
should not. See the else case in arm-get-next-pcs.c:~351

My point was that we should use get_next_pc directly since it's the best
place to detect if the next_pc is in the IT block. And the intent would
be clear.

It would give something like the patch below. (Note the GDB part of this
is missing but it works with GDBServer)

> 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.

Humm but how will the GDBServer 16-bit breakpoint be reported to GDB ?
Won't it always be hit and handled by GDBServer ?

And if you have a GDB breakpoint on an instruction and GDBServer puts
a single step breakpoint on that GDB breakpoint instruction, GDBServer
still knows of the GDB and GDBServer breakpoint types.

So how does GDB get confused ?

----
commit a18806af57b6c8906594ded036009c6b30c5b7db
Author: Antoine Tremblay <antoine.tremblay@ericsson.com>
Date:   Thu Mar 30 12:57:29 2017 -0400

    getnextpc encodes the kind

diff --git a/gdb/arch/arm-get-next-pcs.c b/gdb/arch/arm-get-next-pcs.c
index 398dd59a..f9b5bf1 100644
--- a/gdb/arch/arm-get-next-pcs.c
+++ b/gdb/arch/arm-get-next-pcs.c
@@ -315,6 +315,7 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
 	      itstate = thumb_advance_itstate (itstate);
 	    }
 
+	  pc = MAKE_THUMB2_BP_KIND (pc);
 	  VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc));
 	  return next_pcs;
 	}
@@ -335,6 +336,7 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
 		  itstate = thumb_advance_itstate (itstate);
 		}
 
+	      pc = MAKE_THUMB2_BP_KIND (pc);
 	      VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc));
 	      return next_pcs;
 	    }
@@ -361,8 +363,13 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
 
 	      /* Set a breakpoint on the following instruction.  */
 	      gdb_assert ((itstate & 0x0f) != 0);
+	      pc = MAKE_THUMB2_BP_KIND (pc);
 	      VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc));
 
+	      /* Reset the thumb2 kind bit to avoid affecting the next pc
+		 value.  */
+	      pc = UNMAKE_THUMB2_BP_KIND (pc);
+
 	      cond_negated = (itstate >> 4) & 1;
 
 	      /* Skip all following instructions with the same
@@ -378,6 +385,11 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self)
 		}
 	      while (itstate != 0 && ((itstate >> 4) & 1) == cond_negated);
 
+	      /* Only set a thumb2 breakpoint kind if we are still in an
+		 IT block.  */
+	      if (itstate != 0 && ((itstate >> 4) & 1) == cond_negated)
+		pc = MAKE_THUMB2_BP_KIND (pc);
+
 	      VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc));
 
 	      return next_pcs;
diff --git a/gdb/arch/arm-linux.c b/gdb/arch/arm-linux.c
index 943efe7..4b20f77 100644
--- a/gdb/arch/arm-linux.c
+++ b/gdb/arch/arm-linux.c
@@ -73,7 +73,7 @@ arm_linux_get_next_pcs_fixup (struct arm_get_next_pcs *self,
      step this instruction, this instruction isn't executed yet, and LR
      may not be updated yet.  In other words, GDB can get the target
      address from LR if this instruction isn't BL or BLX.  */
-  if (nextpc > 0xffff0000)
+  if ((nextpc & 0xffffffff) > 0xffff0000)
     {
       int bl_blx_p = 0;
       CORE_ADDR pc = regcache_read_pc (self->regcache);
diff --git a/gdb/arch/arm.h b/gdb/arch/arm.h
index a86dd37..b83fc79 100644
--- a/gdb/arch/arm.h
+++ b/gdb/arch/arm.h
@@ -101,6 +101,9 @@ enum arm_breakpoint_kinds
 #define IS_THUMB_ADDR(addr)    ((addr) & 1)
 #define MAKE_THUMB_ADDR(addr)  ((addr) | 1)
 #define UNMAKE_THUMB_ADDR(addr) ((addr) & ~1)
+#define IS_THUMB2_BP_KIND(addr) (((addr) & 1ULL << 32) >> 32)
+#define MAKE_THUMB2_BP_KIND(addr) ((addr) | (1ULL << 32))
+#define UNMAKE_THUMB2_BP_KIND(addr) ((addr) & ~(1ULL << 32))
 
 /* Support routines for instruction parsing.  */
 #define submask(x) ((1L << ((x) + 1)) - 1)
diff --git a/gdb/gdbserver/linux-aarch32-low.c b/gdb/gdbserver/linux-aarch32-low.c
index 2b710ba..fc66028 100644
--- a/gdb/gdbserver/linux-aarch32-low.c
+++ b/gdb/gdbserver/linux-aarch32-low.c
@@ -242,10 +242,16 @@ arm_breakpoint_kind_from_pc (CORE_ADDR *pcptr)
 	  unsigned short inst1 = 0;
 
 	  target_read_memory (*pcptr, (gdb_byte *) &inst1, 2);
-	  if (thumb_insn_size (inst1) == 4)
-	    return ARM_BP_KIND_THUMB2;
+	  if (thumb_insn_size (inst1) == 4 && IS_THUMB2_BP_KIND (*pcptr))
+	    {
+	      *pcptr = UNMAKE_THUMB2_BP_KIND (*pcptr);
+	      return ARM_BP_KIND_THUMB2;
+	    }
+
+	  return ARM_BP_KIND_THUMB;
 	}
-      return ARM_BP_KIND_THUMB;
+      else
+	return ARM_BP_KIND_THUMB;
     }
   else
     return ARM_BP_KIND_ARM;


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