From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 91892 invoked by alias); 4 Nov 2015 18:48:31 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 91882 invoked by uid 89); 4 Nov 2015 18:48:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 04 Nov 2015 18:48:28 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id B8265935D0; Wed, 4 Nov 2015 18:48:27 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id tA4ImQpx018087; Wed, 4 Nov 2015 13:48:26 -0500 Message-ID: <563A52F9.9040108@redhat.com> Date: Wed, 04 Nov 2015 18:48:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Antoine Tremblay , gdb-patches@sourceware.org Subject: Re: [PATCH 06/10] Replace breakpoint_reinsert_addr by get_next_pcs operation in GDBServer. References: <1446138583-13268-1-git-send-email-antoine.tremblay@ericsson.com> <1446138583-13268-7-git-send-email-antoine.tremblay@ericsson.com> In-Reply-To: <1446138583-13268-7-git-send-email-antoine.tremblay@ericsson.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-11/txt/msg00154.txt.bz2 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 . */ > + > +#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 > #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