Re: [PATCH 1/5] AArch64 GDB and GDBSERVER Port V2

[Joel Brobecker <brobecker at adacore dot com>]

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