| Message ID | 20211116093834.76615-1-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | meson: fix botched compile check conversions | expand |
On Tue, Nov 16, 2021 at 10:38:34AM +0100, Paolo Bonzini wrote: > Fix a bunch of incorrect conversions from configure to Meson, which result > in different outcomes with --extra-cflags=-Werror. > > pthread_setname_np needs "#define _GNU_SOURCE" on Linux (which I am using > also for the non-Linux check, so that it correctly fails with an error > about having too few parameters). > > Fix struct checks to use has_type instead of has_symbol, and "#define > _GNU_SOURCE" too in the case of struct mmsghdr. Ok, so relies on fact that passing an incorrect number of arguments is a fatal error, when function prototypes are available, even without -Werror being set. Side note, GCC looks to be trying to make explicit function prototypes mandatory at last https://linuxplumbersconf.org/event/11/contributions/1014/ > Remove an apostrophe that ended up at the end of a #include line. > > Reported-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> And the setname_np stuff, I've tested on Fedora 34 and FreebSD 12.2 Tested-by: Daniel P. Berrangé <berrange@redhat.com> > --- > meson.build | 24 +++++++++++++++--------- > 1 file changed, 15 insertions(+), 9 deletions(-) > > diff --git a/meson.build b/meson.build > index 2ece4fe088..93a5e50a16 100644 > --- a/meson.build > +++ b/meson.build > @@ -1547,8 +1547,6 @@ config_host_data.set('CONFIG_INOTIFY', > cc.has_header_symbol('sys/inotify.h', 'inotify_init')) > config_host_data.set('CONFIG_INOTIFY1', > cc.has_header_symbol('sys/inotify.h', 'inotify_init1')) > -config_host_data.set('CONFIG_IOVEC', > - cc.has_header_symbol('sys/uio.h', 'struct iovec')) > config_host_data.set('CONFIG_MACHINE_BSWAP_H', > cc.has_header_symbol('machine/bswap.h', 'bswap32', > prefix: '''#include <sys/endian.h> > @@ -1561,8 +1559,6 @@ config_host_data.set('CONFIG_SYSMACROS', > cc.has_header_symbol('sys/sysmacros.h', 'makedev')) > config_host_data.set('HAVE_OPTRESET', > cc.has_header_symbol('getopt.h', 'optreset')) > -config_host_data.set('HAVE_UTMPX', > - cc.has_header_symbol('utmpx.h', 'struct utmpx')) > config_host_data.set('HAVE_IPPROTO_MPTCP', > cc.has_header_symbol('netinet/in.h', 'IPPROTO_MPTCP')) > > @@ -1574,6 +1570,14 @@ config_host_data.set('HAVE_STRUCT_STAT_ST_ATIM', > cc.has_member('struct stat', 'st_atim', > prefix: '#include <sys/stat.h>')) > > +# has_type > +config_host_data.set('CONFIG_IOVEC', > + cc.has_type('struct iovec', > + prefix: '#include <sys/uio.h>')) > +config_host_data.set('HAVE_UTMPX', > + cc.has_type('struct utmpx', > + prefix: '#include <utmpx.h>')) > + > config_host_data.set('CONFIG_EVENTFD', cc.links(''' > #include <sys/eventfd.h> > int main(void) { return eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC); }''')) > @@ -1615,7 +1619,7 @@ config_host_data.set('CONFIG_POSIX_MADVISE', cc.links(gnu_source_prefix + ''' > #include <stddef.h> > int main(void) { return posix_madvise(NULL, 0, POSIX_MADV_DONTNEED); }''')) > > -config_host_data.set('CONFIG_PTHREAD_SETNAME_NP_W_TID', cc.links(''' > +config_host_data.set('CONFIG_PTHREAD_SETNAME_NP_W_TID', cc.links(gnu_source_prefix + ''' > #include <pthread.h> > > static void *f(void *p) { return NULL; } > @@ -1626,7 +1630,7 @@ config_host_data.set('CONFIG_PTHREAD_SETNAME_NP_W_TID', cc.links(''' > pthread_setname_np(thread, "QEMU"); > return 0; > }''', dependencies: threads)) > -config_host_data.set('CONFIG_PTHREAD_SETNAME_NP_WO_TID', cc.links(''' > +config_host_data.set('CONFIG_PTHREAD_SETNAME_NP_WO_TID', cc.links(gnu_source_prefix + ''' > #include <pthread.h> > > static void *f(void *p) { pthread_setname_np("QEMU"); return NULL; } > @@ -1662,8 +1666,10 @@ config_host_data.set('HAVE_MLOCKALL', cc.links(gnu_source_prefix + ''' > > have_l2tpv3 = false > if not get_option('l2tpv3').disabled() and have_system > - have_l2tpv3 = (cc.has_header_symbol('sys/socket.h', 'struct mmsghdr') > - and cc.has_header('linux/ip.h')) > + have_l2tpv3 = cc.has_type('struct mmsghdr', > + prefix: gnu_source_prefix + ''' > + #include <sys/socket.h> > + #include <linux/ip.h>''') > endif > config_host_data.set('CONFIG_L2TPV3', have_l2tpv3) > > @@ -1689,7 +1695,7 @@ config_host_data.set('CONFIG_NETMAP', have_netmap) > # xfs headers will not try to redefine structs from linux headers > # if this macro is set. > config_host_data.set('HAVE_FSXATTR', cc.links(''' > - #include <linux/fs.h>' > + #include <linux/fs.h> > struct fsxattr foo; > int main(void) { > return 0; > -- > 2.33.1 > > Regards, Daniel
On Tue, 16 Nov 2021 at 09:38, Paolo Bonzini <pbonzini@redhat.com> wrote: > > Fix a bunch of incorrect conversions from configure to Meson, which result > in different outcomes with --extra-cflags=-Werror. FWIW this still won't give the right answer for the 'struct iovec' test if you include -Werror via --extra-cflags, because the generated code trips over an "expression result unused" warning: Running compile: Working directory: /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-clang/meson-private/tmpeiz36t2n Command line: clang-7 -m64 -mcx16 /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-clang/meson-private/tmpeiz36t2n/testfile.c -o /mnt/nvmedis k/linaro/qemu-from-laptop/qemu/build/arm-clang/meson-private/tmpeiz36t2n/output.obj -c -fsanitize=undefined -fno-sanitize=shift-base -Werror -D_FI LE_OFFSET_BITS=64 -O0 -Werror=implicit-function-declaration -Werror=unknown-warning-option -Werror=unused-command-line-argument -Werror=ignored-op timization-argument -std=gnu11 Code: #include <sys/uio.h> void bar(void) { sizeof(struct iovec); }; Compiler stdout: Compiler stderr: /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-clang/meson-private/tmpeiz36t2n/testfile.c:3:13: error: expression result unused [-Werror,-Wunused-value] sizeof(struct iovec); ^~~~~~~~~~~~~~~~~~~~ 1 error generated. Checking for type "struct iovec" : NO But maybe we should just explicitly reject -Werror in --extra-cflags... -- PMM
On Tue, Nov 16, 2021 at 11:51:16AM +0000, Peter Maydell wrote: > On Tue, 16 Nov 2021 at 09:38, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > Fix a bunch of incorrect conversions from configure to Meson, which result > > in different outcomes with --extra-cflags=-Werror. > > FWIW this still won't give the right answer for the 'struct iovec' > test if you include -Werror via --extra-cflags, because the > generated code trips over an "expression result unused" warning: > > > Running compile: > Working directory: > /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-clang/meson-private/tmpeiz36t2n > Command line: clang-7 -m64 -mcx16 > /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-clang/meson-private/tmpeiz36t2n/testfile.c > -o /mnt/nvmedis > k/linaro/qemu-from-laptop/qemu/build/arm-clang/meson-private/tmpeiz36t2n/output.obj > -c -fsanitize=undefined -fno-sanitize=shift-base -Werror -D_FI > LE_OFFSET_BITS=64 -O0 -Werror=implicit-function-declaration > -Werror=unknown-warning-option -Werror=unused-command-line-argument > -Werror=ignored-op > timization-argument -std=gnu11 > > Code: > #include <sys/uio.h> > void bar(void) { > sizeof(struct iovec); > }; > Compiler stdout: > > Compiler stderr: > /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-clang/meson-private/tmpeiz36t2n/testfile.c:3:13: > error: expression result unused [-Werror,-Wunused-value] > sizeof(struct iovec); > ^~~~~~~~~~~~~~~~~~~~ > 1 error generated. > > Checking for type "struct iovec" : NO > > > But maybe we should just explicitly reject -Werror in --extra-cflags... I wonder if the problem is more fundamental than that. Passing stuff in --extra-cflags is done to influence the flags used to compile the QEMU end user binaries. Unfortunately --extra-cflags is also getting applied to all the meson.build feature checks. IMHO we would get a more reliable result if the meson.build checks were fully isolated from the cflags we used for building everything else, so the checks get a well understood, predictable environment. IIUC, this current behaviour is a result of us adding cflags using add_global_arguments / add_project_arguments. I wonder if we need to exclusively use the 'c_args' parameter to library()/executable() and friends ? The downside is of course would be extra work to make sure we pass c_args in all the right places. Regards, Daniel
On Tue, 16 Nov 2021 at 12:00, Daniel P. Berrangé <berrange@redhat.com> wrote: > I wonder if the problem is more fundamental than that. Passing > stuff in --extra-cflags is done to influence the flags used to > compile the QEMU end user binaries. Unfortunately --extra-cflags > is also getting applied to all the meson.build feature checks. > > IMHO we would get a more reliable result if the meson.build > checks were fully isolated from the cflags we used for building > everything else, so the checks get a well understood, predictable > environment. If you're using --extra-cflags to pass in "-I/path/to/libfoo/headers" then you do want that to be used in the meson check for "do we have libfoo", though... -- PMM
On Tue, Nov 16, 2021 at 01:10:04PM +0000, Peter Maydell wrote: > On Tue, 16 Nov 2021 at 12:00, Daniel P. Berrangé <berrange@redhat.com> wrote: > > I wonder if the problem is more fundamental than that. Passing > > stuff in --extra-cflags is done to influence the flags used to > > compile the QEMU end user binaries. Unfortunately --extra-cflags > > is also getting applied to all the meson.build feature checks. > > > > IMHO we would get a more reliable result if the meson.build > > checks were fully isolated from the cflags we used for building > > everything else, so the checks get a well understood, predictable > > environment. > > If you're using --extra-cflags to pass in "-I/path/to/libfoo/headers" > then you do want that to be used in the meson check for "do we have > libfoo", though... For pkg-config-ized things meson lets you override those paths per-library IIRC. For the non-pkg-config-izied case, that could be an argument for specialized --extra-header-dir=PATH and --extra-lib-dir=$PATH args. Regards, Daniel
On 11/16/21 12:13, Daniel P. Berrangé wrote: > On Tue, Nov 16, 2021 at 10:38:34AM +0100, Paolo Bonzini wrote: >> Fix a bunch of incorrect conversions from configure to Meson, which result >> in different outcomes with --extra-cflags=-Werror. >> >> pthread_setname_np needs "#define _GNU_SOURCE" on Linux (which I am using >> also for the non-Linux check, so that it correctly fails with an error >> about having too few parameters). >> >> Fix struct checks to use has_type instead of has_symbol, and "#define >> _GNU_SOURCE" too in the case of struct mmsghdr. > > Ok, so relies on fact that passing an incorrect number of arguments > is a fatal error, when function prototypes are available, even > without -Werror being set. > > Side note, GCC looks to be trying to make explicit function prototypes > mandatory at last > > https://linuxplumbersconf.org/event/11/contributions/1014/ Yes, and that's also why Meson adds the -Werror=implicit-declaration option; these days it's simply too dangerous to assume a function is present, if it links but you don't really know which header it comes from. Paolo
On 11/16/21 13:00, Daniel P. Berrangé wrote: > I wonder if the problem is more fundamental than that. Passing > stuff in --extra-cflags is done to influence the flags used to > compile the QEMU end user binaries. Unfortunately --extra-cflags > is also getting applied to all the meson.build feature checks. > > IMHO we would get a more reliable result if the meson.build > checks were fully isolated from the cflags we used for building > everything else, so the checks get a well understood, predictable > environment. > > IIUC, this current behaviour is a result of us adding cflags > using add_global_arguments / add_project_arguments. No, it's not using add_global_arguments/add_project_arguments for --extra-cflags. The --extra-cflags (aka the CFLAGS envvar, or "meson setup -Dc_args") is messy: on one hand it's kinda legacy (we have configure flags to set -O2, -g, -Werror, etc.), on the other hand not really possible to kill it because it's how distros expect to set flags such as -O2. But it's full of pitfalls, and the only good use of it seems to be for -I and -L flags. We already saw issues with it last week with distros adding "-Wall" to CFLAGS or --extra-cflags and that gives you bogus warnings. Unfortunately you certainly want flags such as -g to override earlier flags, and you might even want -Wall to override earlier -Wno-* flags *unless -Werror is in use*. Apart from this, the sizeof() issue (which by the way I didn't see with GCC) has to be fixed in Meson and is probably visible also in e.g. has_members. Paolo
diff --git a/meson.build b/meson.build index 2ece4fe088..93a5e50a16 100644 --- a/meson.build +++ b/meson.build @@ -1547,8 +1547,6 @@ config_host_data.set('CONFIG_INOTIFY', cc.has_header_symbol('sys/inotify.h', 'inotify_init')) config_host_data.set('CONFIG_INOTIFY1', cc.has_header_symbol('sys/inotify.h', 'inotify_init1')) -config_host_data.set('CONFIG_IOVEC', - cc.has_header_symbol('sys/uio.h', 'struct iovec')) config_host_data.set('CONFIG_MACHINE_BSWAP_H', cc.has_header_symbol('machine/bswap.h', 'bswap32', prefix: '''#include <sys/endian.h> @@ -1561,8 +1559,6 @@ config_host_data.set('CONFIG_SYSMACROS', cc.has_header_symbol('sys/sysmacros.h', 'makedev')) config_host_data.set('HAVE_OPTRESET', cc.has_header_symbol('getopt.h', 'optreset')) -config_host_data.set('HAVE_UTMPX', - cc.has_header_symbol('utmpx.h', 'struct utmpx')) config_host_data.set('HAVE_IPPROTO_MPTCP', cc.has_header_symbol('netinet/in.h', 'IPPROTO_MPTCP')) @@ -1574,6 +1570,14 @@ config_host_data.set('HAVE_STRUCT_STAT_ST_ATIM', cc.has_member('struct stat', 'st_atim', prefix: '#include <sys/stat.h>')) +# has_type +config_host_data.set('CONFIG_IOVEC', + cc.has_type('struct iovec', + prefix: '#include <sys/uio.h>')) +config_host_data.set('HAVE_UTMPX', + cc.has_type('struct utmpx', + prefix: '#include <utmpx.h>')) + config_host_data.set('CONFIG_EVENTFD', cc.links(''' #include <sys/eventfd.h> int main(void) { return eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC); }''')) @@ -1615,7 +1619,7 @@ config_host_data.set('CONFIG_POSIX_MADVISE', cc.links(gnu_source_prefix + ''' #include <stddef.h> int main(void) { return posix_madvise(NULL, 0, POSIX_MADV_DONTNEED); }''')) -config_host_data.set('CONFIG_PTHREAD_SETNAME_NP_W_TID', cc.links(''' +config_host_data.set('CONFIG_PTHREAD_SETNAME_NP_W_TID', cc.links(gnu_source_prefix + ''' #include <pthread.h> static void *f(void *p) { return NULL; } @@ -1626,7 +1630,7 @@ config_host_data.set('CONFIG_PTHREAD_SETNAME_NP_W_TID', cc.links(''' pthread_setname_np(thread, "QEMU"); return 0; }''', dependencies: threads)) -config_host_data.set('CONFIG_PTHREAD_SETNAME_NP_WO_TID', cc.links(''' +config_host_data.set('CONFIG_PTHREAD_SETNAME_NP_WO_TID', cc.links(gnu_source_prefix + ''' #include <pthread.h> static void *f(void *p) { pthread_setname_np("QEMU"); return NULL; } @@ -1662,8 +1666,10 @@ config_host_data.set('HAVE_MLOCKALL', cc.links(gnu_source_prefix + ''' have_l2tpv3 = false if not get_option('l2tpv3').disabled() and have_system - have_l2tpv3 = (cc.has_header_symbol('sys/socket.h', 'struct mmsghdr') - and cc.has_header('linux/ip.h')) + have_l2tpv3 = cc.has_type('struct mmsghdr', + prefix: gnu_source_prefix + ''' + #include <sys/socket.h> + #include <linux/ip.h>''') endif config_host_data.set('CONFIG_L2TPV3', have_l2tpv3) @@ -1689,7 +1695,7 @@ config_host_data.set('CONFIG_NETMAP', have_netmap) # xfs headers will not try to redefine structs from linux headers # if this macro is set. config_host_data.set('HAVE_FSXATTR', cc.links(''' - #include <linux/fs.h>' + #include <linux/fs.h> struct fsxattr foo; int main(void) { return 0;
Fix a bunch of incorrect conversions from configure to Meson, which result in different outcomes with --extra-cflags=-Werror. pthread_setname_np needs "#define _GNU_SOURCE" on Linux (which I am using also for the non-Linux check, so that it correctly fails with an error about having too few parameters). Fix struct checks to use has_type instead of has_symbol, and "#define _GNU_SOURCE" too in the case of struct mmsghdr. Remove an apostrophe that ended up at the end of a #include line. Reported-by: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- meson.build | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)