[v2] Make __extern_always_inline usable on clang++ again
diff mbox

Message ID 20140916112618.GO6586@spoyarek.pnq.redhat.com
State New
Headers show

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

Carlos O'Donell Sept. 16, 2014, 4:14 p.m. UTC | #1
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.
Allan McRae Nov. 23, 2014, 9:15 a.m. UTC | #2
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
Siddhesh Poyarekar Nov. 24, 2014, 9:39 a.m. UTC | #3
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
Carlos O'Donell Nov. 24, 2014, 6:48 p.m. UTC | #4
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.
Siddhesh Poyarekar Nov. 25, 2014, 8:01 a.m. UTC | #5
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

Patch
diff mbox

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 \