On 23/12/12 06:34, Joel Brobecker wrote:
> 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.

Joel, Thanks for the review.  All of your comments have been addressed 
in this revised version of the patch, some specific comments included below:

> Note: I am confused now as to whether those debug traces should
> be printed on stdlog or stderr.

We have switched them all to gdb_stdlog.

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

There are two references:

./aarch64-linux-tdep.c:391:  tdep->lowest_pc = 0x8000;
./aarch64-tdep.c:2619:  tdep->lowest_pc = 0x20;

>
>> +  /* Breakpoint pattern for an AArch64 insn.  */
>> +  const char *aarch64_breakpoint;
>> +  /* And its size.  */
>> +  int aarch64_breakpoint_size;
>
> Same here...

This one has been removed.

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

Moved.

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

The code is question is used to handle setjmp/longjmp on newlib, that 
behaviour is newlib specific.  There is no sniffer (yet) in the current 
code, but the functionality can be enabled using "set osabi Newlib".



Proposed ChangeLog:

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

Cheers
/Marcus