Re: [PATCH v4 4/6] Support software single step on ARM in GDBServer.

[Antoine Tremblay <antoine dot tremblay at ericsson 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.