[1/2] This patch fixes GDBServer's run control for single stepping
Commit Message
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
Comments
Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
> You have to add if the current instruction is an IT instruction in wich
> case the next instruction will be in an IT block.
>
Oh, you are right.
> 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
With the current PC and CPSR, it is not difficult to know whether
next_pc is still within IT block nor not, because all instructions in IT
block should be sequentially executed or skipped.
>
> 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.
Yeah, we can record the information of breakpoint type in the return
value of get_next_pc, ...
>
> It would give something like the patch below. (Note the GDB part of this
> is missing but it works with GDBServer)
>
... but using extra bit in CORE_ADDR is not a good idea to me.
>> 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 ?
That was my conclusion at that point. I got some regressions in
gdb.threads/*.exp when I tested my patch (gdb running is on
x86_64-linux), but I can't remember more details.
I am also wondering that we can use some code in
arm_adjust_breakpoint_address about detecting BPADDR is within IT block
or not by scanning instructions backward, if none of two bytes (can be
16-bit thumb instruction or the 2nd half of 32-bit thumb instruction)
matches IT instruction, the PC is not within IT block.
@@ -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;
@@ -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);
@@ -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)
@@ -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;