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

[Alan Hayward <Alan.Hayward@arm.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2703 bytes --]



> On 8 Jun 2018, at 15:37, Simon Marchi <simon.marchi@polymtl.ca> wrote:
> 
> 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!
> 

Ok, thanks for chasing that up. Happy to be cc’ed (or not) on any email.

This patch (I think) only blocks 5/10 and 9/10 in the series. The rest should be
ok to still go in (once reviewed).


Alan.


\x16º&Öéj×!zÊÞ¶êç׏4ïÉb²Ö«r\x18\x1dn–­r\x17¬