| Message ID | 20140813182334.GA3507@spoyarek.pnq.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Aug 13, 2014 at 11:53:35PM +0530, Siddhesh Poyarekar wrote: > There is however a regression in this patch for llvm, since it reverts > the llvm expectation that __GNUC_STDC_INLINE__ or __GNUC_GNU_INLINE__ > definition imply proper extern inline semantics. Depends on what exactly clang does. GCC 4.1 and 4.2 define __GNUC_GNU_INLINE__ though, and do not support gnu_inline attribute in C++, therefore we can't let it through if __GNUC_GNU_INLINE__ is defined, since that is not a guarantee gnu_inline will work in C++. Jakub
On Wed, 13 Aug 2014, Siddhesh Poyarekar wrote: > Hi, > > The check for only __GNUC_STDC_INLINE__ and __GNUC_GNU_INLINE__ may > not be sufficient since those flags were added during initial support > for C99 inlining semantics. There is also a problem with always > defining __extern_inline and __extern_always_inline, since it enables > inline wrapper functions even when GNU inlining semantics are not > guaranteed. This, along with the possibility of such wrappers using > redirection (btowc for example) could result in compiler generating an > infinitely recusrive call to the function. Are you describing a problem that was user-visible in a glibc release? (I guess so, since you don't mention any post-2012 commits as having caused a regression.) What specifically is the problem (building what, with what compiler versions)? A bug report in glibc Bugzilla is needed if it was user-visible in a release.
On Wed, Aug 13, 2014 at 08:51:22PM +0000, Joseph S. Myers wrote: > On Wed, 13 Aug 2014, Siddhesh Poyarekar wrote: > > The check for only __GNUC_STDC_INLINE__ and __GNUC_GNU_INLINE__ may > > not be sufficient since those flags were added during initial support > > for C99 inlining semantics. There is also a problem with always > > defining __extern_inline and __extern_always_inline, since it enables > > inline wrapper functions even when GNU inlining semantics are not > > guaranteed. This, along with the possibility of such wrappers using > > redirection (btowc for example) could result in compiler generating an > > infinitely recusrive call to the function. > > Are you describing a problem that was user-visible in a glibc release? (I > guess so, since you don't mention any post-2012 commits as having caused a > regression.) What specifically is the problem (building what, with what > compiler versions)? A bug report in glibc Bugzilla is needed if it was > user-visible in a release. Support for pre-4.3 gcc in C++ has been broken with those 2012 changes. Consider say: #include <wchar.h> volatile int i = ' '; wint_t (*fn) (int) = btowc; int main () { asm (""); i = fn (i); return 0; } When this is compiled e.g. with g++ 3.2, because of the incorrect BZ #14530, #13741 fix, the program will recurse endlessly. That is because g++ < 4.3 only provided the C++ inline semantics, not gnu_inline, but when glibc headers use __extern_inline, __extern_always_inline or __fortify_function, they rely on GNU extern inline semantics. The C++ inline semantics when btowc can't be inlined is that the compiler emits a comdat out of line, but btowc is: extern wint_t __btowc_alias (int __c) __asm ("btowc"); __extern_inline wint_t __NTH (btowc (int __c)) { return (__builtin_constant_p (__c) && __c >= '\0' && __c <= '\x7f' ? (wint_t) __c : __btowc_alias (__c)); } When the compiler emits out of line copy of this, it will be e.g. on i?86/x86_64 .globl btowc; btowc: jmp btowc; Similarly with any other __extern_*inline functions in glibc headers that sometimes call the original function through aliases. One can get the problematic definitions out of line even with -fkeep-inline-functions and similar. So, what Siddhesh's patch does is it restores the old state, where __extern_inline/__extern_always_inline (and newly __fortify_function) is only defined if the compiler can actually support that semantics. In C++, that is only for g++ known to support gnu_inline attribute in C++. Jakub
On Wed, Aug 13, 2014 at 08:51:22PM +0000, Joseph S. Myers wrote: > On Wed, 13 Aug 2014, Siddhesh Poyarekar wrote: > > > Hi, > > > > The check for only __GNUC_STDC_INLINE__ and __GNUC_GNU_INLINE__ may > > not be sufficient since those flags were added during initial support > > for C99 inlining semantics. There is also a problem with always > > defining __extern_inline and __extern_always_inline, since it enables > > inline wrapper functions even when GNU inlining semantics are not > > guaranteed. This, along with the possibility of such wrappers using > > redirection (btowc for example) could result in compiler generating an > > infinitely recusrive call to the function. > > Are you describing a problem that was user-visible in a glibc release? (I > guess so, since you don't mention any post-2012 commits as having caused a > regression.) What specifically is the problem (building what, with what > compiler versions)? A bug report in glibc Bugzilla is needed if it was > user-visible in a release. Sorry, I have filed a bz now (#17266) with Jakub's explanation; I should have been clearer in saying that the Red Hat bz I mentioned in my email is the symptom that we see again now due to the changes to __extern_always_inline definition in cdefs.h. Siddhesh
Ping, is this patch OK? I don't want to necessarily commit it in 2.20, but it would be nice to keep it in queue for commit and maybe backport it to 2.20 later. Siddhesh On Wed, Aug 13, 2014 at 11:53:35PM +0530, Siddhesh Poyarekar wrote: > Hi, > > The check for only __GNUC_STDC_INLINE__ and __GNUC_GNU_INLINE__ may > not be sufficient since those flags were added during initial support > for C99 inlining semantics. There is also a problem with always > defining __extern_inline and __extern_always_inline, since it enables > inline wrapper functions even when GNU inlining semantics are not > guaranteed. This, along with the possibility of such wrappers using > redirection (btowc for example) could result in compiler generating an > infinitely recusrive call to the function. > > In fact it was such a recursion that led to this code being written > the way it was; see: > > https://bugzilla.redhat.com/show_bug.cgi?id=186410 > > The initial change: > > commit b7bfe116e6304da848759b69a6d713da3e93e936 > Author: Marek Polacek <polacek@redhat.com> > Date: Wed Sep 26 12:58:36 2012 +0200 > > Fix up definitions for older compilers. > > was to fix bugs 14530 and 13741, but they can be resolved by checking > if __fortify_function and/or __extern_always_inline are defined, as it > has been done in this patch. > > I have tested this with gcc 3.2, 3.4 and 4.1. In addition, I have > done a quick audit of uses of __extern_always_inline and > __extern_inline to make sure that none of the uses can result in > compilation errors. > > There is however a regression in this patch for llvm, since it reverts > the llvm expectation that __GNUC_STDC_INLINE__ or __GNUC_GNU_INLINE__ > definition imply proper extern inline semantics. > > If we don't want this fixed in 2.20 at this stage, I'd like to know if > we can backport to the 2.20 branch after release. > > Thanks, > Siddhesh > > 2014-08-13 Siddhesh Poyarekar <siddhesh@redhat.com> > Jakub Jelinek <jakub@redhat.com> > > * libio/stdio.h: Check definition of __fortify_function > instead of __extern_always_inline to include bits/stdio2.h. > * math/bits/math-finite.h [__USE_XOPEN || __USE_ISOC99]: Also > check if __extern_always_inline is defined. > [__USE_MISC || __USE_XOPEN]: Likewise. > [__USE_ISOC99] Likewise. > * misc/sys/cdefs.h (__fortify_function): Define only if > __extern_always_inline is defined. > [!__cplusplus || __GNUC_PREREQ (4,3)]: Revert to defining > __extern_always_inline and __extern_inline only for g++-4.3 > and newer or a compatible gcc. > > diff --git a/libio/stdio.h b/libio/stdio.h > index d8c0bdb..1f4f837 100644 > --- a/libio/stdio.h > +++ b/libio/stdio.h > @@ -932,7 +932,7 @@ extern void funlockfile (FILE *__stream) __THROW; > #ifdef __USE_EXTERN_INLINES > # include <bits/stdio.h> > #endif > -#if __USE_FORTIFY_LEVEL > 0 && defined __extern_always_inline > +#if __USE_FORTIFY_LEVEL > 0 && defined __fortify_function > # include <bits/stdio2.h> > #endif > #ifdef __LDBL_COMPAT > diff --git a/math/bits/math-finite.h b/math/bits/math-finite.h > index aa755de..0656645 100644 > --- a/math/bits/math-finite.h > +++ b/math/bits/math-finite.h > @@ -251,7 +251,8 @@ extern long double __REDIRECT_NTH (lgammal_r, (long double, int *), > # endif > #endif > > -#if defined __USE_XOPEN || defined __USE_ISOC99 > +#if ((defined __USE_XOPEN || defined __USE_ISOC99) \ > + && defined __extern_always_inline) > /* lgamma. */ > __extern_always_inline double __NTH (lgamma (double __d)) > { > @@ -284,7 +285,8 @@ __extern_always_inline long double __NTH (lgammal (long double __d)) > # endif > #endif > > -#if defined __USE_MISC || defined __USE_XOPEN > +#if ((defined __USE_MISC || defined __USE_XOPEN) \ > + && defined __extern_always_inline) > /* gamma. */ > __extern_always_inline double __NTH (gamma (double __d)) > { > @@ -422,7 +424,7 @@ extern long double __REDIRECT_NTH (sqrtl, (long double), __sqrtl_finite); > # endif > #endif > > -#ifdef __USE_ISOC99 > +#if defined __USE_ISOC99 && defined __extern_always_inline > /* tgamma. */ > extern double __gamma_r_finite (double, int *); > __extern_always_inline double __NTH (tgamma (double __d)) > diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h > index 04db956..d8ee73c 100644 > --- a/misc/sys/cdefs.h > +++ b/misc/sys/cdefs.h > @@ -131,7 +131,6 @@ > /* Fortify support. */ > #define __bos(ptr) __builtin_object_size (ptr, __USE_FORTIFY_LEVEL > 1) > #define __bos0(ptr) __builtin_object_size (ptr, 0) > -#define __fortify_function __extern_always_inline __attribute_artificial__ > > #if __GNUC_PREREQ (4,3) > # define __warndecl(name, msg) \ > @@ -318,12 +317,10 @@ > # define __attribute_artificial__ /* Ignore */ > #endif > > -#ifdef __GNUC__ > -/* One of these will be defined if the __gnu_inline__ attribute is > - available. In C++, __GNUC_GNU_INLINE__ will be defined even though > - __inline does not use the GNU inlining rules. If neither macro is > - defined, this version of GCC only supports GNU inline semantics. */ > -# if defined __GNUC_STDC_INLINE__ || defined __GNUC_GNU_INLINE__ > +/* 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) > +# if defined __GNUC_STDC_INLINE__ || defined __cplusplus > # define __extern_inline extern __inline __attribute__ ((__gnu_inline__)) > # define __extern_always_inline \ > extern __always_inline __attribute__ ((__gnu_inline__)) > @@ -331,9 +328,10 @@ > # define __extern_inline extern __inline > # define __extern_always_inline extern __always_inline > # endif > -#else /* Not GCC. */ > -# define __extern_inline /* Ignore */ > -# define __extern_always_inline /* Ignore */ > +#endif > + > +#ifdef __extern_always_inline > +# define __fortify_function __extern_always_inline __attribute_artificial__ > #endif > > /* GCC 4.3 and above allow passing all anonymous arguments of an > -- > 1.9.3 >
Ping! On Mon, Sep 01, 2014 at 10:53:06PM +0530, Siddhesh Poyarekar wrote: > Ping, is this patch OK? I don't want to necessarily commit it in > 2.20, but it would be nice to keep it in queue for commit and maybe > backport it to 2.20 later. > > Siddhesh > > On Wed, Aug 13, 2014 at 11:53:35PM +0530, Siddhesh Poyarekar wrote: > > Hi, > > > > The check for only __GNUC_STDC_INLINE__ and __GNUC_GNU_INLINE__ may > > not be sufficient since those flags were added during initial support > > for C99 inlining semantics. There is also a problem with always > > defining __extern_inline and __extern_always_inline, since it enables > > inline wrapper functions even when GNU inlining semantics are not > > guaranteed. This, along with the possibility of such wrappers using > > redirection (btowc for example) could result in compiler generating an > > infinitely recusrive call to the function. > > > > In fact it was such a recursion that led to this code being written > > the way it was; see: > > > > https://bugzilla.redhat.com/show_bug.cgi?id=186410 > > > > The initial change: > > > > commit b7bfe116e6304da848759b69a6d713da3e93e936 > > Author: Marek Polacek <polacek@redhat.com> > > Date: Wed Sep 26 12:58:36 2012 +0200 > > > > Fix up definitions for older compilers. > > > > was to fix bugs 14530 and 13741, but they can be resolved by checking > > if __fortify_function and/or __extern_always_inline are defined, as it > > has been done in this patch. > > > > I have tested this with gcc 3.2, 3.4 and 4.1. In addition, I have > > done a quick audit of uses of __extern_always_inline and > > __extern_inline to make sure that none of the uses can result in > > compilation errors. > > > > There is however a regression in this patch for llvm, since it reverts > > the llvm expectation that __GNUC_STDC_INLINE__ or __GNUC_GNU_INLINE__ > > definition imply proper extern inline semantics. > > > > If we don't want this fixed in 2.20 at this stage, I'd like to know if > > we can backport to the 2.20 branch after release. > > > > Thanks, > > Siddhesh > > > > 2014-08-13 Siddhesh Poyarekar <siddhesh@redhat.com> > > Jakub Jelinek <jakub@redhat.com> > > > > * libio/stdio.h: Check definition of __fortify_function > > instead of __extern_always_inline to include bits/stdio2.h. > > * math/bits/math-finite.h [__USE_XOPEN || __USE_ISOC99]: Also > > check if __extern_always_inline is defined. > > [__USE_MISC || __USE_XOPEN]: Likewise. > > [__USE_ISOC99] Likewise. > > * misc/sys/cdefs.h (__fortify_function): Define only if > > __extern_always_inline is defined. > > [!__cplusplus || __GNUC_PREREQ (4,3)]: Revert to defining > > __extern_always_inline and __extern_inline only for g++-4.3 > > and newer or a compatible gcc. > > > > diff --git a/libio/stdio.h b/libio/stdio.h > > index d8c0bdb..1f4f837 100644 > > --- a/libio/stdio.h > > +++ b/libio/stdio.h > > @@ -932,7 +932,7 @@ extern void funlockfile (FILE *__stream) __THROW; > > #ifdef __USE_EXTERN_INLINES > > # include <bits/stdio.h> > > #endif > > -#if __USE_FORTIFY_LEVEL > 0 && defined __extern_always_inline > > +#if __USE_FORTIFY_LEVEL > 0 && defined __fortify_function > > # include <bits/stdio2.h> > > #endif > > #ifdef __LDBL_COMPAT > > diff --git a/math/bits/math-finite.h b/math/bits/math-finite.h > > index aa755de..0656645 100644 > > --- a/math/bits/math-finite.h > > +++ b/math/bits/math-finite.h > > @@ -251,7 +251,8 @@ extern long double __REDIRECT_NTH (lgammal_r, (long double, int *), > > # endif > > #endif > > > > -#if defined __USE_XOPEN || defined __USE_ISOC99 > > +#if ((defined __USE_XOPEN || defined __USE_ISOC99) \ > > + && defined __extern_always_inline) > > /* lgamma. */ > > __extern_always_inline double __NTH (lgamma (double __d)) > > { > > @@ -284,7 +285,8 @@ __extern_always_inline long double __NTH (lgammal (long double __d)) > > # endif > > #endif > > > > -#if defined __USE_MISC || defined __USE_XOPEN > > +#if ((defined __USE_MISC || defined __USE_XOPEN) \ > > + && defined __extern_always_inline) > > /* gamma. */ > > __extern_always_inline double __NTH (gamma (double __d)) > > { > > @@ -422,7 +424,7 @@ extern long double __REDIRECT_NTH (sqrtl, (long double), __sqrtl_finite); > > # endif > > #endif > > > > -#ifdef __USE_ISOC99 > > +#if defined __USE_ISOC99 && defined __extern_always_inline > > /* tgamma. */ > > extern double __gamma_r_finite (double, int *); > > __extern_always_inline double __NTH (tgamma (double __d)) > > diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h > > index 04db956..d8ee73c 100644 > > --- a/misc/sys/cdefs.h > > +++ b/misc/sys/cdefs.h > > @@ -131,7 +131,6 @@ > > /* Fortify support. */ > > #define __bos(ptr) __builtin_object_size (ptr, __USE_FORTIFY_LEVEL > 1) > > #define __bos0(ptr) __builtin_object_size (ptr, 0) > > -#define __fortify_function __extern_always_inline __attribute_artificial__ > > > > #if __GNUC_PREREQ (4,3) > > # define __warndecl(name, msg) \ > > @@ -318,12 +317,10 @@ > > # define __attribute_artificial__ /* Ignore */ > > #endif > > > > -#ifdef __GNUC__ > > -/* One of these will be defined if the __gnu_inline__ attribute is > > - available. In C++, __GNUC_GNU_INLINE__ will be defined even though > > - __inline does not use the GNU inlining rules. If neither macro is > > - defined, this version of GCC only supports GNU inline semantics. */ > > -# if defined __GNUC_STDC_INLINE__ || defined __GNUC_GNU_INLINE__ > > +/* 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) > > +# if defined __GNUC_STDC_INLINE__ || defined __cplusplus > > # define __extern_inline extern __inline __attribute__ ((__gnu_inline__)) > > # define __extern_always_inline \ > > extern __always_inline __attribute__ ((__gnu_inline__)) > > @@ -331,9 +328,10 @@ > > # define __extern_inline extern __inline > > # define __extern_always_inline extern __always_inline > > # endif > > -#else /* Not GCC. */ > > -# define __extern_inline /* Ignore */ > > -# define __extern_always_inline /* Ignore */ > > +#endif > > + > > +#ifdef __extern_always_inline > > +# define __fortify_function __extern_always_inline __attribute_artificial__ > > #endif > > > > /* GCC 4.3 and above allow passing all anonymous arguments of an > > -- > > 1.9.3 > > > >
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 08/13/2014 02:23 PM, Siddhesh Poyarekar wrote: > Hi, > > The check for only __GNUC_STDC_INLINE__ and __GNUC_GNU_INLINE__ may > not be sufficient since those flags were added during initial support > for C99 inlining semantics. There is also a problem with always > defining __extern_inline and __extern_always_inline, since it enables > inline wrapper functions even when GNU inlining semantics are not > guaranteed. This, along with the possibility of such wrappers using > redirection (btowc for example) could result in compiler generating an > infinitely recusrive call to the function. > > In fact it was such a recursion that led to this code being written > the way it was; see: > > https://bugzilla.redhat.com/show_bug.cgi?id=186410 > > The initial change: > > commit b7bfe116e6304da848759b69a6d713da3e93e936 > Author: Marek Polacek <polacek@redhat.com> > Date: Wed Sep 26 12:58:36 2012 +0200 > > Fix up definitions for older compilers. > > was to fix bugs 14530 and 13741, but they can be resolved by checking > if __fortify_function and/or __extern_always_inline are defined, as it > has been done in this patch. > > I have tested this with gcc 3.2, 3.4 and 4.1. In addition, I have > done a quick audit of uses of __extern_always_inline and > __extern_inline to make sure that none of the uses can result in > compilation errors. > > There is however a regression in this patch for llvm, since it reverts > the llvm expectation that __GNUC_STDC_INLINE__ or __GNUC_GNU_INLINE__ > definition imply proper extern inline semantics. > > If we don't want this fixed in 2.20 at this stage, I'd like to know if > we can backport to the 2.20 branch after release. I think this *should* be backported to 2.20. OK for master with one comment expansion and [BZ #17266] in ChangeLog. > 2014-08-13 Siddhesh Poyarekar <siddhesh@redhat.com> > Jakub Jelinek <jakub@redhat.com> > > * libio/stdio.h: Check definition of __fortify_function > instead of __extern_always_inline to include bits/stdio2.h. > * math/bits/math-finite.h [__USE_XOPEN || __USE_ISOC99]: Also > check if __extern_always_inline is defined. > [__USE_MISC || __USE_XOPEN]: Likewise. > [__USE_ISOC99] Likewise. > * misc/sys/cdefs.h (__fortify_function): Define only if > __extern_always_inline is defined. > [!__cplusplus || __GNUC_PREREQ (4,3)]: Revert to defining > __extern_always_inline and __extern_inline only for g++-4.3 > and newer or a compatible gcc. > > diff --git a/libio/stdio.h b/libio/stdio.h > index d8c0bdb..1f4f837 100644 > --- a/libio/stdio.h > +++ b/libio/stdio.h > @@ -932,7 +932,7 @@ extern void funlockfile (FILE *__stream) __THROW; > #ifdef __USE_EXTERN_INLINES > # include <bits/stdio.h> > #endif > -#if __USE_FORTIFY_LEVEL > 0 && defined __extern_always_inline > +#if __USE_FORTIFY_LEVEL > 0 && defined __fortify_function OK. > # include <bits/stdio2.h> > #endif > #ifdef __LDBL_COMPAT > diff --git a/math/bits/math-finite.h b/math/bits/math-finite.h > index aa755de..0656645 100644 > --- a/math/bits/math-finite.h > +++ b/math/bits/math-finite.h > @@ -251,7 +251,8 @@ extern long double __REDIRECT_NTH (lgammal_r, (long double, int *), > # endif > #endif > > -#if defined __USE_XOPEN || defined __USE_ISOC99 > +#if ((defined __USE_XOPEN || defined __USE_ISOC99) \ > + && defined __extern_always_inline) OK. > /* lgamma. */ > __extern_always_inline double __NTH (lgamma (double __d)) > { > @@ -284,7 +285,8 @@ __extern_always_inline long double __NTH (lgammal (long double __d)) > # endif > #endif > > -#if defined __USE_MISC || defined __USE_XOPEN > +#if ((defined __USE_MISC || defined __USE_XOPEN) \ > + && defined __extern_always_inline) OK. > /* gamma. */ > __extern_always_inline double __NTH (gamma (double __d)) > { > @@ -422,7 +424,7 @@ extern long double __REDIRECT_NTH (sqrtl, (long double), __sqrtl_finite); > # endif > #endif > > -#ifdef __USE_ISOC99 > +#if defined __USE_ISOC99 && defined __extern_always_inline OK. > /* tgamma. */ > extern double __gamma_r_finite (double, int *); > __extern_always_inline double __NTH (tgamma (double __d)) > diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h > index 04db956..d8ee73c 100644 > --- a/misc/sys/cdefs.h > +++ b/misc/sys/cdefs.h > @@ -131,7 +131,6 @@ > /* Fortify support. */ > #define __bos(ptr) __builtin_object_size (ptr, __USE_FORTIFY_LEVEL > 1) > #define __bos0(ptr) __builtin_object_size (ptr, 0) > -#define __fortify_function __extern_always_inline __attribute_artificial__ OK. > > #if __GNUC_PREREQ (4,3) > # define __warndecl(name, msg) \ > @@ -318,12 +317,10 @@ > # define __attribute_artificial__ /* Ignore */ > #endif > > -#ifdef __GNUC__ > -/* One of these will be defined if the __gnu_inline__ attribute is > - available. In C++, __GNUC_GNU_INLINE__ will be defined even though > - __inline does not use the GNU inlining rules. If neither macro is > - defined, this version of GCC only supports GNU inline semantics. */ > -# if defined __GNUC_STDC_INLINE__ || defined __GNUC_GNU_INLINE__ > +/* GCC 4.3 and above with -std=c99 or -std=gnu99 implements ISO C99 > + inline semantics, unless -fgnu89-inline is used. */ Please expand this comment to say that using just __GNUC_STDC_INLINE__ and __GNUC_GNU_INLINE__ is not a correct solution. > +#if !defined __cplusplus || __GNUC_PREREQ (4,3) > +# if defined __GNUC_STDC_INLINE__ || defined __cplusplus OK. > # define __extern_inline extern __inline __attribute__ ((__gnu_inline__)) > # define __extern_always_inline \ > extern __always_inline __attribute__ ((__gnu_inline__)) > @@ -331,9 +328,10 @@ > # define __extern_inline extern __inline > # define __extern_always_inline extern __always_inline > # endif > -#else /* Not GCC. */ > -# define __extern_inline /* Ignore */ > -# define __extern_always_inline /* Ignore */ > +#endif > + > +#ifdef __extern_always_inline > +# define __fortify_function __extern_always_inline __attribute_artificial__ OK. > #endif > > /* GCC 4.3 and above allow passing all anonymous arguments of an > Marek's patch was a good attempt at cleaning up the semantics of the rules that the glibc headers apply, but it reinforces that the rules are hard to remember and keep straight without good documentation and continued testing with older compiler. Cheers, Carlos. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJUF6WfAAoJECXvCkNsKkr/PxwIANOHzbcr3aPEKBZRnLSe0mke ogZtM5cjvHuPwhStaxqeX5sS+7BeWQ5GSNN4hZ/JrPHNao6R1hlQBfP6Oaf52Wbb vru2PN9nuVKlgT4l9txSoKE7EB5VMBidTDqJ6ahFTgP1MAOZwncgOXXyRNZQ4sCG qpjs3Yq1n0ImSZ5HcNckyW4YYhe5s6NMgz6yEOZf2fFppQgIUCq0q0XjY6VQsK/m WS7loSeowz0A5T3y67WlNKMAKHIjekt7d8zlMQciZlM8+m+Fo28Hp/1cT0799yuZ SMFbkmxDeDMI6xHkphS2hF6HeYh5Y6of1Wch38BPGCifdIpTr9Y72Ibo+BxrObI= =tpsG -----END PGP SIGNATURE-----
On 16/09/14 12:51, Carlos O'Donell wrote: > On 08/13/2014 02:23 PM, Siddhesh Poyarekar wrote: >> Hi, > >> The check for only __GNUC_STDC_INLINE__ and __GNUC_GNU_INLINE__ may >> not be sufficient since those flags were added during initial support >> for C99 inlining semantics. There is also a problem with always >> defining __extern_inline and __extern_always_inline, since it enables >> inline wrapper functions even when GNU inlining semantics are not >> guaranteed. This, along with the possibility of such wrappers using >> redirection (btowc for example) could result in compiler generating an >> infinitely recusrive call to the function. > >> In fact it was such a recursion that led to this code being written >> the way it was; see: > >> https://bugzilla.redhat.com/show_bug.cgi?id=186410 > >> The initial change: > >> commit b7bfe116e6304da848759b69a6d713da3e93e936 >> Author: Marek Polacek <polacek@redhat.com> >> Date: Wed Sep 26 12:58:36 2012 +0200 > >> Fix up definitions for older compilers. > >> was to fix bugs 14530 and 13741, but they can be resolved by checking >> if __fortify_function and/or __extern_always_inline are defined, as it >> has been done in this patch. > >> I have tested this with gcc 3.2, 3.4 and 4.1. In addition, I have >> done a quick audit of uses of __extern_always_inline and >> __extern_inline to make sure that none of the uses can result in >> compilation errors. > >> There is however a regression in this patch for llvm, since it reverts >> the llvm expectation that __GNUC_STDC_INLINE__ or __GNUC_GNU_INLINE__ >> definition imply proper extern inline semantics. > >> If we don't want this fixed in 2.20 at this stage, I'd like to know if >> we can backport to the 2.20 branch after release. > > I think this *should* be backported to 2.20. > > OK for master with one comment expansion and [BZ #17266] in ChangeLog. > I agree. Either give me a ping once it is committed or go ahead with the backport yourself. Allan
On Tue, Sep 16, 2014 at 01:39:39PM +1000, Allan McRae wrote: > I agree. Either give me a ping once it is committed or go ahead with > the backport yourself. Pushed in master and 2.20. Thanks, Siddhesh
Siddhesh Poyarekar <siddhesh@redhat.com> writes: > On Tue, Sep 16, 2014 at 01:39:39PM +1000, Allan McRae wrote: >> I agree. Either give me a ping once it is committed or go ahead with >> the backport yourself. > > Pushed in master and 2.20. When you cherry-pick a commit please use -x. Andreas.
On Tue, Sep 16, 2014 at 11:17:38AM +0200, Andreas Schwab wrote:
> When you cherry-pick a commit please use -x.
Ack, I'll keep that in mind next time.
Siddhesh
diff --git a/libio/stdio.h b/libio/stdio.h index d8c0bdb..1f4f837 100644 --- a/libio/stdio.h +++ b/libio/stdio.h @@ -932,7 +932,7 @@ extern void funlockfile (FILE *__stream) __THROW; #ifdef __USE_EXTERN_INLINES # include <bits/stdio.h> #endif -#if __USE_FORTIFY_LEVEL > 0 && defined __extern_always_inline +#if __USE_FORTIFY_LEVEL > 0 && defined __fortify_function # include <bits/stdio2.h> #endif #ifdef __LDBL_COMPAT diff --git a/math/bits/math-finite.h b/math/bits/math-finite.h index aa755de..0656645 100644 --- a/math/bits/math-finite.h +++ b/math/bits/math-finite.h @@ -251,7 +251,8 @@ extern long double __REDIRECT_NTH (lgammal_r, (long double, int *), # endif #endif -#if defined __USE_XOPEN || defined __USE_ISOC99 +#if ((defined __USE_XOPEN || defined __USE_ISOC99) \ + && defined __extern_always_inline) /* lgamma. */ __extern_always_inline double __NTH (lgamma (double __d)) { @@ -284,7 +285,8 @@ __extern_always_inline long double __NTH (lgammal (long double __d)) # endif #endif -#if defined __USE_MISC || defined __USE_XOPEN +#if ((defined __USE_MISC || defined __USE_XOPEN) \ + && defined __extern_always_inline) /* gamma. */ __extern_always_inline double __NTH (gamma (double __d)) { @@ -422,7 +424,7 @@ extern long double __REDIRECT_NTH (sqrtl, (long double), __sqrtl_finite); # endif #endif -#ifdef __USE_ISOC99 +#if defined __USE_ISOC99 && defined __extern_always_inline /* tgamma. */ extern double __gamma_r_finite (double, int *); __extern_always_inline double __NTH (tgamma (double __d)) diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h index 04db956..d8ee73c 100644 --- a/misc/sys/cdefs.h +++ b/misc/sys/cdefs.h @@ -131,7 +131,6 @@ /* Fortify support. */ #define __bos(ptr) __builtin_object_size (ptr, __USE_FORTIFY_LEVEL > 1) #define __bos0(ptr) __builtin_object_size (ptr, 0) -#define __fortify_function __extern_always_inline __attribute_artificial__ #if __GNUC_PREREQ (4,3) # define __warndecl(name, msg) \ @@ -318,12 +317,10 @@ # define __attribute_artificial__ /* Ignore */ #endif -#ifdef __GNUC__ -/* One of these will be defined if the __gnu_inline__ attribute is - available. In C++, __GNUC_GNU_INLINE__ will be defined even though - __inline does not use the GNU inlining rules. If neither macro is - defined, this version of GCC only supports GNU inline semantics. */ -# if defined __GNUC_STDC_INLINE__ || defined __GNUC_GNU_INLINE__ +/* 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) +# if defined __GNUC_STDC_INLINE__ || defined __cplusplus # define __extern_inline extern __inline __attribute__ ((__gnu_inline__)) # define __extern_always_inline \ extern __always_inline __attribute__ ((__gnu_inline__)) @@ -331,9 +328,10 @@ # define __extern_inline extern __inline # define __extern_always_inline extern __always_inline # endif -#else /* Not GCC. */ -# define __extern_inline /* Ignore */ -# define __extern_always_inline /* Ignore */ +#endif + +#ifdef __extern_always_inline +# define __fortify_function __extern_always_inline __attribute_artificial__ #endif /* GCC 4.3 and above allow passing all anonymous arguments of an
Hi, The check for only __GNUC_STDC_INLINE__ and __GNUC_GNU_INLINE__ may not be sufficient since those flags were added during initial support for C99 inlining semantics. There is also a problem with always defining __extern_inline and __extern_always_inline, since it enables inline wrapper functions even when GNU inlining semantics are not guaranteed. This, along with the possibility of such wrappers using redirection (btowc for example) could result in compiler generating an infinitely recusrive call to the function. In fact it was such a recursion that led to this code being written the way it was; see: https://bugzilla.redhat.com/show_bug.cgi?id=186410 The initial change: commit b7bfe116e6304da848759b69a6d713da3e93e936 Author: Marek Polacek <polacek@redhat.com> Date: Wed Sep 26 12:58:36 2012 +0200 Fix up definitions for older compilers. was to fix bugs 14530 and 13741, but they can be resolved by checking if __fortify_function and/or __extern_always_inline are defined, as it has been done in this patch. I have tested this with gcc 3.2, 3.4 and 4.1. In addition, I have done a quick audit of uses of __extern_always_inline and __extern_inline to make sure that none of the uses can result in compilation errors. There is however a regression in this patch for llvm, since it reverts the llvm expectation that __GNUC_STDC_INLINE__ or __GNUC_GNU_INLINE__ definition imply proper extern inline semantics. If we don't want this fixed in 2.20 at this stage, I'd like to know if we can backport to the 2.20 branch after release. Thanks, Siddhesh 2014-08-13 Siddhesh Poyarekar <siddhesh@redhat.com> Jakub Jelinek <jakub@redhat.com> * libio/stdio.h: Check definition of __fortify_function instead of __extern_always_inline to include bits/stdio2.h. * math/bits/math-finite.h [__USE_XOPEN || __USE_ISOC99]: Also check if __extern_always_inline is defined. [__USE_MISC || __USE_XOPEN]: Likewise. [__USE_ISOC99] Likewise. * misc/sys/cdefs.h (__fortify_function): Define only if __extern_always_inline is defined. [!__cplusplus || __GNUC_PREREQ (4,3)]: Revert to defining __extern_always_inline and __extern_inline only for g++-4.3 and newer or a compatible gcc.