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

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

On 11/04/2015 01:48 PM, Pedro Alves wrote:
> 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.

Yes, fixed.

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

Yes I could do another patch with just that change since I really wanted 
it to be out of the big 08/10 Support software single step on ARM in 
GDBServer patch, but that would kinda remove some context and it would 
still be premature in the same way.

Same applies to 07/10 Share some ARM target dependant code from GDB with 
GDBServer...

I'm keeping it there in the context that this is a preparatory patch 
unless there's an objection.

> > 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.
Done as :

/* This structure contains the shared context of a get_next_pcs call, as
   used in linux target operation "get_next_pcs" in GDBServer and the
   software_single_step architecture dependent function in GDB.  */

> > +{
> > +  /* 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.

Done.

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

Done thanks,

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

No that was a mistake, fixed.

> > +  (*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.

Ok, noted.

Thanks,
Antoine Tremblay