| Message ID | 20140916112618.GO6586@spoyarek.pnq.redhat.com |
|---|---|
| State | New |
| Headers | show |
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 \