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

[Simon Marchi <simon.marchi@polymtl.ca>]

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