[RFC 0/4] meson: Enable -Wundef
A few days ago I have posted a patch[1] that addresses an issue introduced when a meson check was dropped but some uses of the corresponding WITH_ macro were not removed at the same time. That got me thinking about what we can do to prevent such scenarios from happening again in the future. I have come up with something that I think would be effective, but since applying the approach throughout the entire codebase would require a non-trivial amount of work, I figured I'd ask for feedback before embarking on it. The idea is that there are two types of macros we can use for conditional compilation: external ones, coming from the OS or other libraries, and internal ones, which are the result of meson tests. The external ones (e.g. SIOCSIFFLAGS, __APPLE__) are usually only defined if they apply, so it is correct to check for their presence with #ifdef. Using #if will also work, as undefined macros evaluate to zero, but it's not good practice to use them that way. If -Wundef has been passed to the compiler, those incorrect uses will be reported (only on platforms where they are not defined, of course). The internal ones (e.g. WITH_QEMU, WITH_STRUCT_IFREQ) are similar, but in this case we control their definition. This means that using means that the feature is not available on the machine we're building on, but it could also mean that we've removed the meson check and forgot to update all users of the macro. In this case, -Wundef would work 100% reliably to detect the issue: if the meson check doesn't exist, neither will the macro, regardless of what platform we're building on. So the approach I'm suggesting is to use a syntax-check rule to ensure that internal macros are only ever checked with #if instead of Of course this requires a full sweep to fix all cases in which we're not already doing things according to the proposal. Should be fairly easy, if annoying. A couple of examples are included here for demonstration purposes. The bigger impact is going to be on the build system. Right now we generally only define WITH_ macros if the check passed, but that will have to change and the result is going to be quite a bit of additional meson code I'm afraid. Thoughts? [1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/SKJ2... Andrea Bolognani (4): configmake: Check for WIN32 correctly meson: Always define WITH_*_DECL macros syntax-check: Ensure WITH_ macros are used correctly meson: Enable -Wundef build-aux/syntax-check.mk | 5 +++++ configmake.h.in | 2 +- meson.build | 3 +++ tests/virmockstathelpers.c | 28 ++++++++++++++-------------- 4 files changed, 23 insertions(+), 15 deletions(-) -- 2.43.2
The macro is only defined on Windows, so we shouldn't check its value but rather whether it's defined at all. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- configmake.h.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configmake.h.in b/configmake.h.in index c9930b6049..6b309472e5 100644 --- a/configmake.h.in +++ b/configmake.h.in @@ -1,5 +1,5 @@ /* DO NOT EDIT! GENERATED AUTOMATICALLY! */ -#if WIN32 +#ifdef WIN32 # include <winsock2.h> /* avoid mingw pollution on DATADIR */ #endif #mesondefine BINDIR -- 2.43.2
On a Thursday in 2024, Andrea Bolognani wrote:
The macro is only defined on Windows, so we shouldn't check its value but rather whether it's defined at all.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- configmake.h.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/configmake.h.in b/configmake.h.in index c9930b6049..6b309472e5 100644 --- a/configmake.h.in +++ b/configmake.h.in @@ -1,5 +1,5 @@ /* DO NOT EDIT! GENERATED AUTOMATICALLY! */
This comment was obsolete back when it was added.
-#if WIN32 +#ifdef WIN32 # include <winsock2.h> /* avoid mingw pollution on DATADIR */ #endif #mesondefine BINDIR
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Instead of only defining them when the corresponding declaration exists, define them all the time and make their value reflect the availability. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- meson.build | 2 ++ tests/virmockstathelpers.c | 28 ++++++++++++++-------------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/meson.build b/meson.build index 7845f60ff7..9628fbcd59 100644 --- a/meson.build +++ b/meson.build @@ -614,6 +614,8 @@ endforeach foreach function : stat_functions if cc.has_header_symbol('sys/stat.h', function) conf.set('WITH_@0@_DECL'.format(function.to_upper()), 1) + else + conf.set('WITH_@0@_DECL'.format(function.to_upper()), 0) endif endforeach diff --git a/tests/virmockstathelpers.c b/tests/virmockstathelpers.c index 8a76c5e369..a794788d03 100644 --- a/tests/virmockstathelpers.c +++ b/tests/virmockstathelpers.c @@ -80,47 +80,47 @@ */ #if !defined(__APPLE__) -# if !defined(WITH___XSTAT_DECL) -# if defined(WITH_STAT) -# if !defined(WITH___XSTAT) && !defined(WITH_STAT64) +# if !WITH___XSTAT_DECL +# if WITH_STAT +# if !WITH___XSTAT && !WITH_STAT64 # define MOCK_STAT # endif # endif -# if defined(WITH_STAT64) +# if WITH_STAT64 # define MOCK_STAT64 # endif # else /* WITH___XSTAT_DECL */ -# if defined(WITH___XSTAT) && !defined(WITH___XSTAT64) +# if WITH___XSTAT && !WITH___XSTAT64 # define MOCK___XSTAT # endif -# if defined(WITH___XSTAT64) +# if WITH___XSTAT64 # define MOCK___XSTAT64 # endif # endif /* WITH___XSTAT_DECL */ -# if !defined(WITH___LXSTAT_DECL) -# if defined(WITH_LSTAT) -# if !defined(WITH___LXSTAT) && !defined(WITH_LSTAT64) +# if !WITH___LXSTAT_DECL +# if WITH_LSTAT +# if !WITH___LXSTAT && !WITH_LSTAT64 # define MOCK_LSTAT # endif # endif -# if defined(WITH_LSTAT64) +# if WITH_LSTAT64 # define MOCK_LSTAT64 # endif # else /* WITH___LXSTAT_DECL */ -# if defined(WITH___LXSTAT) && !defined(WITH___LXSTAT64) +# if WITH___LXSTAT && !WITH___LXSTAT64 # define MOCK___LXSTAT # endif -# if defined(WITH___LXSTAT64) +# if WITH___LXSTAT64 # define MOCK___LXSTAT64 # endif # endif /* WITH___LXSTAT_DECL */ #else /* __APPLE__ */ # define MOCK_STAT -# if defined(WITH_STAT64_DECL) +# if WITH_STAT64_DECL # define MOCK_STAT64 # endif # define MOCK_LSTAT -# if defined(WITH_LSTAT64_DECL) +# if WITH_LSTAT64_DECL # define MOCK_LSTAT64 # endif #endif -- 2.43.2
They are supposed to always be defined, with their value reflecting the availability of the corresponding feature, so using #ifdef or #if defined() with them is incorrect. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- build-aux/syntax-check.mk | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index 2ac8c5760f..067f47ac03 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -1347,6 +1347,11 @@ sc_rst_since: halt='format :since: correctly' \ $(_sc_search_regexp) +sc_with_ifdef: + @prohibit='^# *(ifdef +|ifndef +|if.*defined\()WITH_' \ + halt='do not use #ifdef or #if defined() for WITH_ macros' \ + $(_sc_search_regexp) + ## ---------- ## ## Exceptions ## -- 2.43.2
This will catch cases in which we try to use the value of a macro that is only defined when the corresponding feature is present. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- meson.build | 1 + 1 file changed, 1 insertion(+) diff --git a/meson.build b/meson.build index 9628fbcd59..3a83fb992a 100644 --- a/meson.build +++ b/meson.build @@ -399,6 +399,7 @@ cc_flags += [ '-Wno-typedef-redefinition', '-Wuninitialized', '-Wunknown-pragmas', + '-Wundef', '-Wunused', '-Wunused-but-set-parameter', '-Wunused-but-set-variable', -- 2.43.2
On a Thursday in 2024, Andrea Bolognani wrote:
A few days ago I have posted a patch[1] that addresses an issue introduced when a meson check was dropped but some uses of the corresponding WITH_ macro were not removed at the same time.
That got me thinking about what we can do to prevent such scenarios from happening again in the future. I have come up with something that I think would be effective, but since applying the approach throughout the entire codebase would require a non-trivial amount of work, I figured I'd ask for feedback before embarking on it.
The idea is that there are two types of macros we can use for conditional compilation: external ones, coming from the OS or other libraries, and internal ones, which are the result of meson tests.
The external ones (e.g. SIOCSIFFLAGS, __APPLE__) are usually only defined if they apply, so it is correct to check for their presence with #ifdef. Using #if will also work, as undefined macros evaluate to zero, but it's not good practice to use them that way. If -Wundef has been passed to the compiler, those incorrect uses will be reported (only on platforms where they are not defined, of course).
The internal ones (e.g. WITH_QEMU, WITH_STRUCT_IFREQ) are similar, but in this case we control their definition. This means that using means that the feature is not available on the machine we're building on, but it could also mean that we've removed the meson check and forgot to update all users of the macro. In this case, -Wundef would work 100% reliably to detect the issue: if the meson check doesn't exist, neither will the macro, regardless of what platform we're building on.
So the approach I'm suggesting is to use a syntax-check rule to ensure that internal macros are only ever checked with #if instead of
Of course this requires a full sweep to fix all cases in which we're not already doing things according to the proposal. Should be fairly easy, if annoying. A couple of examples are included here for demonstration purposes.
The bigger impact is going to be on the build system. Right now we generally only define WITH_ macros if the check passed, but that will have to change and the result is going to be quite a bit of additional meson code I'm afraid.
Thoughts?
I don't like all the extra additional meson code. Can it be somehow simplified? Calling: foreach function : functions conf.set('WITH_@0@'.format(function.to_upper()), cc.has_function(function)) endforeach does not seem to work. Jano
[1] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/message/SKJ2...
Andrea Bolognani (4): configmake: Check for WIN32 correctly meson: Always define WITH_*_DECL macros syntax-check: Ensure WITH_ macros are used correctly meson: Enable -Wundef
build-aux/syntax-check.mk | 5 +++++ configmake.h.in | 2 +- meson.build | 3 +++ tests/virmockstathelpers.c | 28 ++++++++++++++-------------- 4 files changed, 23 insertions(+), 15 deletions(-)
-- 2.43.2 _______________________________________________ Devel mailing list -- devel@lists.libvirt.org To unsubscribe send an email to devel-leave@lists.libvirt.org
On 2/26/25 18:02, Ján Tomko wrote:
On a Thursday in 2024, Andrea Bolognani wrote:
A few days ago I have posted a patch[1] that addresses an issue introduced when a meson check was dropped but some uses of the corresponding WITH_ macro were not removed at the same time.
That got me thinking about what we can do to prevent such scenarios from happening again in the future. I have come up with something that I think would be effective, but since applying the approach throughout the entire codebase would require a non-trivial amount of work, I figured I'd ask for feedback before embarking on it.
The idea is that there are two types of macros we can use for conditional compilation: external ones, coming from the OS or other libraries, and internal ones, which are the result of meson tests.
The external ones (e.g. SIOCSIFFLAGS, __APPLE__) are usually only defined if they apply, so it is correct to check for their presence with #ifdef. Using #if will also work, as undefined macros evaluate to zero, but it's not good practice to use them that way. If -Wundef has been passed to the compiler, those incorrect uses will be reported (only on platforms where they are not defined, of course).
The internal ones (e.g. WITH_QEMU, WITH_STRUCT_IFREQ) are similar, but in this case we control their definition. This means that using means that the feature is not available on the machine we're building on, but it could also mean that we've removed the meson check and forgot to update all users of the macro. In this case, -Wundef would work 100% reliably to detect the issue: if the meson check doesn't exist, neither will the macro, regardless of what platform we're building on.
So the approach I'm suggesting is to use a syntax-check rule to ensure that internal macros are only ever checked with #if instead of
Of course this requires a full sweep to fix all cases in which we're not already doing things according to the proposal. Should be fairly easy, if annoying. A couple of examples are included here for demonstration purposes.
The bigger impact is going to be on the build system. Right now we generally only define WITH_ macros if the check passed, but that will have to change and the result is going to be quite a bit of additional meson code I'm afraid.
Thoughts?
I don't like all the extra additional meson code. Can it be somehow simplified?
Calling: foreach function : functions conf.set('WITH_@0@'.format(function.to_upper()), cc.has_function(function)) endforeach does not seem to work.
That's because conf.set(varname, value) behaves differently for different types of $value: if $value is boolean then "#define/#undef $varname" is generated, if $value is int then "#define $varname $value" is generated, and if $value is string then "#define $varname $value" is generated. Long story short, we want .to_int(): foreach function : stat_functions conf.set('WITH_@0@_DECL'.format(function.to_upper()), cc.has_header_symbol('sys/stat.h', function).to_int()) endforeach But this patch series fixes only a portion of the problem (that where compiler complained about -Wundef). Similar pattern exists for other $functions, e,g: tests/virmock.h:#ifndef WITH___OPEN_2_DECL I suggest turning at least those two foreach loops above and below too. This still leaves us with plenty of: if condition conf.set('WITH_SOMETHING', 1) endif but well, who said this is trivial task? Michal
On Thu, Feb 27, 2025 at 10:48:43AM +0100, Michal Prívozník wrote:
On 2/26/25 18:02, Ján Tomko wrote:
On a Thursday in 2024, Andrea Bolognani wrote:
A few days ago I have posted a patch[1] that addresses an issue introduced when a meson check was dropped but some uses of the corresponding WITH_ macro were not removed at the same time.
That got me thinking about what we can do to prevent such scenarios from happening again in the future. I have come up with something that I think would be effective, but since applying the approach throughout the entire codebase would require a non-trivial amount of work, I figured I'd ask for feedback before embarking on it.
The idea is that there are two types of macros we can use for conditional compilation: external ones, coming from the OS or other libraries, and internal ones, which are the result of meson tests.
The external ones (e.g. SIOCSIFFLAGS, __APPLE__) are usually only defined if they apply, so it is correct to check for their presence with #ifdef. Using #if will also work, as undefined macros evaluate to zero, but it's not good practice to use them that way. If -Wundef has been passed to the compiler, those incorrect uses will be reported (only on platforms where they are not defined, of course).
The internal ones (e.g. WITH_QEMU, WITH_STRUCT_IFREQ) are similar, but in this case we control their definition. This means that using means that the feature is not available on the machine we're building on, but it could also mean that we've removed the meson check and forgot to update all users of the macro. In this case, -Wundef would work 100% reliably to detect the issue: if the meson check doesn't exist, neither will the macro, regardless of what platform we're building on.
So the approach I'm suggesting is to use a syntax-check rule to ensure that internal macros are only ever checked with #if instead of
Of course this requires a full sweep to fix all cases in which we're not already doing things according to the proposal. Should be fairly easy, if annoying. A couple of examples are included here for demonstration purposes.
The bigger impact is going to be on the build system. Right now we generally only define WITH_ macros if the check passed, but that will have to change and the result is going to be quite a bit of additional meson code I'm afraid.
Thoughts?
I don't like all the extra additional meson code. Can it be somehow simplified?
Calling: foreach function : functions conf.set('WITH_@0@'.format(function.to_upper()), cc.has_function(function)) endforeach does not seem to work.
That's because conf.set(varname, value) behaves differently for different types of $value:
if $value is boolean then "#define/#undef $varname" is generated, if $value is int then "#define $varname $value" is generated, and if $value is string then "#define $varname $value" is generated.
Long story short, we want .to_int():
foreach function : stat_functions conf.set('WITH_@0@_DECL'.format(function.to_upper()), cc.has_header_symbol('sys/stat.h', function).to_int()) endforeach
But this patch series fixes only a portion of the problem (that where compiler complained about -Wundef). Similar pattern exists for other $functions, e,g:
tests/virmock.h:#ifndef WITH___OPEN_2_DECL
I suggest turning at least those two foreach loops above and below too. This still leaves us with plenty of:
if condition conf.set('WITH_SOMETHING', 1) endif
but well, who said this is trivial task?
This all sounds like a large amount of churn, which risks introducing regressions, for a very small amount of potential future benefit.... Not convinced it is worth it. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (4)
-
Andrea Bolognani -
Daniel P. Berrangé -
Ján Tomko -
Michal Prívozník