Message ID | 20180605220556.7418-1-sergiodj@redhat.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 45163 invoked by alias); 5 Jun 2018 22:06:21 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 45149 invoked by uid 89); 5 Jun 2018 22:06:20 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.6 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=consolidate, unbreak X-HELO: mx1.redhat.com Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 05 Jun 2018 22:06:19 +0000 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8B6B640200A0; Tue, 5 Jun 2018 22:06:17 +0000 (UTC) Received: from psique.yyz.redhat.com (unused-10-15-17-196.yyz.redhat.com [10.15.17.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id 27BE12166BB2; Tue, 5 Jun 2018 22:06:17 +0000 (UTC) From: Sergio Durigan Junior <sergiodj@redhat.com> To: GDB Patches <gdb-patches@sourceware.org> Cc: Alan Hayward <alan.hayward@arm.com>, nd@arm.com, Sergio Durigan Junior <sergiodj@redhat.com> Subject: [PATCH] Guard declarations of 'sve_*_from_*' macros on Aarch64 (and unbreak build) Date: Tue, 5 Jun 2018 18:05:56 -0400 Message-Id: <20180605220556.7418-1-sergiodj@redhat.com> In-Reply-To: <87zi09vw7m.fsf@redhat.com> References: <87zi09vw7m.fsf@redhat.com> X-IsSubscribed: yes |
Commit Message
Sergio Durigan Junior
June 5, 2018, 10:05 p.m. UTC
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(+)
Comments
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
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 +#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 +#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 /* Maximum supported VQ value. Increase if required. */