Re: [PATCH 1/2] Call target_can_do_single_step from maybe_software_singlestep

[Andrew Burgess <aburgess@redhat.com>]

Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

> When the PikeOS osabi sniffer was added, Pedro suggested that a target
> could omit stepping from its vCont? reply packet to tell gdb that
> software single-step must be used:
>
> https://sourceware.org/legacy-ml/gdb-patches/2018-09/msg00312.html
>
> This patch implements this idea by moving the call to
> target_can_do_single_step into maybe_software_singlestep.

So this changes the behaviour for record-full.c, which includes two
calls to insert_single_step_breakpoints, which bypasses
maybe_software_singlestep.  If I'm reading the code correctly, after
this change, we'll switch from using h/w stepping to s/w stepping.

I'm assuming this isn't an intentional change as it's not mentioned.

I'm tempted to suggest that the two calls to
insert_single_step_breakpoints in record-full.c should really be calling
maybe_software_singlestep; I don't believe we can be recording in a
backward direction -- that makes no sense, we record forward, and use
the record to move backward.  And other than a duplicate call to
gdbarch_software_single_step_p, calling maybe_software_singlestep seems
otherwise harmless .... and after this patch we'd pick up the
target_can_do_single_step call, which is probably what we want.

Of course, if we did change record-full.c to use
maybe_software_singlestep then there would only be one call to
insert_single_step_breakpoints, so we could possibly fold this into
maybe_software_singlestep?

Thanks,
Andrew



>
> I've also removed some FIXME comments from gdbarch_components.py, and
> slightly updated the documentation for gdbarch_software_single_step.
> I think these comments are somewhat obsolete now that
> target_can_do_single_step exists -- the current approach isn't exactly
> what the comments intended, but on the other hand, it exists and
> works.
> ---
>  gdb/arm-linux-tdep.c      |  5 -----
>  gdb/gdbarch-gen.h         | 12 +++---------
>  gdb/gdbarch_components.py | 12 +++---------
>  gdb/infrun.c              | 12 +++++++++---
>  4 files changed, 15 insertions(+), 26 deletions(-)
>
> diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
> index dfa816990ff..f4da996728b 100644
> --- a/gdb/arm-linux-tdep.c
> +++ b/gdb/arm-linux-tdep.c
> @@ -923,11 +923,6 @@ arm_linux_software_single_step (struct regcache *regcache)
>    struct gdbarch *gdbarch = regcache->arch ();
>    struct arm_get_next_pcs next_pcs_ctx;
>  
> -  /* If the target does have hardware single step, GDB doesn't have
> -     to bother software single step.  */
> -  if (target_can_do_single_step () == 1)
> -    return {};
> -
>    arm_get_next_pcs_ctor (&next_pcs_ctx,
>  			 &arm_linux_get_next_pcs_ops,
>  			 gdbarch_byte_order (gdbarch),
> diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
> index 101b1b73636..1d86879f00a 100644
> --- a/gdb/gdbarch-gen.h
> +++ b/gdb/gdbarch-gen.h
> @@ -721,16 +721,10 @@ extern void set_gdbarch_get_memtag (struct gdbarch *gdbarch, gdbarch_get_memtag_
>  extern CORE_ADDR gdbarch_memtag_granule_size (struct gdbarch *gdbarch);
>  extern void set_gdbarch_memtag_granule_size (struct gdbarch *gdbarch, CORE_ADDR memtag_granule_size);
>  
> -/* FIXME/cagney/2001-01-18: This should be split in two.  A target method that
> -   indicates if the target needs software single step.  An ISA method to
> -   implement it.
> +/* Return a vector of addresses at which the software single step
> +   breakpoints should be inserted.  An empty vector means software single
> +   step is not used.
>  
> -   FIXME/cagney/2001-01-18: The logic is backwards.  It should be asking if the
> -   target can single step.  If not, then implement single step using breakpoints.
> -
> -   Return a vector of addresses on which the software single step
> -   breakpoints should be inserted.  NULL means software single step is
> -   not used.
>     Multiple breakpoints may be inserted for some instructions such as
>     conditional branch.  However, each implementation must always evaluate
>     the condition and only put the breakpoint at the branch destination if
> diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
> index 23e5789327c..7f2380ae0a7 100644
> --- a/gdb/gdbarch_components.py
> +++ b/gdb/gdbarch_components.py
> @@ -1301,16 +1301,10 @@ For a non-zero value, this represents the number of bytes of memory per tag.
>  
>  Function(
>      comment="""
> -FIXME/cagney/2001-01-18: This should be split in two.  A target method that
> -indicates if the target needs software single step.  An ISA method to
> -implement it.
> +Return a vector of addresses at which the software single step
> +breakpoints should be inserted.  An empty vector means software single
> +step is not used.
>  
> -FIXME/cagney/2001-01-18: The logic is backwards.  It should be asking if the
> -target can single step.  If not, then implement single step using breakpoints.
> -
> -Return a vector of addresses on which the software single step
> -breakpoints should be inserted.  NULL means software single step is
> -not used.
>  Multiple breakpoints may be inserted for some instructions such as
>  conditional branch.  However, each implementation must always evaluate
>  the condition and only put the breakpoint at the branch destination if
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 58da1cef29e..d1dce33bd3a 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -2242,9 +2242,15 @@ maybe_software_singlestep (struct gdbarch *gdbarch)
>  {
>    bool hw_step = true;
>  
> -  if (execution_direction == EXEC_FORWARD
> -      && gdbarch_software_single_step_p (gdbarch))
> -    hw_step = !insert_single_step_breakpoints (gdbarch);
> +  if (execution_direction == EXEC_FORWARD)
> +    {
> +      if (target_can_do_single_step () == 1)
> +	{
> +	  /* The target definitely has hardware single step.  */
> +	}
> +      else if (gdbarch_software_single_step_p (gdbarch))
> +	hw_step = !insert_single_step_breakpoints (gdbarch);
> +    }
>  
>    return hw_step;
>  }
>
> -- 
> 2.40.1