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

[Antoine Tremblay <antoine.tremblay@ericsson.com>]

Yao Qi writes:

> On 17-02-17 19:17:56, Antoine Tremblay wrote:
>> > In ARM ARM, we have the pseudo code,
>> >
>> > boolean InITBlock()
>> > return (ITSTATE.IT<3:0> != ‘0000’);
>> >
>> > ITSTATE can be got from CPSR.
>>
>> Yes that's good if you're inserting a breakpoint at current PC but
>> otherwise you will need something else...
>
> 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.

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.

Now I find myself trying to replicate the logic in thumb_get_next_pcs_raw
for IT blocks in arm_breakpoint_kind_from_current_state and it doesn't
feel right.

It gives something like:

void
set_single_step_breakpoint (CORE_ADDR stop_at, ptid_t ptid)
{
  struct single_step_breakpoint *bp;

  gdb_assert (ptid_get_pid (current_ptid) == ptid_get_pid (ptid));

  CORE_ADDR placed_address = stop_at;
  int breakpoint_kind
    = target_breakpoint_kind_from_current_state (&placed_address);

  bp = (struct single_step_breakpoint *) set_breakpoint_type_at_with_kind
    (single_step_breakpoint, placed_address, NULL, breakpoint_kind);
  bp->ptid = ptid;
}

int
arm_breakpoint_kind_from_current_state (CORE_ADDR *pcptr)
{
  if (arm_is_thumb_mode ())
    {

    if ( current_pcs_or_expected_next_pcs_are_in_it_block  ??? /* That's
    not right... */

	{
	  *pcptr = UNMAKE_THUMB_ADDR (*pcptr);
	  return ARM_BP_KIND_THUMB;
	}
      else
	{
	  *pcptr = MAKE_THUMB_ADDR (*pcptr);
	  return arm_breakpoint_kind_from_pc (pcptr);
	}
    }
  else
    {
      return arm_breakpoint_kind_from_pc (pcptr);
    }
}

It would be much better if get_next_pcs could select the breakpoint kind
itself and hint that to set_single_step_breakpoint like :

 VEC (struct { next_pc, breakpoint_kind })  = (*the_low_target.get_next_pcs) (regcache);

  for (i = 0; VEC_iterate (struct  { CORE_ADDR, kind }, next_pcs, i, pc); ++i)
    set_single_step_breakpoint (pc, kind, current_ptid);

But of course that means changing that virtual function for all archs
etc... :(

I'm thinking of going with that approch but I would like to know if you
have any other solutions to propose or if that sounds OK before writing
all that code ?

Thanks,
Antoine