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

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

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