This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v6 5/7] Support software single step on ARM in GDBServer.



On 12/07/2015 10:17 AM, Yao Qi wrote:

Antoine Tremblay <antoine.tremblay@ericsson.com> writes:


gdb/ChangeLog:
	* Makefile.in: Add arm-get-next-pcs.c/o, arm-linux.c/o.
	to all arm targets.


Please describe how Makefile.in is changed, as I suggested in previous patch.


OK.


	* aarch32-linux-nat.c: Include arch/arm-get-next-pcs.h.
	* arch/arm-get-next-pcs.c: New file.
	* arch/arm-get-next-pcs.h: New file.
	* arch/arm-linux.h: New file.
	* arch/arm-linux.c: New file.
	* arm-linux-nat.c: Include arch/arm.h, arch/arm-get-next-pcs.h
	arch/arm-linux.h.
	* arm-linux-tdep.c: Include arch/arm.h,arch/arm-get-next-pcs.h
	arch/arm-linux.h.
	(arm_linux_get_next_pcs_ops): New struct.
	(ARM_SIGCONTEXT_R0, ARM_UCONTEXT_SIGCONTEXT,
	ARM_OLD_RT_SIGFRAME_SIGINFO, ARM_OLD_RT_SIGFRAME_UCONTEXT,
	ARM_NEW_RT_SIGFRAME_UCONTEXT, ARM_NEW_SIGFRAME_MAGIC): Move stack
	layout defines to arch/arm-linux.h.
	(arm_linux_sigreturn_next_pc_offset): Move to arch/arm-linux.c.
	(arm_linux_software_single_step): Adjust for
	arm_get_next_pcs implementation.
	* arm-tdep.c: Include arch/arm-get-next-pcs.h.
	(arm_get_next_pcs_ops): New struct.
	(submask): Move macro to arm-get-next-pcs.h.
	(bit): Likewise.
	(bits): Likewise.
	(sbits): Likewise.
	(BranchDest): Likewise.



	(thumb_instruction_changes_pc): Move to arm-get-next-pcs.c
	(thumb2_instruction_changes_pc): Likewise.
	(arm_instruction_changes_pc): Likewise.


I feel they should be moved to arch/arm.c, because they are not about
getting the next pcs.
OK. I went at first with the idea that if they were only used in get_next_pc to leave them with it as the criteria since I was not sure where to draw the line. But these do feel more generic. 


	(shifted_reg_val): Likewise.
	(thumb_advance_itstate): Likewise.


and this one.


OK.


	(thumb_get_next_pc_raw): Likewise.
	(arm_get_next_pc_raw): Likewise.
	(arm_get_next_pc): Likewise.
	(thumb_deal_with_atomic_sequence_raw): Likewise.
	(arm_deal_with_atomic_sequence_raw): Likewise.
	(arm_deal_with_atomic_sequence): Likewise.
	(arm_get_next_pcs_read_memory_unsigned_integer): New function.
	(arm_get_next_pcs_addr_bits_remove): Likewise.
	(arm_get_next_pcs_syscall_next_pc): Likewise.
	(arm_get_next_pcs_is_thumb): Likewise.
	(arm_software_single_step): Adjust for arm_get_next_pcs
	implementation.
	* arm-tdep.h: (arm_get_next_pc): Remove declaration.
	(arm_get_next_pcs_read_memory_unsigned_integer):
	New declaration.
	(arm_get_next_pcs_addr_bits_remove): Likewise.
	(arm_get_next_pcs_syscall_next_pc): Likewise.
	(arm_software_single_step): Likewise.
	(arm_deal_with_atomic_sequence: Remove declaration.
	(arm_get_next_pcs_is_thumb): New delcaration.
	* arm-wince-tdep.c: Include arch/arm-get-next-pcs.h.
	* armbsd-tdep.c: Likewise.
	* armnbsd-tdep.c: Likewise.
	* armobsd-tdep.c: Likewise.


Why do we need to include arm-get-next-pcs.h in these *-tdep.c files?
We don't now, some defines were needed before, I forgot to clean that up sorry. 


	* common/gdb_vecs.h: Add CORE_ADDR vector definition.
	* configure.tgt: Add arm-get-next-pcs.o, arm-linux.o to all
	relevent arm targets.
	* symtab.h: Move CORE_ADDR vector definition to gdb_vecs.h.

gdb/gdbserver/ChangeLog:
	* Makefile.in : Add arm-get-next-pcs.c/o, arm-linux.c/o.


Please describe what are changed.



OK.


	* configure.srv: Add arm-get-next-pcs.o, arm-linux.o
	to needed arm targets.
	* linux-arm-low.c: Include arch/arm-linux.h,
	aarch/arm-get-next-pcs.h.
	(get_next_pcs_ops): New struct.
	(get_next_pcs_addr_bits_remove): New function.
	(get_next_pcs_is_thumb): New function.
	(get_next_pcs_read_memory_unsigned_integer): Likewise.
	(arm_sigreturn_next_pc): Likewise.
	(get_next_pcs_syscall_next_pc): Likewise.
	(arm_gdbserver_get_next_pcs): Likewise.
	(struct linux_target_ops) <arm_gdbserver_get_next_pcs>:
	Initialize.
	* linux-low.h: Move CORE_ADDR vector definition to gdb_vecs.h.
	* server.h: Include gdb_vecs.h.
---
  gdb/Makefile.in               |   16 +-
  gdb/aarch32-linux-nat.c       |    1 +
  gdb/arch/arm-get-next-pcs.c   | 1216 +++++++++++++++++++++++++++++++++
  gdb/arch/arm-get-next-pcs.h   |   95 +++
  gdb/arch/arm-linux.c          |   59 ++
  gdb/arch/arm-linux.h          |   73 ++
  gdb/arm-linux-nat.c           |    2 +
  gdb/arm-linux-tdep.c          |  136 ++--
  gdb/arm-tdep.c                | 1495 +++++------------------------------------
  gdb/arm-tdep.h                |   18 +-
  gdb/arm-wince-tdep.c          |    1 +
  gdb/armbsd-tdep.c             |    1 +
  gdb/armnbsd-tdep.c            |    1 +
  gdb/armobsd-tdep.c            |    1 +
  gdb/common/gdb_vecs.h         |    2 +
  gdb/configure.tgt             |   20 +-
  gdb/gdbserver/Makefile.in     |    9 +-
  gdb/gdbserver/configure.srv   |    2 +
  gdb/gdbserver/linux-arm-low.c |  144 +++-
  gdb/gdbserver/linux-low.h     |    2 -
  gdb/gdbserver/server.h        |    1 +
  gdb/symtab.h                  |    2 -
  22 files changed, 1871 insertions(+), 1426 deletions(-)
  create mode 100644 gdb/arch/arm-get-next-pcs.c
  create mode 100644 gdb/arch/arm-get-next-pcs.h
  create mode 100644 gdb/arch/arm-linux.c
  create mode 100644 gdb/arch/arm-linux.h






+
+/* Checks for an atomic sequence of instructions beginning with a LDREX{,B,H,D}
+   instruction and ending with a STREX{,B,H,D} instruction.  If such a sequence
+   is found, attempt to step through it.  The end of the sequence address is
+   added to the next_pcs list.  */
+
+static int
+thumb_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self,
+				     CORE_ADDR pc,
+				     VEC (CORE_ADDR) **next_pcs)


