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:

> +  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);
> +	}
> +      next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds>
> +			  (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2));

It is incorrect to choose ARM_BP_KIND_THUMB2 if the instruction is
16-bit.  IMO, this function should only tell whether PC is in IT block
nor not.  It shouldn't involve any breakpoint kinds selection.

> +      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);
> +	    }
> +

If all the following instructions' condition is false, breakpoint should
be set on the first instruction out side of IT block.  We can still use
16-bit thumb breakpoint.

> +	  next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds>
> +			      (MAKE_THUMB_ADDR (pc),
> ARM_BP_KIND_THUMB2));

The same issue.

> +	  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.  */

How do we fall through now?

> +	}
> +      else
> +	{
> +	  int cond_negated;
> +
> +	  /* There are conditional instructions after this one.
> +	     If this instruction modifies the flags, then we can
> +	     not predict what the next executed instruction will
> +	     be.  Fortunately, this instruction is archi2tecturally
> +	     forbidden to branch; we know it will fall through.
> +	     Start by skipping past it.  */
> +	  pc += thumb_insn_size (inst1);
> +	  itstate = thumb_advance_itstate (itstate);
> +
> +	  /* Set a breakpoint on the following instruction.  */
> +	  gdb_assert ((itstate & 0x0f) != 0);
> +	  next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds>
> +			      (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2));
> +
> +	  cond_negated = (itstate >> 4) & 1;
> +
> +	  /* Skip all following instructions with the same
> +	     condition.  If there is a later instruction in the IT
> +	     block with the opposite condition, set the other
> +	     breakpoint there.  If not, then set a breakpoint on
> +	     the instruction after the IT block.  */
> +	  do
> +	    {
> +	      inst1 = read_mem_uint (pc, 2, byte_order_for_code);
> +	      pc += thumb_insn_size (inst1);
> +	      itstate = thumb_advance_itstate (itstate);
> +	    }
> +	  while (itstate != 0 && ((itstate >> 4) & 1) == cond_negated);
> +
> +	  if (itstate != 0 && ((itstate >> 4) & 1) == cond_negated)
> +	    {
> +	      next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds>
> +				  (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2));
> +	    }
> +	  else
> +	    {
> +	      next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds>
> +				  (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB));
> +	    }

Why do you choose breakpoint in this way?

> 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);

I read my patch again, but it looks wrong.  If we single-step an
instruction with a state change, like bx or blx, current get_next_pcs
correctly marked the address bit.  However, with the change like this,
we'll get the wrong breakpoint kind.

-- 
Yao (齐尧)