Guard declarations of 'sve_*_from_*' macros on Aarch64 (and unbreak build)

  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.  It is important to mention that the
system headers use a value to multiply/divide the vector
length (SVE_VQ_BYTES, defined as 16 on the system I'm using) which is
different than the values being used by the macros defined in GDB.  I
wasn't sure how to consolidate that, so I chose to ignore this
discrepancy.

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.

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(+)
  

On Tuesday, June 05 2018, I wrote:

> In order to fix this breakage, this commit guards the declaration of
> the macros using #ifndef's.  It is important to mention that the
> system headers use a value to multiply/divide the vector
> length (SVE_VQ_BYTES, defined as 16 on the system I'm using) which is
> different than the values being used by the macros defined in GDB.  I
> wasn't sure how to consolidate that, so I chose to ignore this
> discrepancy.

Actually, I just saw that there's no discrepancy.  The system header
defines the following macros:

  #define SVE_VQ_BYTES            16      /* number of bytes per quadword */                     
  ...
  #define sve_vq_from_vl(vl)      ((vl) / SVE_VQ_BYTES)                                          
  #define sve_vl_from_vq(vq)      ((vq) * SVE_VQ_BYTES)                                          

and GDB does this:

  #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

which is fine.  I'll remove this comment from the commit log.

Thanks,
  

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

>  It is important to mention that the

> system headers use a value to multiply/divide the vector

> length (SVE_VQ_BYTES, defined as 16 on the system I'm using) which is

> different than the values being used by the macros defined in GDB.  I

> wasn't sure how to consolidate that, so I chose to ignore this

> discrepancy.

> 


Yes, (as you pointed out in the next email), they compile down to the same.
When I moved them I changed to explicit values to remove the dependency.


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


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


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

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



Alan.
  

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.

Simon
  

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