diff mbox

[00/14] More #include cleanups

Message ID 56BA1F5B.3050903@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eric Blake Feb. 9, 2016, 5:18 p.m. UTC
On 02/09/2016 10:10 AM, Eric Blake wrote:

> but with the new header order it expands to:
> 
> # 1989 "/usr/include/c++/5.3.1/x86_64-redhat-linux/bits/c++config.h" 3
> namespace std
> {
> 
> # 1991 "/usr/include/c++/5.3.1/x86_64-redhat-linux/bits/c++config.h"
>  __attribute__ (( always_inline )) __inline__
> # 1991 "/usr/include/c++/5.3.1/x86_64-redhat-linux/bits/c++config.h" 3
>         namespace __cxx11 __attribute__((__abi_tag__ ("cxx11"))) { }
> }
> 
> So _something_ in osdep.h is redefining 'inline' with disastrous effects
> to C++.

This hack seems to fix things; but I'm not enough of a C++ expert to
know if it is the best fix.

__attribute__((unused));

Comments

Peter Maydell Feb. 9, 2016, 6:04 p.m. UTC | #1
On 9 February 2016 at 17:18, Eric Blake <eblake@redhat.com> wrote:
> On 02/09/2016 10:10 AM, Eric Blake wrote:
>
>> but with the new header order it expands to:
>>
>> # 1989 "/usr/include/c++/5.3.1/x86_64-redhat-linux/bits/c++config.h" 3
>> namespace std
>> {
>>
>> # 1991 "/usr/include/c++/5.3.1/x86_64-redhat-linux/bits/c++config.h"
>>  __attribute__ (( always_inline )) __inline__
>> # 1991 "/usr/include/c++/5.3.1/x86_64-redhat-linux/bits/c++config.h" 3
>>         namespace __cxx11 __attribute__((__abi_tag__ ("cxx11"))) { }
>> }
>>
>> So _something_ in osdep.h is redefining 'inline' with disastrous effects
>> to C++.
>
> This hack seems to fix things; but I'm not enough of a C++ expert to
> know if it is the best fix.
>
> diff --git i/include/qemu/compiler.h w/include/qemu/compiler.h
> index d22eb01..d50b806 100644
> --- i/include/qemu/compiler.h
> +++ w/include/qemu/compiler.h
> @@ -77,6 +77,7 @@
>  #define typeof_field(type, field) typeof(((type *)0)->field)
>  #define type_check(t1,t2) ((t1*)0 - (t2*)0)
>
> +#ifndef __cplusplus
>  #ifndef always_inline
>  #if !((__GNUC__ < 3) || defined(__APPLE__))
>  #ifdef __OPTIMIZE__
> @@ -88,6 +89,7 @@
>  #undef inline
>  #define inline always_inline
>  #endif
> +#endif

Ah, well tracked down. I guess my system headers don't happen
to rely on this bit of inline syntax.

Next question -- do we really need to redefine "inline" to
always_inline at all, in C or C++? (cc'd a few people who might
have an opinion). My instinct is to say "don't do this, just
trust the compiler to decide whether to bother inlining".

Git blame says this was added back in commit df2542c737ea2 in
2007, with the rationale "To avoid discarded inlining bug"...

thanks
-- PMM
Paolo Bonzini Feb. 9, 2016, 6:17 p.m. UTC | #2
On 09/02/2016 19:04, Peter Maydell wrote:
> Next question -- do we really need to redefine "inline" to
> always_inline at all, in C or C++? (cc'd a few people who might
> have an opinion). My instinct is to say "don't do this, just
> trust the compiler to decide whether to bother inlining".

Besides this, it sounds like a thing you'd do in the old -ffixed-reg days...

Paolo
Eric Blake Feb. 9, 2016, 6:29 p.m. UTC | #3
On 02/09/2016 11:04 AM, Peter Maydell wrote:

>>> So _something_ in osdep.h is redefining 'inline' with disastrous effects
>>> to C++.
>>
>> This hack seems to fix things; but I'm not enough of a C++ expert to
>> know if it is the best fix.
>>
>> diff --git i/include/qemu/compiler.h w/include/qemu/compiler.h
>> index d22eb01..d50b806 100644
>> --- i/include/qemu/compiler.h
>> +++ w/include/qemu/compiler.h
>> @@ -77,6 +77,7 @@
>>  #define typeof_field(type, field) typeof(((type *)0)->field)
>>  #define type_check(t1,t2) ((t1*)0 - (t2*)0)
>>
>> +#ifndef __cplusplus
>>  #ifndef always_inline
>>  #if !((__GNUC__ < 3) || defined(__APPLE__))
>>  #ifdef __OPTIMIZE__
>> @@ -88,6 +89,7 @@
>>  #undef inline
>>  #define inline always_inline
>>  #endif
>> +#endif
> 
> Ah, well tracked down. I guess my system headers don't happen
> to rely on this bit of inline syntax.
> 
> Next question -- do we really need to redefine "inline" to
> always_inline at all, in C or C++? (cc'd a few people who might
> have an opinion). My instinct is to say "don't do this, just
> trust the compiler to decide whether to bother inlining".

If ripping that chunk out doesn't hurt anything, I'm all in favor of it.
Redefining 'inline' tends to hurt more than it prevents (and while there
IS the case of C89 vs. C99 vs. GNU having different semantics for
'static inline', those are best worked around by writing and
consistently using wrapper macros such as 'STATIC_INLINE', not
redefining 'inline' itself).

> 
> Git blame says this was added back in commit df2542c737ea2 in
> 2007, with the rationale "To avoid discarded inlining bug"...

The date alone says it may be time to quit doing it :)
diff mbox

Patch

diff --git i/include/qemu/compiler.h w/include/qemu/compiler.h
index d22eb01..d50b806 100644
--- i/include/qemu/compiler.h
+++ w/include/qemu/compiler.h
@@ -77,6 +77,7 @@ 
 #define typeof_field(type, field) typeof(((type *)0)->field)
 #define type_check(t1,t2) ((t1*)0 - (t2*)0)

+#ifndef __cplusplus
 #ifndef always_inline
 #if !((__GNUC__ < 3) || defined(__APPLE__))
 #ifdef __OPTIMIZE__
@@ -88,6 +89,7 @@ 
 #undef inline
 #define inline always_inline
 #endif
+#endif

 #define QEMU_BUILD_BUG_ON(x) \
     typedef char glue(qemu_build_bug_on__,__LINE__)[(x)?-1:1]