Re: [PATCH 06/10] Replace breakpoint_reinsert_addr by get_next_pcs operation in GDBServer.

[Pedro Alves <palves@redhat.com>]

On 10/29/2015 05:09 PM, Antoine Tremblay wrote:
> This patch in preparation for software single step support on ARM. It refactors
> breakpoint_reinsert_addr into get_next_pcs so that multiple location can be
> returned.
> 
> When software single stepping there can be multiple possible next addresses
> because we're stepping over a branch instruction for example.

ITYM mean a _conditional_ branch instruction.

> 
> The operation get_next_pcs handles that by returning a vector of all the
> possible next addresses.
> 
> Software breakpoints are installed at each location returned.
> 
> No regressions, tested on ubuntu 14.04 ARMv7 and x86.
> With gdbserver-{native,extended} / { -marm -mthumb }
> 
> gdb/ChangeLog:
> 
> 	* common/get-next-pcs.h: New file.
> 	* common/gdb_vecs.h: Add CORE_ADDR vect.
> 	* symtab.h: Move CORE_ADDR vect to gdb_vecs.h

Missing period in last line.  (Pedantically, AFAICS, in this patch,
nothing in gdb includes common/get-next-pcs.h, and nothing
in gdbserver includes symtab.h.  So seems like putting the VEC
in common/gdb_vecs.h here is premature.)

> 
> gdb/gdbserver/ChangeLog:
> 	* linux-aarch64-low.c (struct linux_target_ops): Rename
> 	breakpoint_reinsert_addr to get_next_pcs.
> 	* linux-arm-low.c (struct linux_target_ops): Likewise.
> 	* linux-bfin-low.c (struct linux_target_ops): Likewise.
> 	* linux-cris-low.c (struct linux_target_ops): Likewise.
> 	* linux-crisv32-low.c (struct linux_target_ops): Likewise.
> 	* linux-low.c (can_software_single_step): Likewise.
> 	(install_software_single_step_breakpoints): New function.
> 	(start_step_over): Use isntall_software_single_step_breakpoints.
> 	* linux-low.h: Rename breakpoint_reinsert_addr to
> 	get_next_pcs.
> 	* linux-mips-low.c (struct linux_target_ops): Likewise.
> 	* linux-nios2-low.c (struct linux_target_ops): Likewise.
> 	* linux-sparc-low.c (struct linux_target_ops): Likewise.


This LGTM.  Some nits below.

> +++ b/gdb/common/get-next-pcs.h
> @@ -0,0 +1,34 @@
> +/* Common code for software single stepping support.
> +
> +   Copyright (C) 2015 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef GET_NEXT_PCS_H
> +#define GET_NEXT_PCS_H 1
> +
> +#include "gdb_vecs.h"
> +
> +struct get_next_pcs

There should be an intro comment mentioning the purpose of
this structure.

> +{
> +  /* Resulting vector of possible next addresses.  */
> +  VEC (CORE_ADDR) *result;
> +  /* Base PC from which to get the next pcs.  */
> +  CORE_ADDR pc;
> +  struct regcache* regcache;

This field should be documented too.

