Message ID | HE1PR07MB09054F1EE94A5C0E57E41AAEE4A00@HE1PR07MB0905.eurprd07.prod.outlook.com |
---|---|
State | New |
Headers | show |
On Fri, Feb 19, 2016 at 11:08:48AM +0000, Bernd Edlinger wrote: > On 19.02.2016 11:56, Jakub Jelinek wrote: > > > > On Fri, Feb 19, 2016 at 10:50:34AM +0000, Bernd Edlinger wrote: > > > While I think that we should probably not define __GNUC_GNU_INLINE__ at all for C++, > > > because it is meaningless, I am warned that this could break (already broken) header files. > > > > It is not meaningless. The various headers need to know if it is safe to > > use the gnu_inline attribute in C++. > > > > In any case, the desirable state is that e.g. the -E -dD output should be > > identical if you explicitly request the default -std= version vs. if it is > > set implicitly. We should verify it is the case even for C. > > > > Jakub > > I absolutely agree with you. > The correct solution is probably doing this: > > --- gcc/cp/cfns.h.jj 2016-01-04 15:30:50.000000000 +0100 > +++ gcc/cp/cfns.h 2016-02-19 12:00:15.730375049 +0100 > @@ -124,9 +124,6 @@ > > #ifdef __GNUC__ > __inline > -#ifdef __GNUC_STDC_INLINE__ > -__attribute__ ((__gnu_inline__)) > -#endif > #endif > const char * > libc_name_p (register const char *str, register unsigned int len) This is of course wrong. cfns.h is a generated header, so you shouldn't patch it. If it is regenerated with a newer gperf (I have 3.0.4 installed), you get there: @@ -124,7 +124,7 @@ hash (register const char *str, register #ifdef __GNUC__ __inline -#ifdef __GNUC_STDC_INLINE__ +#if defined __GNUC_STDC_INLINE__ || defined __GNUC_GNU_INLINE__ __attribute__ ((__gnu_inline__)) #endif #endif Jakub
On 19.02.2016 12:31, Jakub Jelinek wrote: > On Fri, Feb 19, 2016 at 11:08:48AM +0000, Bernd Edlinger wrote: > > On 19.02.2016 11:56, Jakub Jelinek wrote: > > > > > > On Fri, Feb 19, 2016 at 10:50:34AM +0000, Bernd Edlinger wrote: > > > > While I think that we should probably not define __GNUC_GNU_INLINE__ at all for C++, > > > > because it is meaningless, I am warned that this could break (already broken) header files. > > > > > > It is not meaningless. The various headers need to know if it is safe to > > > use the gnu_inline attribute in C++. > > > > > > In any case, the desirable state is that e.g. the -E -dD output should be > > > identical if you explicitly request the default -std= version vs. if it is > > > set implicitly. We should verify it is the case even for C. > > > > > > Jakub > > > > I absolutely agree with you. > > The correct solution is probably doing this: > > > > --- gcc/cp/cfns.h.jj 2016-01-04 15:30:50.000000000 +0100 > > +++ gcc/cp/cfns.h 2016-02-19 12:00:15.730375049 +0100 > > @@ -124,9 +124,6 @@ > > > > #ifdef __GNUC__ > > __inline > > -#ifdef __GNUC_STDC_INLINE__ > > -__attribute__ ((__gnu_inline__)) > > -#endif > > #endif > > const char * > > libc_name_p (register const char *str, register unsigned int len) > > This is of course wrong. cfns.h is a generated header, so you shouldn't > patch it. > If it is regenerated with a newer gperf (I have 3.0.4 installed), you get there: > @@ -124,7 +124,7 @@ hash (register const char *str, register > > #ifdef __GNUC__ > __inline > -#ifdef __GNUC_STDC_INLINE__ > +#if defined __GNUC_STDC_INLINE__ || defined __GNUC_GNU_INLINE__ > __attribute__ ((__gnu_inline__)) > #endif > #endif > > Jakub Damnit!! I dont know how this file is ever supposed to compile on a C99 compiler. but with this change it does not even compile with -std=c90 any more: cat test.c #include "cfns.h" gcc -std=c90 test.c In file included from test.c:1:0: cfns.gperf:101:1: error: 'gnu_inline' attribute present on 'libc_name_p' cfns.gperf:26:14: error: but not here gcc -std=c99 test.c In file included from test.c:1:0: cfns.gperf:101:1: error: 'gnu_inline' attribute present on 'libc_name_p' cfns.gperf:26:14: error: but not here cfns.gperf: In function 'libc_name_p': cfns.gperf:328:34: warning: implicit declaration of function 'strcmp' [-Wimplicit-function-declaration] So this leaves only one solution, to define neither __GNUC_STDC_INLINE__ nor __GNUC_GNU_INLINE__ for C++, at least the inline and gnu_inline is completely different on C and C++. But I am anxious this will break more things than gperf, and I would like to fix this in a safe way. Bernd.
On Fri, Feb 19, 2016 at 11:53:03AM +0000, Bernd Edlinger wrote: > > #ifdef __GNUC__ > > __inline > > -#ifdef __GNUC_STDC_INLINE__ > > +#if defined __GNUC_STDC_INLINE__ || defined __GNUC_GNU_INLINE__ > > __attribute__ ((__gnu_inline__)) > > #endif > > #endif > > > > Jakub > > Damnit!! > > I dont know how this file is ever supposed to compile on a C99 compiler. > but with this change it does not even compile with -std=c90 any more: > > cat test.c > #include "cfns.h" > > gcc -std=c90 test.c > In file included from test.c:1:0: > cfns.gperf:101:1: error: 'gnu_inline' attribute present on 'libc_name_p' > cfns.gperf:26:14: error: but not here > > gcc -std=c99 test.c > In file included from test.c:1:0: > cfns.gperf:101:1: error: 'gnu_inline' attribute present on 'libc_name_p' > cfns.gperf:26:14: error: but not here > cfns.gperf: In function 'libc_name_p': > cfns.gperf:328:34: warning: implicit declaration of function 'strcmp' [-Wimplicit-function-declaration] > > > So this leaves only one solution, to define neither __GNUC_STDC_INLINE__ Of course not, and that would be the wrong thing to do. The definition spot of libc_name_p comes from gperf itself, the prototype from cfns.gperf, which we can of course adjust. > nor __GNUC_GNU_INLINE__ for C++, at least the inline and gnu_inline > is completely different on C and C++. But I am anxious this will break more > things than gperf, and I would like to fix this in a safe way. IMNSHO we should just keep it consistent with what g++ e.g. 5.x did. Thus, $ g++ -E -dD -xc++ /dev/null -O2 -std=c++98 2>&1 | grep _INLINE_ #define __GNUC_GNU_INLINE__ 1 $ g++ -E -dD -xc++ /dev/null -O2 -std=c++11 2>&1 | grep _INLINE_ #define __GNUC_STDC_INLINE__ 1 $ g++ -E -dD -xc++ /dev/null -O2 -std=c++14 2>&1 | grep _INLINE_ #define __GNUC_STDC_INLINE__ 1 $ g++ -E -dD -xc++ /dev/null -O2 -std=gnu++98 2>&1 | grep _INLINE_ #define __GNUC_GNU_INLINE__ 1 $ g++ -E -dD -xc++ /dev/null -O2 -std=gnu++11 2>&1 | grep _INLINE_ #define __GNUC_STDC_INLINE__ 1 $ g++ -E -dD -xc++ /dev/null -O2 -std=gnu++14 2>&1 | grep _INLINE_ #define __GNUC_STDC_INLINE__ 1 $ g++ -E -dD -xc++ /dev/null -O2 2>&1 | grep _INLINE_ #define __GNUC_GNU_INLINE__ 1 We want to define what we did with the explicit -std= options, and just change the output in the default case (last invocation), to #define __GNUC_STDC_INLINE__ 1 because the default is different. Jakub
On 19.02.2016 12:59, Jakub Jelinek wrote: > > Of course not, and that would be the wrong thing to do. > The definition spot of libc_name_p comes from gperf itself, the prototype > from cfns.gperf, which we can of course adjust. > Yes, now I understand. Thanks. > > IMNSHO we should just keep it consistent with what g++ e.g. 5.x did. >Thus, > $ g++ -E -dD -xc++ /dev/null -O2 -std=c++98 2>&1 | grep _INLINE_ > #define __GNUC_GNU_INLINE__ 1 > $ g++ -E -dD -xc++ /dev/null -O2 -std=c++11 2>&1 | grep _INLINE_ > #define __GNUC_STDC_INLINE__ 1 > $ g++ -E -dD -xc++ /dev/null -O2 -std=c++14 2>&1 | grep _INLINE_ > #define __GNUC_STDC_INLINE__ 1 > $ g++ -E -dD -xc++ /dev/null -O2 -std=gnu++98 2>&1 | grep _INLINE_ > #define __GNUC_GNU_INLINE__ 1 > $ g++ -E -dD -xc++ /dev/null -O2 -std=gnu++11 2>&1 | grep _INLINE_ > #define __GNUC_STDC_INLINE__ 1 > $ g++ -E -dD -xc++ /dev/null -O2 -std=gnu++14 2>&1 | grep _INLINE_ > #define __GNUC_STDC_INLINE__ 1 > $ g++ -E -dD -xc++ /dev/null -O2 2>&1 | grep _INLINE_ > #define __GNUC_GNU_INLINE__ 1 > We want to define what we did with the explicit -std= options, and just > change the output in the default case (last invocation), to > #define __GNUC_STDC_INLINE__ 1 > because the default is different. > > Jakub OK. I can do that. I will send a new patch in the evening. Thanks Bernd.
--- gcc/cp/cfns.h.jj 2016-01-04 15:30:50.000000000 +0100 +++ gcc/cp/cfns.h 2016-02-19 12:00:15.730375049 +0100 @@ -124,9 +124,6 @@ #ifdef __GNUC__ __inline -#ifdef __GNUC_STDC_INLINE__ -__attribute__ ((__gnu_inline__)) -#endif #endif const char * libc_name_p (register const char *str, register unsigned int len)