We can return VEC (CORE_ADDR) * here, if it is NULL, the sequence is not
found, otherwise, the end of the sequence addresses are returned in the vector.



OK.


+
+/* Checks for an atomic sequence of instructions beginning with a LDREX{,B,H,D}
+   instruction and ending with a STREX{,B,H,D} instruction.  If such a sequence
+   is found, attempt to step through it.  The end of the sequence address is
+   added to the next_pcs list.  */
+
+static int
+arm_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self,
+				   CORE_ADDR pc,
+				   VEC (CORE_ADDR) **next_pcs)


Likewise.


+
+/* See arm-get-next-pcs.h.  */
+
+VEC (CORE_ADDR) *
+arm_get_next_pcs (struct arm_get_next_pcs *self, CORE_ADDR pc)
+{
+  VEC (CORE_ADDR) *next_pcs = NULL;
+
+  if (self->ops->is_thumb (self))
+    {
+      if (!thumb_deal_with_atomic_sequence_raw (self, pc, &next_pcs))
+	return thumb_get_next_pcs_raw (self, pc, &next_pcs);


Why do we need to pass next_pcs to thumb_get_next_pcs_raw?  Isn't
simpler to write it like this,

         next_pcs = thumb_deal_with_atomic_sequence_raw (self, pc);
         if (next_pcs == NULL)
           next_pcs = thumb_get_next_pcs_raw (self, pc);
Indeed, I was focused on not losing the VEC allocation/resize and forgot thumb_deal_with_atomic_sequence_raw and thumb_get_next_pcs_raw will not combine their vectors. 

Thanks.


+    }
+  else
+    {
+      if (!arm_deal_with_atomic_sequence_raw (self, pc, &next_pcs))
+	return arm_get_next_pcs_raw (self, pc, &next_pcs);
+    }
+
+  return next_pcs;
+}



diff --git a/gdb/arch/arm-get-next-pcs.h b/gdb/arch/arm-get-next-pcs.h
new file mode 100644
index 0000000..39921e0
--- /dev/null
+++ b/gdb/arch/arm-get-next-pcs.h
@@ -0,0 +1,95 @@
+/* Common code for ARM software single stepping support.
+
+   Copyright (C) 1988-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 ARM_GET_NEXT_PCS_H
+#define ARM_GET_NEXT_PCS_H 1
+
+/* Support routines for instruction parsing.  */
+#define submask(x) ((1L << ((x) + 1)) - 1)
+#define bits(obj,st,fn) (((obj) >> (st)) & submask ((fn) - (st)))
+#define bit(obj,st) (((obj) >> (st)) & 1)
+#define sbits(obj,st,fn) \
+  ((long) (bits(obj,st,fn) | ((long) bit(obj,fn) * ~ submask (fn - st))))
+#define BranchDest(addr,instr) \
+  ((CORE_ADDR) (((unsigned long) (addr)) + 8 + (sbits (instr, 0, 23) << 2)))
+


These macros should be moved to arch/arm.h.



OK.
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]