> +};
> +
> +#endif /* GET_NEXT_PCS_H */
> diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c
> index a0bc1c2..3e21042 100644
> --- a/gdb/gdbserver/linux-aarch64-low.c
> +++ b/gdb/gdbserver/linux-aarch64-low.c
> @@ -2959,7 +2959,7 @@ struct linux_target_ops the_low_target =
>    aarch64_set_pc,
>    NULL, /* breakpoint_kind_from_pc */
>    aarch64_sw_breakpoint_from_kind,
> -  NULL, /* breakpoint_reinsert_addr */
> +  NULL, /* get_next_pcs */
>    0,    /* decr_pc_after_break */
>    aarch64_breakpoint_at,
>    aarch64_supports_z_point_type,
> diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
> index 8e32fae..0b87367 100644
> --- a/gdb/gdbserver/linux-arm-low.c
> +++ b/gdb/gdbserver/linux-arm-low.c
> @@ -1027,7 +1027,7 @@ struct linux_target_ops the_low_target = {
>    arm_set_pc,
>    arm_breakpoint_kind_from_pc,
>    arm_sw_breakpoint_from_kind,
> -  NULL, /* breakpoint_reinsert_addr */
> +  NULL, /* get_next_pcs */
>    0,
>    arm_breakpoint_at,
>    arm_supports_z_point_type,
> diff --git a/gdb/gdbserver/linux-bfin-low.c b/gdb/gdbserver/linux-bfin-low.c
> index 912d253..1de2f78 100644
> --- a/gdb/gdbserver/linux-bfin-low.c
> +++ b/gdb/gdbserver/linux-bfin-low.c
> @@ -141,7 +141,7 @@ struct linux_target_ops the_low_target = {
>    bfin_set_pc,
>    NULL, /* breakpoint_kind_from_pc */
>    bfin_sw_breakpoint_from_kind,
> -  NULL, /* breakpoint_reinsert_addr */
> +  NULL, /* get_next_pcs */
>    2,
>    bfin_breakpoint_at,
>    NULL, /* supports_z_point_type */
> diff --git a/gdb/gdbserver/linux-cris-low.c b/gdb/gdbserver/linux-cris-low.c
> index 9f4519c..6902a45 100644
> --- a/gdb/gdbserver/linux-cris-low.c
> +++ b/gdb/gdbserver/linux-cris-low.c
> @@ -139,7 +139,7 @@ struct linux_target_ops the_low_target = {
>    cris_set_pc,
>    NULL, /* breakpoint_kind_from_pc */
>    cris_sw_breakpoint_from_kind,
> -  NULL, /* breakpoint_reinsert_addr */
> +  NULL, /* get_next_pcs */
>    0,
>    cris_breakpoint_at,
>  };
> diff --git a/gdb/gdbserver/linux-crisv32-low.c b/gdb/gdbserver/linux-crisv32-low.c
> index 2404d0e..28c1981 100644
> --- a/gdb/gdbserver/linux-crisv32-low.c
> +++ b/gdb/gdbserver/linux-crisv32-low.c
> @@ -422,7 +422,7 @@ struct linux_target_ops the_low_target = {
>    cris_set_pc,
>    NULL, /* breakpoint_kind_from_pc */
>    cris_sw_breakpoint_from_kind,
> -  NULL, /* breakpoint_reinsert_addr */
> +  NULL, /* get_next_pcs */
>    0,
>    cris_breakpoint_at,
>    cris_supports_z_point_type,
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index a3b0f3a..1f423ba 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -54,6 +54,7 @@
>  #include <elf.h>
>  #endif
>  #include "nat/linux-namespaces.h"
> +#include "common/get-next-pcs.h"
>  
>  #ifndef SPUFS_MAGIC
>  #define SPUFS_MAGIC 0x23c9b64e
> @@ -281,12 +282,12 @@ can_hardware_single_step (void)
>  }
>  
>  /* True if the low target can software single-step.  Such targets
> -   implement the BREAKPOINT_REINSERT_ADDR callback.  */
> +   implement the GET_NEXT_PCS callback.  */
>  
>  static int
>  can_software_single_step (void)
>  {
> -  return (the_low_target.breakpoint_reinsert_addr != NULL);
> +  return (the_low_target.get_next_pcs != NULL);
>  }
>  
>  /* True if the low target supports memory breakpoints.  If so, we'll
> @@ -3876,6 +3877,29 @@ enqueue_pending_signal (struct lwp_info *lwp, int signal, siginfo_t *info)
>    lwp->pending_signals = p_sig;
>  }
>  
> +/* Install breakpoints for software single stepping.  */
> +
> +static void
> +install_software_single_step_breakpoints (struct lwp_info *lwp)
> +{
> +  struct get_next_pcs next_pcs;
> +  int i;
> +  CORE_ADDR pc;
> +
> +  struct regcache *regcache = get_thread_regcache (current_thread, 1);

Formatting.  Variable should be declared in the declaration block
along with the others, and then there should be a line break.

> +  next_pcs.pc = get_pc (lwp);
> +  next_pcs.regcache = regcache;
> +  next_pcs.result = VEC_alloc (CORE_ADDR, 1);

Is there a reason to allocate upfront instead of setting it to NULL?

> +  (*the_low_target.get_next_pcs) (&next_pcs);
> +
> +  for (i = 0; VEC_iterate (CORE_ADDR, next_pcs.result, i, pc); ++i)
> +    {
> +      set_reinsert_breakpoint (pc);
> +    }

At some point we should do a pass and rename "reinsert breakpoint"
to "software single-step breakpoint" throughout.  It's no longer
just about reinserting a breakpoint that we step over.

> +
> +  VEC_free (CORE_ADDR, next_pcs.result);
> +}
> +



Thanks,
Pedro Alves