Re: [PATCH 1/5] aarch64-tdep basic port.

[Joel Brobecker <brobecker at adacore dot 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>
> 
>         * Makefile.in: Add AArch64.
>         * aarch64-tdep.c: New file.
>         * aarch64-tdep.h: New file.
>         * 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).

A few more minor comments, and I expect the next version of this
patch to be good to go.

> +/* Decode an opcode if it represents an immediate ADD or SUB instruction.
> +
> +   ADDR specifies the address of the opcode.
> +   INSN specifies the opcode to test.
> +   RD receives the 'rd' field from the decoded instruction.
> +   RN receives the 'rn' field from the decoded instruction.
> +
> +   Return 1 if the opcodes matches and is decoded, otherwise 0.
> + */

This is a bit of a nit-pick, but since there are going to be other
modificasions, the closing "*/" is typically placed at the end of
the comment, rather than on the next line (unless there is not
enough room to do so). Can you adjust all function descriptions
accordingly?

> +  /* See if we can determine the end of the prologue via the symbol
> +     table.  If so, then return either PC, or the PC after the
> +     prologue, whichever is greater.  */
> +  if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
> +    {
> +      CORE_ADDR post_prologue_pc = skip_prologue_using_sal (gdbarch, func_addr);

This line is too long. Can you reformat it as follow?

        CORE_ADDR post_prologue_pc
          = skip_prologue_using_sal (gdbarch, func_addr);

> +      if (sal.line == 0)
> +	/* No line info so use the current PC.  */
> +	prologue_end = prev_pc;
> +      else if (sal.end < prologue_end)
> +	/* The next line begins after the function end.  */
> +	prologue_end = sal.end;

Because you moved the block inside the if/else section, it visually
looks as if there was 2 statements. We've decided for the GDB project
that we would use curly braces in this case, even if they are actually
superfluous. Thus:

      if (sal.line == 0)
        {
	  /* No line info so use the current PC.  */
	  prologue_end = prev_pc;
        }
      else if (sal.end < prologue_end)
        {
	  /* The next line begins after the function end.  */
	  prologue_end = sal.end;
        }

> +	    member0_type = check_typedef (TYPE_FIELD_TYPE (ty, 0));
> +	    if (TYPE_CODE (member0_type) == TYPE_CODE_FLT)
> +	      {
> +		int i;
> +		for (i = 0; i < TYPE_NFIELDS (ty); i++)

Missing empty line after variable declarations...

> +  /* 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.

> +  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);

Missing empty line after variable declaration...

> +  /* Now we have tuned the configuration, set a few final things,
> +     based on what the OS ABI has told us.  */
> +
> +  if (tdep->jb_pc >= 0)
> +    set_gdbarch_get_longjmp_target (gdbarch, aarch64_get_longjmp_target);

Have you answered the comment I made in my first review? I can't
find anything...

 | 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.

> +  /* Debug this file's internals.  */
> +  add_setshow_zinteger_cmd ("aarch64", class_maintenance, &aarch64_debug, _("\

This is something that I missed in the first review, because
relatively recent, but we've decided that it was friendlier
to use a boolean, instead of a zinteger. If you eventually
need to use more than 2 states, it's pretty easy to upgrade.

> diff --git a/gdb/features/aarch64-core.xml b/gdb/features/aarch64-core.xml
> new file mode 100644
> index 0000000..e1e9dc3
> --- /dev/null
> +++ b/gdb/features/aarch64-core.xml
> @@ -0,0 +1,46 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2009-2012 Free Software Foundation, Inc.

Can you update the copyright year range of all XML files to include
2013, please?

Thank you,
-- 
Joel