diff mbox series

[PATCH-for-9.0?,v2,4/4] meson: Enable -Wstatic-in-inline

Message ID 20240326171009.26696-5-philmd@linaro.org
State New
Headers show
Series overall: Avoid using inlined functions with external linkage again | expand

Commit Message

Philippe Mathieu-Daudé March 26, 2024, 5:10 p.m. UTC 
Compilers are clever enough to inline code when necessary.

The only case we accept an inline function is static in
header (we use C, not C++).

Add the -Wstatic-in-inline CPPFLAG to prevent public and
inline function to be added in the code base.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20240313184954.42513-5-philmd@linaro.org>
---
 meson.build | 1 +
 1 file changed, 1 insertion(+)

Comments

Thomas Huth March 26, 2024, 5:28 p.m. UTC | #1 
On 26/03/2024 18.10, Philippe Mathieu-Daudé wrote:
> Compilers are clever enough to inline code when necessary.
> 
> The only case we accept an inline function is static in
> header (we use C, not C++).
> 
> Add the -Wstatic-in-inline CPPFLAG to prevent public and

I think this is rather a compiler than a pre-processor flag, so 
s/CPPFLAG/CFLAGS/ ?

Anyway:
Reviewed-by: Thomas Huth <thuth@redhat.com>


> inline function to be added in the code base.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Message-Id: <20240313184954.42513-5-philmd@linaro.org>
> ---
>   meson.build | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/meson.build b/meson.build
> index c9c3217ba4..f400f7d36c 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -591,6 +591,7 @@ warn_flags = [
>     '-Wold-style-definition',
>     '-Wredundant-decls',
>     '-Wshadow=local',
> +  '-Wstatic-in-inline',
>     '-Wstrict-prototypes',
>     '-Wtype-limits',
>     '-Wundef',
Philippe Mathieu-Daudé March 27, 2024, 7:49 a.m. UTC | #2 
On 26/3/24 18:28, Thomas Huth wrote:
> On 26/03/2024 18.10, Philippe Mathieu-Daudé wrote:
>> Compilers are clever enough to inline code when necessary.
>>
>> The only case we accept an inline function is static in
>> header (we use C, not C++).
>>
>> Add the -Wstatic-in-inline CPPFLAG to prevent public and
> 
> I think this is rather a compiler than a pre-processor flag, so 
> s/CPPFLAG/CFLAGS/ ?

Oops indeed you are right, thanks!

> 
> Anyway:
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
> 
>> inline function to be added in the code base.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> Message-Id: <20240313184954.42513-5-philmd@linaro.org>
>> ---
>>   meson.build | 1 +
>>   1 file changed, 1 insertion(+)
Paolo Bonzini March 27, 2024, 9:26 a.m. UTC | #3 
On 3/26/24 18:10, Philippe Mathieu-Daudé wrote:
> Compilers are clever enough to inline code when necessary.
> 
> The only case we accept an inline function is static in
> header (we use C, not C++).
> 
> Add the -Wstatic-in-inline CPPFLAG to prevent public and
> inline function to be added in the code base.

No problem with the first three patches, but -Wstatic-in-inline is not
warning for non-static inline functions.  The warning is enabled by
default by GCC (which has no way to disable it even) and by clang
outside headers:

f.h:
static int y;

inline int f()
{
     return y;
}

f.c:
#include "f.h"

int main()
{
}

$ clang f.c
./f.h:5:12: warning: static variable 'y' is used in an inline function with external linkage [-Wstatic-in-inline]

$ gcc f.c
f.h:5:12: warning: ‘y’ is static but used in inline function ‘f’ which is not static

The actual effect of this patch is to enable the warning on clang *even
outside headers* (clang only enables the warning in headers by default
because, if a static variable belongs to the main source file, it has a
single definition anyway unlike if it's defined in an included file).

For now I'm queuing patches 1-3 only.

Paolo

> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Message-Id: <20240313184954.42513-5-philmd@linaro.org>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>   meson.build | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/meson.build b/meson.build
> index c9c3217ba4..f400f7d36c 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -591,6 +591,7 @@ warn_flags = [
>     '-Wold-style-definition',
>     '-Wredundant-decls',
>     '-Wshadow=local',
> +  '-Wstatic-in-inline',
>     '-Wstrict-prototypes',
>     '-Wtype-limits',
>     '-Wundef',
Philippe Mathieu-Daudé March 27, 2024, 12:42 p.m. UTC | #4 
On 27/3/24 10:26, Paolo Bonzini wrote:
> On 3/26/24 18:10, Philippe Mathieu-Daudé wrote:
>> Compilers are clever enough to inline code when necessary.
>>
>> The only case we accept an inline function is static in
>> header (we use C, not C++).
>>
>> Add the -Wstatic-in-inline CPPFLAG to prevent public and
>> inline function to be added in the code base.
> 
> No problem with the first three patches, but -Wstatic-in-inline is not
> warning for non-static inline functions.  The warning is enabled by
> default by GCC (which has no way to disable it even) and by clang
> outside headers:
> 
> f.h:
> static int y;
> 
> inline int f()
> {
>      return y;
> }
> 
> f.c:
> #include "f.h"
> 
> int main()
> {
> }
> 
> $ clang f.c
> ./f.h:5:12: warning: static variable 'y' is used in an inline function 
> with external linkage [-Wstatic-in-inline]
> 
> $ gcc f.c
> f.h:5:12: warning: ‘y’ is static but used in inline function ‘f’ which 
> is not static
> 
> The actual effect of this patch is to enable the warning on clang *even
> outside headers* (clang only enables the warning in headers by default
> because, if a static variable belongs to the main source file, it has a
> single definition anyway unlike if it's defined in an included file).

IIUC your comment, you are worried about system headers declaring
non-static inline functions?

> 
> For now I'm queuing patches 1-3 only.
> 
> Paolo
> 
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> Message-Id: <20240313184954.42513-5-philmd@linaro.org>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   meson.build | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/meson.build b/meson.build
>> index c9c3217ba4..f400f7d36c 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -591,6 +591,7 @@ warn_flags = [
>>     '-Wold-style-definition',
>>     '-Wredundant-decls',
>>     '-Wshadow=local',
>> +  '-Wstatic-in-inline',
>>     '-Wstrict-prototypes',
>>     '-Wtype-limits',
>>     '-Wundef',
>
Paolo Bonzini March 27, 2024, 2:12 p.m. UTC | #5 
Il mer 27 mar 2024, 13:42 Philippe Mathieu-Daudé <philmd@linaro.org> ha
scritto:

> IIUC your comment, you are worried about system headers declaring
> non-static inline functions?
>

No, it's just that the flag (and thus the patch) is not doing what the
commit message says.

Perhaps you could instead add a checkpatch test that catches occurrences of
inline without static.

Paolo


> >
> > For now I'm queuing patches 1-3 only.
> >
> > Paolo
> >
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> >> Message-Id: <20240313184954.42513-5-philmd@linaro.org>
> >> Reviewed-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >>   meson.build | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/meson.build b/meson.build
> >> index c9c3217ba4..f400f7d36c 100644
> >> --- a/meson.build
> >> +++ b/meson.build
> >> @@ -591,6 +591,7 @@ warn_flags = [
> >>     '-Wold-style-definition',
> >>     '-Wredundant-decls',
> >>     '-Wshadow=local',
> >> +  '-Wstatic-in-inline',
> >>     '-Wstrict-prototypes',
> >>     '-Wtype-limits',
> >>     '-Wundef',
> >
>
>
diff mbox series

Patch

diff --git a/meson.build b/meson.build
index c9c3217ba4..f400f7d36c 100644
--- a/meson.build
+++ b/meson.build
@@ -591,6 +591,7 @@  warn_flags = [
   '-Wold-style-definition',
   '-Wredundant-decls',
   '-Wshadow=local',
+  '-Wstatic-in-inline',
   '-Wstrict-prototypes',
   '-Wtype-limits',
   '-Wundef',