Message ID | 20140916112618.GO6586@spoyarek.pnq.redhat.com |
---|---|
State | Committed |
Headers |
Received: (qmail 16131 invoked by alias); 16 Sep 2014 11:26:31 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 16117 invoked by uid 89); 16 Sep 2014 11:26:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.7 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Date: Tue, 16 Sep 2014 16:56:18 +0530 From: Siddhesh Poyarekar <siddhesh@redhat.com> To: Jakub Jelinek <jakub@redhat.com> Cc: libc-alpha@sourceware.org, David Sommerseth <davids@redhat.com>, carlos@redhat.com, allan@archlinux.org Subject: [PATCH v2] Make __extern_always_inline usable on clang++ again Message-ID: <20140916112618.GO6586@spoyarek.pnq.redhat.com> References: <20140916102016.GM6586@spoyarek.pnq.redhat.com> <20140916104146.GX17454@tucnak.redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="ctUzwJm0i+kwMBIK" Content-Disposition: inline In-Reply-To: <20140916104146.GX17454@tucnak.redhat.com> User-Agent: Mutt/1.5.22.1-rc1 (2013-10-16) |
Commit Message
Siddhesh Poyarekar
Sept. 16, 2014, 11:26 a.m. UTC
On Tue, Sep 16, 2014 at 12:41:46PM +0200, Jakub Jelinek wrote: > s/fot/for/; why are you using a separate hunk for clang instead of using the > earlier one? I thought it looked ugly when I wrote the entire conditional in one place but I notice now (since you pointed it out) that it actually makes things wrong. > > +#if !defined __extern_always_inline && defined __clang__ > > +# if defined __GNUC_STDC_INLINE__ || defined __GNUC_GNU_INLINE__ > > +# define __extern_inline extern __inline __attribute__ ((__gnu_inline__)) > > +# define __extern_always_inline \ > > + extern __always_inline __attribute__ ((__gnu_inline__)) > > +# else > > +# define __extern_inline extern __inline > > +# define __extern_always_inline extern __always_inline > > This doesn't look right. If you have clang that doesn't have gnu_inline > support, in C++ extern __inline definitely is not the right semantics. > You don't want to define the macros then. Here's the updated patch, again tested with the same program. Siddhesh [BZ #17266] * misc/sys/cdefs.h: Define __extern_always_inline for clang 4.2 and newer.
Comments
On 09/16/2014 07:26 AM, Siddhesh Poyarekar wrote: > On Tue, Sep 16, 2014 at 12:41:46PM +0200, Jakub Jelinek wrote: >> s/fot/for/; why are you using a separate hunk for clang instead of using the >> earlier one? > > I thought it looked ugly when I wrote the entire conditional in one > place but I notice now (since you pointed it out) that it actually > makes things wrong. > >>> +#if !defined __extern_always_inline && defined __clang__ >>> +# if defined __GNUC_STDC_INLINE__ || defined __GNUC_GNU_INLINE__ >>> +# define __extern_inline extern __inline __attribute__ ((__gnu_inline__)) >>> +# define __extern_always_inline \ >>> + extern __always_inline __attribute__ ((__gnu_inline__)) >>> +# else >>> +# define __extern_inline extern __inline >>> +# define __extern_always_inline extern __always_inline >> >> This doesn't look right. If you have clang that doesn't have gnu_inline >> support, in C++ extern __inline definitely is not the right semantics. >> You don't want to define the macros then. > > Here's the updated patch, again tested with the same program. > > Siddhesh > > [BZ #17266] > * misc/sys/cdefs.h: Define __extern_always_inline for clang > 4.2 and newer. > > > diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h > index d8ee73c..6a789e7 100644 > --- a/misc/sys/cdefs.h > +++ b/misc/sys/cdefs.h > @@ -318,8 +318,14 @@ > #endif > > /* GCC 4.3 and above with -std=c99 or -std=gnu99 implements ISO C99 > - inline semantics, unless -fgnu89-inline is used. */ > -#if !defined __cplusplus || __GNUC_PREREQ (4,3) > + inline semantics, unless -fgnu89-inline is used. > + > + clang++ identifies itself as gcc-4.2, but has support for GNU inlining > + semantics, that can be checked for by using the __GNUC_STDC_INLINE_ and > + __GNUC_GNU_INLINE__ macro definitions. */ OK, Good comments. > +#if (!defined __cplusplus || __GNUC_PREREQ (4,3) \ > + || (defined __clang__ && (defined __GNUC_STDC_INLINE__ \ > + || defined __GNUC_GNU_INLINE__))) > # if defined __GNUC_STDC_INLINE__ || defined __cplusplus > # define __extern_inline extern __inline __attribute__ ((__gnu_inline__)) > # define __extern_always_inline \ The existing code says: (1) If GCC 4.3 or later and __GNUC_STDC_INLINE__ then add __gnu_inline__. (2) If GCC 4.3 or later and it's C++ then add __gnu_inline__. (3) if it's not C++ and __GNUC_STDC_INLINE__ then add __gnu_inline__ The new code says: (1) If GCC 4.3 or later and __GNUC_STDC_INLINE__ then add __gnu_inline__. (2) If GCC 4.3 or later and it's C++ then add __gnu_inline__. (3) If it's not C++ and __GNUC_STDC_INLINE__ then add __gnu_inline__. (4) If Clang and __GNUC_STDC_INLINE__ then add __gnu_inline__. - Notice if we split the compilers up we might ahve folded this condition. (5) If Clang and __GNUC_GNU_INLINE__ and C++ then add __gnu_inline__. I omitted some of the redundant conditional sets. This looks correct. So OK. My preference is still to have this conditional be distinct for clang and instead set something like HAVE_FOO, similarly for gcc, and then have a single #if HAVE_FOO to set or not set the definitions to add or not __gnu_inline__. This allows a cleaner representation and folding of the checks for each compiler based on the compiler's intricacies. Cheers, Carlos.
Any reason this patch was not committed to the 2.20 branch. The extern inline one was (BZ #17266, commit 979add9f) and without this one the 2.20 branch is quite broken with clang. Allan
On Sun, Nov 23, 2014 at 07:15:12PM +1000, Allan McRae wrote: > Any reason this patch was not committed to the 2.20 branch. The extern > inline one was (BZ #17266, commit 979add9f) and without this one the > 2.20 branch is quite broken with clang. No reason at all. I'll backport it. Siddhesh
On 11/23/2014 04:15 AM, Allan McRae wrote: > Any reason this patch was not committed to the 2.20 branch. The extern > inline one was (BZ #17266, commit 979add9f) and without this one the > 2.20 branch is quite broken with clang. No reason that I know of. Please go ahead and push it into 2.20. Per my suggested rules[1] the fact that this was OK for master makes it OK for 2.20. [1] https://sourceware.org/ml/libc-alpha/2014-10/msg00057.html Cheers, Carlos.
On Mon, Nov 24, 2014 at 01:48:34PM -0500, Carlos O'Donell wrote:
> No reason that I know of. Please go ahead and push it into 2.20.
I did it yesterday:
https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=d73ac1bb436cf1adb62335f53b4fc91a02f40a3b
Siddhesh
diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h index d8ee73c..6a789e7 100644 --- a/misc/sys/cdefs.h +++ b/misc/sys/cdefs.h @@ -318,8 +318,14 @@ #endif /* GCC 4.3 and above with -std=c99 or -std=gnu99 implements ISO C99 - inline semantics, unless -fgnu89-inline is used. */ -#if !defined __cplusplus || __GNUC_PREREQ (4,3) + inline semantics, unless -fgnu89-inline is used. + + clang++ identifies itself as gcc-4.2, but has support for GNU inlining + semantics, that can be checked for by using the __GNUC_STDC_INLINE_ and + __GNUC_GNU_INLINE__ macro definitions. */ +#if (!defined __cplusplus || __GNUC_PREREQ (4,3) \ + || (defined __clang__ && (defined __GNUC_STDC_INLINE__ \ + || defined __GNUC_GNU_INLINE__))) # if defined __GNUC_STDC_INLINE__ || defined __cplusplus # define __extern_inline extern __inline __attribute__ ((__gnu_inline__)) # define __extern_always_inline \