Re: [PATCH 1/2] gdbarch software_single_step returns VEC (CORE_ADDR) *

[Pedro Alves <palves@redhat.com>]

On 03/23/2016 02:10 PM, Yao Qi wrote:

 
> However, breakpoint insertion in arm is a little different, which
> uses arm_insert_single_step_breakpoint, and it updates a global
> variable arm_override_mode, so that arm_pc_is_thumb can get the
> right arm thumb mode even the program is wrong (see
> gdb.arch/thumb-singlestep.exp).  I failed to remove global variable
> arm_override_mode, so have to add a new gdbarch method
> insert_single_step_breakpoint, which is in default
> insert_single_step_breakpoint for all targets except arm.
> 

I think the problem is the ambiguity in arm_breakpoint_from_pc,
and to the fact that we don't "normalize" breakpoint addresses
before passing them down to target_insert_breakpoint. 

Some callers start with an address coming from the user and want
to consult symbol tables / mapping symbols.  Other caller really
want to trust the thumb bit as set in the address.

Note that arm_breakpoint_from_pc uses arm_pc_is_thumb, which is
what consults symbol tables / mapping symbols.

I think the fix would to make arm_breakpoint_from_pc always trust
that the address bit is already encoded correctly, and trust
IS_THUMB_ADDR, similarly to how the gdbserver version does, in
arm_breakpoint_kind_from_pc.

Then, we'd still need to consult the mapping symbols
consultation, or IOW, do something based on arm_pc_is_thumb _before_
target_insert_breakpoint is reached.  That is, call something like
arm_pc_is_thumb and use the result to encode the thumb bit correctly in
the address passed to target_insert_breakpoint.  IOW, "normalize" the
target address, using some gdbarch method, _before_ that address is passed
to the target routines in the first place.

Along the way, several other functions would stop using arm_pc_is_thumb,
but use IS_THUMB_ADDR directly.  E.g., arm_remote_breakpoint_from_pc.

WDYT?


> -# A return value of 1 means that the software_single_step breakpoints
> -# were inserted; 0 means they were not.
> -F:int:software_single_step:struct frame_info *frame:frame
> +# Return a vector of addresses on which the software single step
> +# breakpoints are inserted.  NULL means software single step is not used.

s/are inserted/should be inserted/

> +F:VEC (CORE_ADDR) *:software_single_step:struct frame_info *frame:frame
> +
> +m:void:insert_single_step_breakpoint:struct address_space *aspace, CORE_ADDR pc:aspace, pc::insert_single_step_breakpoint::0
>   
>   # Return non-zero if the processor is executing a delay slot and a
>   # further single-step is needed before the instruction finishes.
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 696105d..5dbcf7a 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -2248,11 +2248,28 @@ maybe_software_singlestep (struct gdbarch *gdbarch, CORE_ADDR pc)
>     int hw_step = 1;
>   
>     if (execution_direction == EXEC_FORWARD
> -      && gdbarch_software_single_step_p (gdbarch)
> -      && gdbarch_software_single_step (gdbarch, get_current_frame ()))
> +      && gdbarch_software_single_step_p (gdbarch))
>       {
> -      hw_step = 0;
> +      struct frame_info *frame = get_current_frame ();
> +      VEC (CORE_ADDR) * next_pcs;
> +
> +      next_pcs = gdbarch_software_single_step (gdbarch, frame);
> +
> +      if (next_pcs != NULL)
> +	{
> +	  int i;
> +	  CORE_ADDR pc;
> +	  struct address_space *aspace = get_frame_address_space (frame);
> +
> +	  hw_step = 0;
> +
> +	  for (i = 0; VEC_iterate (CORE_ADDR, next_pcs, i, pc); i++)
> +	    gdbarch_insert_single_step_breakpoint (gdbarch, aspace, pc);
> +
> +	  VEC_free (CORE_ADDR, next_pcs);
> +	}

This pattern of starting from a VEC of addresses, and a frame and
calling gdbarch_insert_single_step_breakpoint on each address
appears multiple times.  Can we put it in a convenience function?

Though the other calls in record-full.c didn't go through
gdbarch_insert_single_step_breakpoint -- why's that?

Otherwise, this is fine with me.   

Thanks,
Pedro Alves