Re: [PATCH v2 02/10] Add Aarch64 SVE Linux headers

Simon Marchi
2018-06-08T14:37:36Z

On 2018-06-08 10:13, Alan Hayward wrote:
> (Moved review to correct thread)
> Thanks for the reviews.
>
>> On 7 Jun 2018, at 21:18, Simon Marchi <simon.marchi@ericsson.com>
>> wrote:
>>
>> Hi Alan,
>>
>> Just some quick comments.
>>
>> I get this when building on x86-64 with --enable-targets=all:
>
> Hmm.. I had lost that flag from my build script. I Re-added it, and
> reproduced the issues.
>
>>
>> CXX aarch64-tdep.o
>> In file included from
>> /home/emaisin/src/binutils-gdb/gdb/nat/aarch64-sve-linux-ptrace.h:29:0,
>> from
>> /home/emaisin/src/binutils-gdb/gdb/aarch64-tdep.c:61:
>> /home/emaisin/src/binutils-gdb/gdb/nat/aarch64-linux-sigcontext.h:19:22:
>> error: field ‘head’ has incomplete type ‘_aarch64_ctx’
>> struct _aarch64_ctx head;
>> ^
>> /home/emaisin/src/binutils-gdb/gdb/nat/aarch64-linux-sigcontext.h:19:9:
>> note: forward declaration of ‘struct _aarch64_ctx’
>> struct _aarch64_ctx head;
>> ^
>>
>> First, we should not include "nat/aarch64-sve-linux-ptrace.h" (a file
>> that only makes
>> sense when building on AArch64) in aarch64-tdep.c, a file built on all
>> architecture
>> when including the support for AArch64 debugging. It looks like
>> aarch64-tdep.c
>> needs sve_vq_from_vl. Maybe that definition could be moved to arch/,
>> which can be
>> included in aarch64-tdep.c.
>>
>
> I had put it in there because I wanted to try and make it a complete
> block
> copied from Linux. The issue makes sense, so I’ve updated to restore
> sve_vq_from_vl/sve_vl_from_vq back to arch/aarch64.h and removed it
> from
> nat/aarch64-linux-sigcontext.h
>
>
>> Then, is the _aarch64_ctx structure guaranteed to be defined on older
>> AArch64 kernels
>> or should we include it too?
>
>
> _aarch64_ctx is part of the standard aarch64 signal handling. A quick
> git blame gives
> me 2012 - which is roughly the age of aarch64. So, it should always be
> defined.
>
>
> Updated patch below. Checked it builds (with other sve patches) on:
> X86 all-targets
> Aarch64 Linux 4.10 (pre sve headers) ubuntu 16.04
> Aarch64 Linux 4.15 (with sve headers) ubuntu 18.04
>
> Are you ok with the new version?

The code looks good to me, thanks. I am still unsure about the
licensing side of it, let me ask the FSF people about it, I'll come back
to you when it's done. I hope it won't take too long!

Simon