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

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

Yao Qi writes:

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

Good point this does not look correct indeed, a call to
breakpoint_from_pc would be better at this point.

> IMO, this function should only tell whether PC is in IT block
> nor not.  It shouldn't involve any breakpoint kinds selection.
>

Yes I think you're right, I could make it return std::pair<CORE_ADDR,
bool (is_it_block ())>

And use breakpoint_from_pc if true , and BP_KIND_THUMB if false.

>> +      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?

No breakpoints are added to the vector, so it falls through.

>
>> +	}
>> +      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?
>

In the case of if (itstate != 0 && ((itstate >> 4) & 1) == cond_negated)
we are in the IT block

But if that is false we're after the IT block so it doesn't need a
thumb2 breakpoint. (See the comment above in the code)

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

You're right, that's a problem however I think it could be fixed in
arm_breakpoint_kind_from_current_state by adding a case where
placed_address != current_pc in which case the thumb bit would be the
deciding factor rather then arm_is_thumb_mode ()...

I'll resubmit a proper patch with those fixes.