This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/5] aarch64-linux-tdep patch
- 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, 13 Jan 2013 11:30:43 +0400
- Subject: Re: [PATCH 2/5] aarch64-linux-tdep patch
- References: <50AD0319.6020400@arm.com> <20121223064919.GL5370@adacore.com> <50EADB67.3070000@arm.com>
> 2013-01-07 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.
> * configure.tgt: Add aarch64-none-linux-gnu.
> * aarch64-tdep.h (gdbarch_tdep): Define gregset
> and fpregset.
The description of the Makefile.in changes is missing.
And the config/aarch64/aarch64-linux.mh should be removed.
Almost there...
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 673efa1..02fe931 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -519,7 +519,7 @@ TARGET_OBS = @TARGET_OBS@
> # All target-dependent objects files that require 64-bit CORE_ADDR
> # (used with --enable-targets=all --enable-64-bit-bfd).
> ALL_64_TARGET_OBS = \
> - aarch64-tdep.o \
> + aarch64-linux-tdep.o aarch64-tdep.o \
This is a bit of a nitpick, but OSABI-specific files are usually
added after the generic tdep entry...
> ALLDEPFILES = \
> - aarch64-tdep.c \
> + aarch64-linux-tdep.c aarch64-tdep.c \
Same here...
> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
> new file mode 100644
> index 0000000..83128a6
> --- /dev/null
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -0,0 +1,299 @@
[...]
> +#include "stap-probe.h"
> +#include "user-regs.h"
> +
> +#include <ctype.h>
Can you explain why you added includes of "stap-probe.h", "user-regs.h"
and <ctype.h>? It doesn't look like the code changed much since the
previous version of the patch...
> +/* Signal frame handling.
> +
> + +----------+ ^
> + | saved lr | |
> + +->| saved fp |--+
> + | | |
This comment was just before aarch64_linux_sigframe_init, which
you moved. I am wondering whether you wanted to move this comment
as well...
> +static void
> +aarch64_linux_sigframe_init (const struct tramp_frame *self,
> + struct frame_info *this_frame,
> + struct trad_frame_cache *this_cache,
> + CORE_ADDR func)
I'd also appreciate if you wouldn't mind adding a small description
of what this function is expected to do. Since it is a "method" of
struct tramp_frame, and the expected behavior is described at the
definition of that struct, it's sufficient to say:
/* Implement the "init" method of struct tramp_frame. */
> + /* Note: We do not do anything with the two pseudo registers orig_X0
> + and syscallno (the 35th and 36th entries in the register buffer)
> + at the moment. */
If you know why, it'd be interesting to document it here...
> --- a/gdb/configure.tgt
> +++ b/gdb/configure.tgt
> @@ -36,6 +36,13 @@ aarch64*-*-elf)
> gdb_target_obs="aarch64-tdep.o"
> ;;
>
> +aarch64*-*-linux*)
> + # Target: AArch64 linux
> + gdb_target_obs="aarch64-tdep.o aarch64-linux-tdep.o \
> + glibc-tdep.o linux-tdep.o solib-svr4.o \
> + symfile-mem.o"
> + build_gdbserver=yes
The "build_gdbserver" portion needs to be moved to the patch
adding gdbserver support. Otherwise, someone attempting to
build GDB without --disable-gdbserver after applying this patch
would get a failure.
--
Joel