This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 06/10] Replace breakpoint_reinsert_addr by get_next_pcs operation in GDBServer.
- From: Antoine Tremblay <antoine dot tremblay at ericsson dot com>
- To: Pedro Alves <palves at redhat dot com>, <gdb-patches at sourceware dot org>
- Date: Thu, 5 Nov 2015 09:34:57 -0500
- Subject: Re: [PATCH 06/10] Replace breakpoint_reinsert_addr by get_next_pcs operation in GDBServer.
- Authentication-results: sourceware.org; auth=none
- References: <1446138583-13268-1-git-send-email-antoine dot tremblay at ericsson dot com> <1446138583-13268-7-git-send-email-antoine dot tremblay at ericsson dot com> <563A52F9 dot 9040108 at redhat 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