[PATCH] gdbserver: Add GNU/Linux support for ARC
Shahab Vahedi
shahab.vahedi@gmail.com
Tue Oct 6 16:21:13 GMT 2020
Hi Simon,
On Wed, Sep 16, 2020 at 03:59:42PM -0400, Simon Marchi wrote:
> On 2020-08-26 12:56 p.m., Shahab Vahedi via Gdb-patches wrote:
> > +static const struct target_desc *
> > +arc_linux_read_description (void)
> > +{
> > +#ifdef __ARC700__
> > + arc_gdbarch_features features (4, ARC_ISA_ARCV1);
> > +#else
> > + arc_gdbarch_features features (4, ARC_ISA_ARCV2);
> > +#endif
>
> I should have mentionned this in an earlier review, but
> "arc_gdbarch_features" is a bit mis-named. "gdbarch" specifically
> refers to the type/interface in GDB, but arc_gdbarch_features is not
> related to that. "arc_arch_features" would be a bit better, if you want
> to change it in an obvious patch.
I renamed it in the next version. Therefore, v2 is a patch series and
not a single patch anymore.
> > +/* The breakpoint instruction is TRAP_S 1, network function ntohs can be
> > + used to convert its little-endian form (0x3e78) to the host
> > + representation, which may be little-endian or big-endian (network
> > + representation is defined to be little-endian). */
>
> Isn't network byte order big endian?
Got rid of it.
> > +static void
> > +arc_fill_gregset (struct regcache *regcache, void *buf)
> > +{
> > + struct user_regs_struct *regbuf = (struct user_regs_struct *) buf;
> > +
> > + /* Core registers. */
> > + collect_register_by_name (regcache, "r0", &(regbuf->scratch.r0));
> > ...
> > + /* PC should be written to "return address", instead of stop address. */
> > + collect_register_by_name (regcache, "pc", &(regbuf->scratch.ret));
>
> Can you explain why the PC is written to "ret", whereas when going the
> other way we take the value from the "stop_pc" field?
I've added the following comments in the code in v2. I hope it is
helpful.
/* The current "pc" value must be written to "eret" (exception return
address) register, because that is the address that the kernel code
will jump back to after a breakpoint exception has been raised.
The "pc_stop" value is ignored by the genregs_set() in
linux/arch/arc/kernel/ptrace.c. */
collect_register_by_name (regcache, "pc", &(regbuf->scratch.ret));
/* The genregs_get() in linux/arch/arc/kernel/ptrace.c populates the
pseudo register "stop_pc" with the "efa" (exception fault address)
register. This was deemed necessary, because the breakpoint
instruction, "trap_s 1", is a committing one; i.e. the "eret"
(excpetion return address) register will be pointing to the next
instruction, while "efa" points to the address that raised the
breakpoint. */
supply_register_by_name (regcache, "pc", &(regbuf->stop_pc));
> > +
> > +const gdb_byte *
> > +arc_target::sw_breakpoint_from_kind (int kind, int *size)
> > +{
> > + static bool initialized = false;
> > + static gdb_byte arc_linux_trap_s[TRAP_S_1_SIZE] = { 0, };
> > +
> > + if (!initialized)
> > + {
> > + uint16_t breakpoint = ntohs (TRAP_S_1_OPCODE);
> > + *((uint16_t *) arc_linux_trap_s) = breakpoint;
> > + initialized = true;
> > + }
> > +
> > + gdb_assert (kind == TRAP_S_1_SIZE);
>
> I always find it a little odd that we choose the breakpoint size as the
> kind, instead of just an arbitrary number (first one 0, second one 1,
> etc). What if we want to use another breakpoint instruction that is
> two bytes long? Anyway, not really a problem, just something I wonder
> about.
I agree. I rather use an enum that acts like a key in a map that returns
the size. I see this pattern in "gdb" code as well. I plan to tackle
this in a separate patch. So I can focus on sharing the code base as
much as possible.
Cheers,
Shahab
More information about the Gdb-patches
mailing list