This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/5] AArch64 GDB and GDBSERVER Port V2
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Marcus Shawcroft <marcus dot shawcroft at arm dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Sun, 23 Dec 2012 10:49:19 +0400
- Subject: Re: [PATCH 2/5] AArch64 GDB and GDBSERVER Port V2
- References: <50AD0319.6020400@arm.com>
> 2012-11-21 Jim MacArthur <jim.macarthur@arm.com>
> Marcus Shawcroft <marcus.shawcroft@arm.com>
> Nigel Stephens <nigel.stephens@arm.com>
> Yufeng Zhang <yufeng.zhang@arm.com>
>
> * aarch64-linux-tdep.c: New file.
> * config/aarch64/aarch64-linux.mh: New file.
As discussed in the review of patch #1, a piece from patch #1 for
configure.tgt will need to be included here.
IIRC, there is also a change in aarch64-tdep.h that needs to be
moved to this patch.
> +++ b/gdb/aarch64-linux-tdep.c
[...]
> +#include <sys/ptrace.h>
This is a big no-no for a tdep file: It prevents a user from
building an aarch64-linux cross debugger. But I do not see
a call to ptrace in this file, so perhaps it can simply be deleted.
> +/* The general-purpose regset consists of 31 X registers, plus SP, PC,
> + PSTATE and two extra pseudo 64-bit registers, as defined in the
> + AArch64 port of the Linux kernel. */
> +#define AARCH64_LINUX_SIZEOF_GREGSET (36 * X_REGISTER_SIZE)
> +/* The fp regset consists of 32 V registers, plus FPCR and FPSR which
> + are 4 bytes wide each, and the whole structure is padded to 128 bit
> + alignment. */
> +#define AARCH64_LINUX_SIZEOF_FPREGSET (33 * V_REGISTER_SIZE)
Can you add an empty line between the two macros. I think it will
make things a little clearer.
> +/* Signal frame handling.
> +
> + +----------+ ^
> + | saved lr | |
> + +->| saved fp |--+
> + | | |
> +
These types of detailed comments are always very much appreciated.
Thank you!
> +static void
> +aarch64_linux_sigframe_init (const struct tramp_frame *,
> + struct frame_info *,
> + struct trad_frame_cache *,
> + CORE_ADDR);
Please provide the names of the parameters.
> +static const struct tramp_frame aarch64_linux_rt_sigframe = {
Brace at the start of the next line, please.
> +extern void aarch64_linux_supply_gregset (struct regcache *, const gdb_byte *);
> +extern void aarch64_linux_supply_fpregset (struct regcache *, const gdb_byte *);
Please provide the names of the arguments in your prototypes.
--
Joel