This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v4 4/6] Support software single step on ARM in GDBServer.
- From: Antoine Tremblay <antoine dot tremblay at ericsson dot com>
- To: Yao Qi <qiyaoltc at gmail dot com>
- Cc: <gdb-patches at sourceware dot org>
- Date: Thu, 3 Dec 2015 08:58:29 -0500
- Subject: Re: [PATCH v4 4/6] Support software single step on ARM in GDBServer.
- Authentication-results: sourceware.org; auth=none
- References: <1449062264-18565-1-git-send-email-antoine dot tremblay at ericsson dot com> <1449062264-18565-5-git-send-email-antoine dot tremblay at ericsson dot com> <86a8pru6hf dot fsf at gmail dot com>
On 12/03/2015 06:17 AM, Yao Qi wrote:
Antoine Tremblay <antoine.tremblay@ericsson.com> writes:
Some comments on the design,
+/* Context for a get_next_pcs call on ARM. */
+struct arm_get_next_pcs
+{
+ /* Operations implementations. */
+ struct arm_get_next_pcs_ops *ops;
+ /* Byte order for data. */
+ int byte_order;
+ /* Byte order for code. */
+ int byte_order_for_code;
+ /* Is the pc in thumb mode. */
+ int is_thumb;
+ /* Use 32bit or 26 bit pc. */
+ int arm_apcs_32;
+ /* Thumb2 breakpoint instruction. */
+ const gdb_byte *arm_thumb2_breakpoint;
These fields are GDB specific, GDBserver doesn't need them at all.
Can we move them to arm_gdb_get_next_pcs? Field is_thumb is used in
both sides, but can't we compute it in two sides (through arm_is_thumb
and arm_is_thumb_mode) respectively, rather than having a field here?
byte_order fields seemed like a good idea at first and I liked your
suggested change for read_memory_unsigned_integer.
However GDB is using 2 byte orders : byte_order (for data) and
byte_order_for_code to support BE8 endianness.
This complicates things a bit since in common code I can't call:
self->ops->read_memory_unsigned_integer (self, loc, 2)
I would have no way to specify if it should read with byte_order or with
byte_order_for_code.
So unfortunately these need to stay in the common struct.
is_thumb: OK it will add an operation in arm_get_next_pcs_ops but that's
fine.
arm_apcs_32: I can move the apcs32 check on GDB's side indeed.
const gdb_byte *arm_thumb2_breakpoint: This one needs to stay, since
while on GDB's side it could be computed through regcache/gdbarch, on
GDBServer's side it's directly a variable.
Still removing arm_apcs_32 and is_thumbs simplifies things thanks !
+ struct regcache *regcache;
+};
+
+/* get_next_pcs operations. */
+struct arm_get_next_pcs_ops
+{
+ ULONGEST (*read_memory_unsigned_integer) (CORE_ADDR memaddr,
+ int len,
+ int byte_order);
We need argument struct arm_get_next_pcs *self, and get rid of argument
byte_order, which can be got through self.
See above answer about byte_order fields.
+ CORE_ADDR (*syscall_next_pc) (struct arm_get_next_pcs *self, CORE_ADDR pc);
+ CORE_ADDR (*addr_bits_remove) (struct arm_get_next_pcs *self, CORE_ADDR val);
+};
+/* Context for a get_next_pcs call on ARM in GDB. */
+struct arm_gdb_get_next_pcs
+{
+ /* Common context for gdb/gdbserver. */
+ struct arm_get_next_pcs base;
+ /* Frame information. */
+ struct frame_info *frame;
FRAME is still used in arm_get_next_pcs_syscall_next_pc, but we should
use regcache instead of frame there. Then we can remove frame here.
I answer this in patch 3.
+ /* Architecture dependent information. */
+ struct gdbarch *gdbarch;
Is gdbarch used?
+};
No indeed I forgot to clean that up, fixed.