Re: [PATCH] Guard declarations of 'sve_*_from_*' macros on Aarch64 (and unbreak build)

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Wednesday, June 06 2018, Alan Hayward wrote:

>> On 5 Jun 2018, at 23:05, Sergio Durigan Junior <sergiodj@redhat.com> wrote:
>> 
>> Commit 122394f1476b1c925a281b15399119500c8231c1 ("Function for reading
>> the Aarch64 SVE vector length") has added macros to manipulate SVE
>> vector sizes based on Linux kernel sources, but did not guard them
>> with #ifndef's, which breaks the build when the system headers already
>> have these macros:
>> 
>>    CXX    aarch64-linux-nat.o
>>  In file included from ../../gdb/aarch64-tdep.h:25,
>>                   from ../../gdb/aarch64-linux-nat.c:30:
>>  ../../gdb/arch/aarch64.h:79: error: "sve_vq_from_vl" redefined [-Werror]
>>   #define sve_vq_from_vl(vl) ((vl) / 0x10)
>> 
>>  In file included from /usr/include/bits/sigcontext.h:30,
>>                   from /usr/include/signal.h:291,
>>                   from build-gnulib/import/signal.h:52,
>>                   from ../../gdb/linux-nat.h:23,
>>                   from ../../gdb/aarch64-linux-nat.c:26:
>>  /usr/include/asm/sigcontext.h:154: note: this is the location of the previous definition
>>   #define sve_vq_from_vl(vl) ((vl) / SVE_VQ_BYTES)
>> 
>>  In file included from ../../gdb/aarch64-tdep.h:25,
>>                   from ../../gdb/aarch64-linux-nat.c:30:
>>  ../../gdb/arch/aarch64.h:80: error: "sve_vl_from_vq" redefined [-Werror]
>>   #define sve_vl_from_vq(vq) ((vq) * 0x10)
>> 
>>  In file included from /usr/include/bits/sigcontext.h:30,
>>                   from /usr/include/signal.h:291,
>>                   from build-gnulib/import/signal.h:52,
>>                   from ../../gdb/linux-nat.h:23,
>>                   from ../../gdb/aarch64-linux-nat.c:26:
>>  /usr/include/asm/sigcontext.h:155: note: this is the location of the previous definition
>>   #define sve_vl_from_vq(vq) ((vq) * SVE_VQ_BYTES)
>> 
>> In order to fix this breakage, this commit guards the declaration of
>> the macros using #ifndef’s.
>
> Apologies for breaking this.
> I originally had them in nat/aarch64-sve-linux-ptrace.h, within the
> SVE_SIG_ZREGS_SIZE block, which does guard appropriately.
>
> Thanks for fixing so quickly.

No problem.

>> Tested by making sure GDB compiles fine again.  Since the system I'm
>> using doesn't have support for SVE, there's no way I can really test
>> these changes.
>> 
>
> Changes work fine for me on my SVE builds.

Thanks for testing!

>> gdb/ChangeLog:
>> 2018-06-05  Sergio Durigan Junior  <sergiodj@redhat.com>
>> 
>> 	* arch/aarch64.h (sve_vg_from_vl): Guard with #ifndef.
>> 	(sve_vl_from_vg): Likewise.
>> 	(sve_vq_from_vl): Likewise.
>> 	(sve_vl_from_vq): Likewise.
>> 	(sve_vq_from_vg): Likewise.
>> 	(sve_vg_from_vq): Likewise.
>> ---
>> gdb/arch/aarch64.h | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>> 
>> diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
>> index 9040d8d4c8..c378a4b239 100644
>> --- a/gdb/arch/aarch64.h
>> +++ b/gdb/arch/aarch64.h
>> @@ -74,12 +74,24 @@ enum aarch64_regnum
>>    VG : Vector Gradient.
>> 	The number of 64bit chunks in an SVE Z register.  */
>> 
>> +#ifndef sve_vg_from_vl
>> #define sve_vg_from_vl(vl)	((vl) / 8)
>> +#endif
>> +#ifndef sve_vl_from_vg
>> #define sve_vl_from_vg(vg)	((vg) * 8)
>> +#endif
>
> The guards around these first two aren’t needed. The kernel only
> defines the VQ to/from VL macros - as those are the only values the
> kernel cares about. Only GDB cares about VG because that is needed
> for dwarf.

Ah, fair enough.  Removed.

>> +#ifndef sve_vq_from_vl
>> #define sve_vq_from_vl(vl)	((vl) / 0x10)
>> +#endif
>> +#ifndef sve_vl_from_vq
>> #define sve_vl_from_vq(vq)	((vq) * 0x10)
>> +#endif
>
> These two are fine!
>
>> +#ifndef sve_vq_from_vg
>> #define sve_vq_from_vg(vg)	(sve_vq_from_vl (sve_vl_from_vg (vg)))
>> +#endif
>> +#ifndef sve_vg_from_vq
>> #define sve_vg_from_vq(vq)	(sve_vg_from_vl (sve_vl_from_vq (vq)))
>> +#endif
>
> Again these last two aren’t needed.

Removed.

On Wednesday, June 06 2018, Simon Marchi wrote:

> On 2018-06-06 03:34 AM, Alan Hayward wrote:
>> FYI, either today or tomorrow I’ll be posting a new set of SVE patches.
>> As part of that series, due to review comments, I’ll be moving the all
>> the linux kernel defines to two new files which contain only kernel
>> defines (From ptrace.h and sigcontext.h). I’ll double check this works
>> with new header files. Regardless of that, I think your patch should
>> still go in to unbreak the build until then.
>> 
>> 
>> Changes are good to me if the VG guards are removed (but I’m not a
>> maintainer so cannot officially approve it).
>
> LGTM with Alan's proposed changes.

Thanks, pushed:

e5a77256e8961294b3ea7d483124834311ca363b

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/