[PATCH 1/5] AArch64 GDB and GDBSERVER Port V2
Joel Brobecker
brobecker@adacore.com
Sun Dec 23 06:34:00 GMT 2012
Marcus,
I sincerely apologize for the delay in getting this reviewed. Several
of us seem to have had a busy end of year, and it is not easy to find
a slot of quiet time long enough to review such large patches.
> 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>
>
> * Makefile.in: Add AArch64.
> * aarch64-tdep.c: New file.
> * aarch64-tdep.h: New file.
> * configure.host: Add AArch64.
> * configure.tgt: Add AArch64.
> * features/Makefile: Add AArch64.
> * features/aarch64-core.xml: New file.
> * features/aarch64-fpu.xml: New file.
> * features/aarch64-without-fpu.c: New file (generated).
> * features/aarch64-without-fpu.xml: New file.
> * features/aarch64.c: New file (generated).
> * features/aarch64.xml: New file.
> * regformats/aarch64-without-fpu.dat: New file (generated).
> * regformats/aarch64.dat: New file (generated).
See my comments below. Nice code overall, just many many little nits
that should be very easy to fix. A few important questions at the end,
as well.
A request: Please fix the subject of each of your patches so that
it actually tells us what the objective of the patch is. I kept
getting confused while trying to refer to patch numbers while
writing this review...
I will take a look at the rest of the GDB patches knowing that
a second review will be needed anyways, because some of the fixes
requested here will mean a change in those other patches.
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 3dd7b85..b1bed86 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -517,6 +517,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-linux-tdep.o aarch64-newlib-tdep.o aarch64-tdep.o \
This change should only add aarch64-tdep.o to the list.
> ALLDEPFILES = \
> + aarch64-linux-nat.c \
> + aarch64-linux-tdep.c aarch64-newlib-tdep.c aarch64-tdep.c \
Same here.
> +static int32_t
> +extract_signed_bitfield (uint32_t insn, unsigned width, unsigned offset)
Can you add a comment documenting the function, please? There are
several other functions that need the same treatment. Please
make sure to document the various arguments.
> +static int
> +decode_add_sub_imm (CORE_ADDR addr, uint32_t insn, unsigned *rd, unsigned *rn,
> + int32_t * imm)
> +{
> + if ((insn & 0x9f000000) == 0x91000000)
Can you add a small comment explaining what instructions
this check matches?
> + if (decode_masked_match (insn, 0x9f000000, 0x90000000))
Same here; although perhaps the function description might be
sufficient to make this check obvious.
> +static CORE_ADDR
> +aarch64_analyze_prologue (struct gdbarch *gdbarch,
[...]
> + pv_t regs[32];
I tend to try to avoid litteral constants like these. Can we create
a named constant, or at worst a macro, which names this value? It
makes it more obvious what it is about; and also avoids repeating
this constant later, for instance:
> + for (i = 0; i < 32; i++)
> + regs[i] = pv_register (i, 0);
> + /* If recording this store would invalidate the store area
> + (perhaps because rn is not known) then we should abandon
> + further prologue analysis. */
> + if (pv_area_store_would_trash (stack,
> + pv_add_constant (regs[rn], imm)) ||
> + pv_area_store_would_trash (stack,
> + pv_add_constant (regs[rn], imm + 8)))
The binary opeator "||" should be at the start of the next line,
rather than at the end. Alternatively, you can write this as
two conditions. For instance
if (pv_area_store_would_trash (stack,
pv_add_constant (regs[rn], imm)))
break;
if (pv_area_store_would_trash (stack,
pv_add_constant (regs[rn], imm + 8)))
break;
> + for (i = 0; i < 32; i++)
> + {
> + CORE_ADDR offset;
> + if (pv_area_find_reg (stack, gdbarch, i, &offset))
Can you add an empty line after the variable declaration?
> + if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
> + {
> + CORE_ADDR post_prologue_pc =
> + skip_prologue_using_sal (gdbarch, func_addr);
Same here...
> +/* Called by aarch64_make_prologue_cache only. */
> +
> +static void
> +aarch64_scan_prologue (struct frame_info *this_frame,
> + struct aarch64_prologue_cache *cache)
> +{
Can you document what the function does, rather than who calls it?
> + if (sal.line == 0) /* no line info, use current PC */
/* No line info, use the current PC. */
> + prologue_end = prev_pc;
> + else if (sal.end < prologue_end) /* next line begins after fn end */
Same as above (sentences start with capital letters and end with a
period).
> + if (prev_regnum == AARCH64_PC_REGNUM)
> + {
> + CORE_ADDR lr;
> + lr = frame_unwind_register_unsigned (this_frame, AARCH64_LR_REGNUM);
Missing empty line after variable declaration.
> + /* SP is generally not saved to the stack, but this frame is
> + identified by the next frame's stack pointer at the time of the
> + call. The value was already reconstructed into PREV_SP. */
> + /*
> + * +----------+ ^
> + * | saved lr | |
> + * +->| saved fp |--+
> + * | | |
> + * | | | <- Previous SP
> + * | +----------+
> + * | | saved lr |
> + * +--| saved fp |<- FP
> + * | |
> + * | |<- SP
> + * +----------+
> + */
Can you remove the leading '*' in your comment above?
> +/* AArch64 default frame base information. */
> +struct frame_base aarch64_normal_base = {
Curly brace on the next line...
> +static CORE_ADDR
> +aarch64_unwind_pc (struct gdbarch *gdbarch, struct frame_info *this_frame)
> +{
> + CORE_ADDR pc = frame_unwind_register_unsigned (this_frame, AARCH64_PC_REGNUM);
This line is too long.
> +/* When arguments must be pushed onto the stack, they go on in reverse
> + order. The code below implements a FILO (stack) to do this. */
> +
> +struct stack_item
> +{
> + int len;
> + struct stack_item *prev;
> + void *data;
> +};
> +
> +static struct stack_item *
> +push_stack_item (struct stack_item *prev, const bfd_byte *contents, int len)
[etc]
Why not use a VEC, here? Also, at a minimum, the various fields in
your struct stack_item need to be documented individually.
> + if (TYPE_NFIELDS (ty) > 0 && TYPE_NFIELDS (ty) <= 4)
> + {
> + struct type *member0_type;
> + member0_type = check_typedef (TYPE_FIELD_TYPE (ty, 0));
Empty line after variable declaration...
> + if (TYPE_CODE (member0_type) == TYPE_CODE_FLT)
> + {
> + int i;
> + for (i = 0; i < TYPE_NFIELDS (ty); i++)
Same here...
> + {
> + struct type *member1_type;
> + member1_type = check_typedef (TYPE_FIELD_TYPE (ty, i));
And there...
> +/* AArch64 function call information structure. */
> +struct aarch64_call_info
> +{
> + unsigned argnum;
> + unsigned ncrn;
> + unsigned nvrn;
> + unsigned nsaa;
> + struct stack_item *si;
> +};
Can you document each field individually, please?
> + if (aarch64_debug)
> + fprintf_unfiltered (gdb_stdlog, "arg %d in %s = 0x%s\n",
^^^^^^^^^^
> + info->argnum,
> + gdbarch_register_name (gdbarch, regnum),
> + phex (regval, X_REGISTER_SIZE));
> + fprintf_unfiltered (gdb_stdlog, "arg %d in %s\n",
^^^^^^^^^^
> + info->argnum,
> + gdbarch_register_name (gdbarch, regnum));
> + if (aarch64_debug)
> + fprintf_unfiltered (gdb_stdlog, "arg %d len=%d @ sp + %d\n",
^^^^^^^^^^
> + info->argnum, len, info->nsaa);
Note: I am confused now as to whether those debug traces should
be printed on stdlog or stderr. I thought it was stderr, but
I see stdlog used everywhere for this purpose. Your code uses
either, but I am thinking now that it should be gdb_stdlog.
Can you adjust your code to consistently use the same output?
> + /* Push stack alignment padding. */
> + int pad = align - (info->nsaa & (align - 1));
> + info->si = push_stack_item (info->si, buf, pad);
Empty line after variable declaration...
> + gdb_assert (TYPE_CODE (func_type) == TYPE_CODE_FUNC ||
> + TYPE_CODE (func_type) == TYPE_CODE_METHOD);
"||" at start of next line.
> + /* If we were given an initial argument for the return slot because
> + lang_struct_return was true. Lose it. */
^^^ I'd use a comma, here.
> + {
> + const bfd_byte *buf = value_contents (arg);
> + struct type *target_type =
> + check_typedef (TYPE_TARGET_TYPE (arg_type));
> + pass_in_v (gdbarch, regcache, &info, buf);
Empty line after variable declaration...
> + int elements = TYPE_NFIELDS (arg_type);
> + /* Homogeneous Aggregates */
Same here...
> + if (info.nvrn + elements < 8)
> + {
> + int i;
> + for (i = 0; i < elements; i++)
And here...
> + {
> + /* We know that we have sufficient registers
> + available therefore this will never fallback
> + to the stack. */
> + struct value *field =
> + value_primitive_field (arg, 0, i, arg_type);
> + struct type *field_type =
> + check_typedef (value_type (field));
> + pass_in_v_or_stack (gdbarch, regcache, &info, field_type,
> + value_contents_writeable (field));
And here...
> +static int
> +aarch64_gdb_print_insn (bfd_vma memaddr, disassemble_info * info)
^^^
No space between '*' and 'info'.
> +static const char aarch64_default_breakpoint[] = {0x00,0x00,0x20,0xd4};
Missing spaces after comas.
> + bfd_byte buf[V_REGISTER_SIZE];
> + int len = TYPE_LENGTH (type);
> + regcache_cooked_read (regs, AARCH64_V0_REGNUM, buf);
Empty line after variable declarations...
> + else if (TYPE_CODE (type) == TYPE_CODE_COMPLEX)
> + {
> + int regno = AARCH64_V0_REGNUM;
> + bfd_byte buf[V_REGISTER_SIZE];
> + struct type *target_type = check_typedef (TYPE_TARGET_TYPE (type));
> + int len = TYPE_LENGTH (target_type);
> + regcache_cooked_read (regs, regno, buf);
Same.
> + if (TYPE_CODE (type) == TYPE_CODE_FLT)
> + {
> + bfd_byte buf[V_REGISTER_SIZE];
> + int len = TYPE_LENGTH (type);
> + memcpy (buf, valbuf, len > V_REGISTER_SIZE ? V_REGISTER_SIZE : len);
Same.
> +/* Implement the "pseudo_register_reggroup_p" tdesc_arch_data method. */
> +
> +static int
> +aarch64_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
> + struct reggroup *group)
> +{
> + regnum -= gdbarch_num_regs (gdbarch);
> +
> + if (regnum >= AARCH64_Q0_REGNUM && regnum < AARCH64_Q0_REGNUM + 32)
> + return group == all_reggroup || group == vector_reggroup;
> + else if (regnum >= AARCH64_D0_REGNUM && regnum < AARCH64_D0_REGNUM + 32)
> + return group == all_reggroup || group == vector_reggroup
> + || group == float_reggroup;
> + else if (regnum >= AARCH64_S0_REGNUM && regnum < AARCH64_S0_REGNUM + 32)
> + return group == all_reggroup || group == vector_reggroup
> + || group == float_reggroup;
> + else if (regnum >= AARCH64_H0_REGNUM && regnum < AARCH64_H0_REGNUM + 32)
> + return group == all_reggroup || group == vector_reggroup;
> + else if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
> + return group == all_reggroup || group == vector_reggroup;
For multi-line return expressions, the GNU Coding Standards require
that the expression be enclosed between parentheses. The parentheses
are strictly unnecessary, but help code formatters.
Thus, for instance:
return (group == all_reggroup || group == vector_reggroup
|| group == float_reggroup);
> + gdb_assert (0);
Use gdb_assert_not_reached.
> + /* Ensure the register buffer is zero, we want gdb writes of the
> + various 'scalar' pseudo registers to behavior like architectural
^^^^^^^^^^^ to behave?
> + writes, register width bytes are written the remainder are set to
writes: Register width bytes are written, and the remainder [...]
> + zero. */
> + memset (reg_buf, 0, sizeof (reg_buf));
> + gdb_assert (0);
Use gdb_assert_not_reached
> + if (tdep->jb_pc >= 0)
> + set_gdbarch_get_longjmp_target (gdbarch, aarch64_get_longjmp_target);
This code looks useless AFAICT, as tdep->jb_pc is set to -1 earlier
in this function, and I don't think anything changes its value before
this code.
> +void
> +_initialize_aarch64_tdep (void)
> +{
> + struct cmd_list_element *new_set, *new_show;
> + const char *setname;
> + const char *setdesc;
Can you delete those unused variables?
> +/* Register numbers of various important registers. */
> +enum gdb_regnum
> +{
> + AARCH64_X0_REGNUM, /* First integer register */
I would prefer it if you renamed this enum into something less
generic. For instance: enum aarch64_regnum.
> +/* Size of integer registers. */
> +#define X_REGISTER_SIZE 8
> +#define B_REGISTER_SIZE 1
> +#define H_REGISTER_SIZE 2
> +#define S_REGISTER_SIZE 4
> +#define D_REGISTER_SIZE 8
> +#define V_REGISTER_SIZE 16
> +#define Q_REGISTER_SIZE 16
Alignment issue if first macro.
> +/* Target-dependent structure in gdbarch. */
> +struct gdbarch_tdep
> +{
> + /* Lowest address at which instructions will appear. */
> + CORE_ADDR lowest_pc;
Why put this in the tdep vector if it is a constant? I checked
the other patches, and they do not reference this field at all.
> + /* Breakpoint pattern for an AArch64 insn. */
> + const char *aarch64_breakpoint;
> + /* And its size. */
> + int aarch64_breakpoint_size;
Same here...
> + /* Cached core file helpers. */
> + struct regset *gregset;
> + struct regset *fpregset;
These are unused in this patch. It's a bit of a pain, but can you
please remove them from this patch, and add it to the patch that
really uses them?
> diff --git a/gdb/configure.host b/gdb/configure.host
> index 7dc35e1..c5a7a3e 100644
This diff should be part of the patch that introduces native
support to GDB (ie patches #3 and #4).
> diff --git a/gdb/configure.tgt b/gdb/configure.tgt
> index 36d4304..63fd4b0 100644
> --- a/gdb/configure.tgt
> +++ b/gdb/configure.tgt
> @@ -31,6 +31,18 @@ esac
> # map target info into gdb names.
>
> case "${targ}" in
> +aarch64*-*-elf)
> + # Target: AArch64 embedded system
> + gdb_target_obs="aarch64-tdep.o aarch64-newlib-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
> + ;;
Similarly, the part that handles aarch64*-*-linux* should be
moved to patch #2 where it introduces aarch64-linux target
support.
I am also confused as to the relationship between this code
and the "newlib" one. From what I can tell, there is no way
to configure GDB without the newlib code. However, that code
seems to be unused in practice (because no sniffer seems to
be returning GDB_OSABI_NEWLIB).
Either:
(1) The newlib code is an integral part of the bare-metal
aarch64 code, in which case I think it should be part of this
patch. But then how is it activated (I may be missing something);
(2) The newlib code is not necessary, in which case
aarch64-newlib-tdep.o needs to be removed from gdb_target_obs.
--
Joel
More information about the Gdb-patches
mailing list