| Message ID | 20231205183226.26636-1-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Only compile the hypervisor with -Wdeclaration-after-statement | expand |
On 05/12/2023 6:32 pm, Julien Grall wrote: > diff --git a/xen/Makefile b/xen/Makefile > index ca571103c868..360fb6dcae57 100644 > --- a/xen/Makefile > +++ b/xen/Makefile > @@ -393,6 +393,7 @@ CFLAGS-$(CONFIG_CC_SPLIT_SECTIONS) += -ffunction-sections -fdata-sections > > CFLAGS += -nostdinc -fno-builtin -fno-common > CFLAGS += -Werror -Wredundant-decls -Wwrite-strings -Wno-pointer-arith > +$(call cc-option-add,CFLAGS,CC,-Wdeclaration-after-statement) I agree with the direction of the patch, but this should be a plain CFLAGS += All compilers we support, even on x86, know -Wdeclaration-after-statement so it doesn't need a toolchain-check to activate. https://godbolt.org/z/PM7bb1d55 With that fixed, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Hi Andrew, On 05/12/2023 19:49, Andrew Cooper wrote: > On 05/12/2023 6:32 pm, Julien Grall wrote: >> diff --git a/xen/Makefile b/xen/Makefile >> index ca571103c868..360fb6dcae57 100644 >> --- a/xen/Makefile >> +++ b/xen/Makefile >> @@ -393,6 +393,7 @@ CFLAGS-$(CONFIG_CC_SPLIT_SECTIONS) += -ffunction-sections -fdata-sections >> >> CFLAGS += -nostdinc -fno-builtin -fno-common >> CFLAGS += -Werror -Wredundant-decls -Wwrite-strings -Wno-pointer-arith >> +$(call cc-option-add,CFLAGS,CC,-Wdeclaration-after-statement) > > I agree with the direction of the patch, but this should be a plain > CFLAGS += > > All compilers we support, even on x86, know > -Wdeclaration-after-statement so it doesn't need a toolchain-check to > activate. That's good to know, I wasn't sure and.. > > https://godbolt.org/z/PM7bb1d55 .. I keep forgetting godbolt.org can help to check old compilers. I will update the patch in my tree. > > With that fixed, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks! Cheers,
On 05.12.2023 19:32, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > Right now, all tools and hypervisor will be complied with the option > -Wdeclaration-after-statement. While most of the code in the hypervisor > is controlled by us, for tools we may import external libraries. > > The build will fail if one of them are using the construct we are > trying to prevent. This is the case when building against Python 3.12 > and Yocto: > > | In file included from /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/Python.h:44, > | from xen/lowlevel/xc/xc.c:8: > | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/object.h: In function 'Py_SIZE': > | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/object.h:233:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] > | 233 | PyVarObject *var_ob = _PyVarObject_CAST(ob); > | | ^~~~~~~~~~~ > | In file included from /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/Python.h:53: > | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/cpython/longintrepr.h: In function '_PyLong_CompactValue': > | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/cpython/longintrepr.h:121:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] > | 121 | Py_ssize_t sign = 1 - (op->long_value.lv_tag & _PyLong_SIGN_MASK); > | | ^~~~~~~~~~ > | cc1: all warnings being treated as errors > > Looking at the tools directory, a fair few directory already add > -Wno-declaration-after-statement to inhibit the default behavior. > > We have always build the hypervisor with the flag, so for now remove > only the flag for anything but the hypervisor. We can decide at later > time whether we want to relax. > > Also remove the -Wno-declaration-after-statement in some subdirectory > as the flag is now unnecessary. With all these removals, don't you need to add the option centrally somewhere? Or else are you sure that no compiler version, including distro-customized ones, would ever come with the warning enabled by default? Jan
Hi Jan, On 06/12/2023 08:52, Jan Beulich wrote: > On 05.12.2023 19:32, Julien Grall wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> Right now, all tools and hypervisor will be complied with the option >> -Wdeclaration-after-statement. While most of the code in the hypervisor >> is controlled by us, for tools we may import external libraries. >> >> The build will fail if one of them are using the construct we are >> trying to prevent. This is the case when building against Python 3.12 >> and Yocto: >> >> | In file included from /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/Python.h:44, >> | from xen/lowlevel/xc/xc.c:8: >> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/object.h: In function 'Py_SIZE': >> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/object.h:233:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] >> | 233 | PyVarObject *var_ob = _PyVarObject_CAST(ob); >> | | ^~~~~~~~~~~ >> | In file included from /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/Python.h:53: >> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/cpython/longintrepr.h: In function '_PyLong_CompactValue': >> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/cpython/longintrepr.h:121:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] >> | 121 | Py_ssize_t sign = 1 - (op->long_value.lv_tag & _PyLong_SIGN_MASK); >> | | ^~~~~~~~~~ >> | cc1: all warnings being treated as errors >> >> Looking at the tools directory, a fair few directory already add >> -Wno-declaration-after-statement to inhibit the default behavior. >> >> We have always build the hypervisor with the flag, so for now remove >> only the flag for anything but the hypervisor. We can decide at later >> time whether we want to relax. >> >> Also remove the -Wno-declaration-after-statement in some subdirectory >> as the flag is now unnecessary. > > With all these removals, don't you need to add the option centrally > somewhere? Or else are you sure that no compiler version, including > distro-customized ones, would ever come with the warning enabled by > default? I can't really go through the dozens of different built compilers... But I would find odd that a compiler will force this option. I view it as a style enforcement option and that's not up to a distro to decide what every projects do. Also, Allowing your thinking, we would need to add -Wno-switch-default & co just in case a compiler decide to enable it. So where would we stop adding -Wno-*? I don't think this is very scalable. Cheers,
On 06.12.2023 11:52, Julien Grall wrote: > Hi Jan, > > On 06/12/2023 08:52, Jan Beulich wrote: >> On 05.12.2023 19:32, Julien Grall wrote: >>> From: Julien Grall <jgrall@amazon.com> >>> >>> Right now, all tools and hypervisor will be complied with the option >>> -Wdeclaration-after-statement. While most of the code in the hypervisor >>> is controlled by us, for tools we may import external libraries. >>> >>> The build will fail if one of them are using the construct we are >>> trying to prevent. This is the case when building against Python 3.12 >>> and Yocto: >>> >>> | In file included from /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/Python.h:44, >>> | from xen/lowlevel/xc/xc.c:8: >>> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/object.h: In function 'Py_SIZE': >>> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/object.h:233:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] >>> | 233 | PyVarObject *var_ob = _PyVarObject_CAST(ob); >>> | | ^~~~~~~~~~~ >>> | In file included from /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/Python.h:53: >>> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/cpython/longintrepr.h: In function '_PyLong_CompactValue': >>> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/cpython/longintrepr.h:121:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] >>> | 121 | Py_ssize_t sign = 1 - (op->long_value.lv_tag & _PyLong_SIGN_MASK); >>> | | ^~~~~~~~~~ >>> | cc1: all warnings being treated as errors >>> >>> Looking at the tools directory, a fair few directory already add >>> -Wno-declaration-after-statement to inhibit the default behavior. >>> >>> We have always build the hypervisor with the flag, so for now remove >>> only the flag for anything but the hypervisor. We can decide at later >>> time whether we want to relax. >>> >>> Also remove the -Wno-declaration-after-statement in some subdirectory >>> as the flag is now unnecessary. >> >> With all these removals, don't you need to add the option centrally >> somewhere? Or else are you sure that no compiler version, including >> distro-customized ones, would ever come with the warning enabled by >> default? > > I can't really go through the dozens of different built compilers... But > I would find odd that a compiler will force this option. I view it as a > style enforcement option and that's not up to a distro to decide what > every projects do. > > Also, Allowing your thinking, we would need to add -Wno-switch-default & > co just in case a compiler decide to enable it. So where would we stop > adding -Wno-*? > > I don't think this is very scalable. I agree on this point, but: With the change you do there's a (slim) risk of introducing build breakage. With other -W* / -Wno-* options we haven't had reports of build issues. Yet then, thinkking more generally, imo it would be a good idea to always for either the "yes" or the "no" option for warnings we care about controlling ourselves. But that's nothing belonging in your change, of course. Jan
Hi Jan, On 06/12/2023 11:05, Jan Beulich wrote: > On 06.12.2023 11:52, Julien Grall wrote: >> Hi Jan, >> >> On 06/12/2023 08:52, Jan Beulich wrote: >>> On 05.12.2023 19:32, Julien Grall wrote: >>>> From: Julien Grall <jgrall@amazon.com> >>>> >>>> Right now, all tools and hypervisor will be complied with the option >>>> -Wdeclaration-after-statement. While most of the code in the hypervisor >>>> is controlled by us, for tools we may import external libraries. >>>> >>>> The build will fail if one of them are using the construct we are >>>> trying to prevent. This is the case when building against Python 3.12 >>>> and Yocto: >>>> >>>> | In file included from /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/Python.h:44, >>>> | from xen/lowlevel/xc/xc.c:8: >>>> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/object.h: In function 'Py_SIZE': >>>> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/object.h:233:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] >>>> | 233 | PyVarObject *var_ob = _PyVarObject_CAST(ob); >>>> | | ^~~~~~~~~~~ >>>> | In file included from /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/Python.h:53: >>>> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/cpython/longintrepr.h: In function '_PyLong_CompactValue': >>>> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/cpython/longintrepr.h:121:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] >>>> | 121 | Py_ssize_t sign = 1 - (op->long_value.lv_tag & _PyLong_SIGN_MASK); >>>> | | ^~~~~~~~~~ >>>> | cc1: all warnings being treated as errors >>>> >>>> Looking at the tools directory, a fair few directory already add >>>> -Wno-declaration-after-statement to inhibit the default behavior. >>>> >>>> We have always build the hypervisor with the flag, so for now remove >>>> only the flag for anything but the hypervisor. We can decide at later >>>> time whether we want to relax. >>>> >>>> Also remove the -Wno-declaration-after-statement in some subdirectory >>>> as the flag is now unnecessary. >>> >>> With all these removals, don't you need to add the option centrally >>> somewhere? Or else are you sure that no compiler version, including >>> distro-customized ones, would ever come with the warning enabled by >>> default? >> >> I can't really go through the dozens of different built compilers... But >> I would find odd that a compiler will force this option. I view it as a >> style enforcement option and that's not up to a distro to decide what >> every projects do. >> >> Also, Allowing your thinking, we would need to add -Wno-switch-default & >> co just in case a compiler decide to enable it. So where would we stop >> adding -Wno-*? >> >> I don't think this is very scalable. > > I agree on this point, but: With the change you do there's a (slim) risk > of introducing build breakage. With other -W* / -Wno-* options we haven't > had reports of build issues. The chance is very unlikely here. So I am not in favor of doing this. I would like the opinion from the others. Cheers,
On 06.12.2023 12:07, Julien Grall wrote: > Hi Jan, > > On 06/12/2023 11:05, Jan Beulich wrote: >> On 06.12.2023 11:52, Julien Grall wrote: >>> Hi Jan, >>> >>> On 06/12/2023 08:52, Jan Beulich wrote: >>>> On 05.12.2023 19:32, Julien Grall wrote: >>>>> From: Julien Grall <jgrall@amazon.com> >>>>> >>>>> Right now, all tools and hypervisor will be complied with the option >>>>> -Wdeclaration-after-statement. While most of the code in the hypervisor >>>>> is controlled by us, for tools we may import external libraries. >>>>> >>>>> The build will fail if one of them are using the construct we are >>>>> trying to prevent. This is the case when building against Python 3.12 >>>>> and Yocto: >>>>> >>>>> | In file included from /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/Python.h:44, >>>>> | from xen/lowlevel/xc/xc.c:8: >>>>> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/object.h: In function 'Py_SIZE': >>>>> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/object.h:233:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] >>>>> | 233 | PyVarObject *var_ob = _PyVarObject_CAST(ob); >>>>> | | ^~~~~~~~~~~ >>>>> | In file included from /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/Python.h:53: >>>>> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/cpython/longintrepr.h: In function '_PyLong_CompactValue': >>>>> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/cpython/longintrepr.h:121:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] >>>>> | 121 | Py_ssize_t sign = 1 - (op->long_value.lv_tag & _PyLong_SIGN_MASK); >>>>> | | ^~~~~~~~~~ >>>>> | cc1: all warnings being treated as errors >>>>> >>>>> Looking at the tools directory, a fair few directory already add >>>>> -Wno-declaration-after-statement to inhibit the default behavior. >>>>> >>>>> We have always build the hypervisor with the flag, so for now remove >>>>> only the flag for anything but the hypervisor. We can decide at later >>>>> time whether we want to relax. >>>>> >>>>> Also remove the -Wno-declaration-after-statement in some subdirectory >>>>> as the flag is now unnecessary. >>>> >>>> With all these removals, don't you need to add the option centrally >>>> somewhere? Or else are you sure that no compiler version, including >>>> distro-customized ones, would ever come with the warning enabled by >>>> default? >>> >>> I can't really go through the dozens of different built compilers... But >>> I would find odd that a compiler will force this option. I view it as a >>> style enforcement option and that's not up to a distro to decide what >>> every projects do. >>> >>> Also, Allowing your thinking, we would need to add -Wno-switch-default & >>> co just in case a compiler decide to enable it. So where would we stop >>> adding -Wno-*? >>> >>> I don't think this is very scalable. >> >> I agree on this point, but: With the change you do there's a (slim) risk >> of introducing build breakage. With other -W* / -Wno-* options we haven't >> had reports of build issues. > > The chance is very unlikely here. So I am not in favor of doing this. I > would like the opinion from the others. Well, it's Anthony anyway who has the final say. I'm not going to insist, I merely wanted to point out a possible issue. Jan
On 06/12/2023 8:52 am, Jan Beulich wrote: > On 05.12.2023 19:32, Julien Grall wrote: >> From: Julien Grall <jgrall@amazon.com> >> >> Right now, all tools and hypervisor will be complied with the option >> -Wdeclaration-after-statement. While most of the code in the hypervisor >> is controlled by us, for tools we may import external libraries. >> >> The build will fail if one of them are using the construct we are >> trying to prevent. This is the case when building against Python 3.12 >> and Yocto: >> >> | In file included from /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/Python.h:44, >> | from xen/lowlevel/xc/xc.c:8: >> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/object.h: In function 'Py_SIZE': >> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/object.h:233:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] >> | 233 | PyVarObject *var_ob = _PyVarObject_CAST(ob); >> | | ^~~~~~~~~~~ >> | In file included from /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/Python.h:53: >> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/cpython/longintrepr.h: In function '_PyLong_CompactValue': >> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/cpython/longintrepr.h:121:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] >> | 121 | Py_ssize_t sign = 1 - (op->long_value.lv_tag & _PyLong_SIGN_MASK); >> | | ^~~~~~~~~~ >> | cc1: all warnings being treated as errors >> >> Looking at the tools directory, a fair few directory already add >> -Wno-declaration-after-statement to inhibit the default behavior. >> >> We have always build the hypervisor with the flag, so for now remove >> only the flag for anything but the hypervisor. We can decide at later >> time whether we want to relax. >> >> Also remove the -Wno-declaration-after-statement in some subdirectory >> as the flag is now unnecessary. > With all these removals, don't you need to add the option centrally > somewhere? Or else are you sure that no compiler version, including > distro-customized ones, would ever come with the warning enabled by > default? declaration-after-statement being permitted is covered by the -std we choose. No distro is plausibly going to ship a compiler which takes -std=gnu99 and has declaration-after-statement forced off if in that configuration. ~Andrew
On Tue, Dec 05, 2023 at 06:32:26PM +0000, Julien Grall wrote: > From: Julien Grall <jgrall@amazon.com> > > Right now, all tools and hypervisor will be complied with the option > -Wdeclaration-after-statement. While most of the code in the hypervisor > is controlled by us, for tools we may import external libraries. > > The build will fail if one of them are using the construct we are > trying to prevent. This is the case when building against Python 3.12 > and Yocto: > > | In file included from /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/Python.h:44, > | from xen/lowlevel/xc/xc.c:8: > | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/object.h: In function 'Py_SIZE': > | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/object.h:233:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] > | 233 | PyVarObject *var_ob = _PyVarObject_CAST(ob); > | | ^~~~~~~~~~~ > | In file included from /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/Python.h:53: > | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/cpython/longintrepr.h: In function '_PyLong_CompactValue': > | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/cpython/longintrepr.h:121:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] > | 121 | Py_ssize_t sign = 1 - (op->long_value.lv_tag & _PyLong_SIGN_MASK); > | | ^~~~~~~~~~ > | cc1: all warnings being treated as errors > > Looking at the tools directory, a fair few directory already add > -Wno-declaration-after-statement to inhibit the default behavior. > > We have always build the hypervisor with the flag, so for now remove > only the flag for anything but the hypervisor. We can decide at later > time whether we want to relax. > > Also remove the -Wno-declaration-after-statement in some subdirectory > as the flag is now unnecessary. > > Part of the commit message was take from Alexander's first proposal: > > Link: https://lore.kernel.org/xen-devel/20231128174729.3880113-1-alex@linutronix.de/ > Reported-by: Alexander Kanavin <alex@linutronix.de> > Signed-off-by: Julien Grall <jgrall@amazon.com> Tools side of the patch looks good to me, so Acked-by: Anthony PERARD <anthony.perard@citrix.com> Thanks,
On Tue, Dec 5, 2023 at 4:49 PM Julien Grall <julien@xen.org> wrote: > > From: Julien Grall <jgrall@amazon.com> > > Right now, all tools and hypervisor will be complied with the option > -Wdeclaration-after-statement. While most of the code in the hypervisor > is controlled by us, for tools we may import external libraries. > > The build will fail if one of them are using the construct we are > trying to prevent. This is the case when building against Python 3.12 > and Yocto: > > | In file included from /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/Python.h:44, > | from xen/lowlevel/xc/xc.c:8: > | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/object.h: In function 'Py_SIZE': > | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/object.h:233:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] > | 233 | PyVarObject *var_ob = _PyVarObject_CAST(ob); > | | ^~~~~~~~~~~ > | In file included from /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/Python.h:53: > | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/cpython/longintrepr.h: In function '_PyLong_CompactValue': > | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/cpython/longintrepr.h:121:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] > | 121 | Py_ssize_t sign = 1 - (op->long_value.lv_tag & _PyLong_SIGN_MASK); > | | ^~~~~~~~~~ > | cc1: all warnings being treated as errors > > Looking at the tools directory, a fair few directory already add > -Wno-declaration-after-statement to inhibit the default behavior. > > We have always build the hypervisor with the flag, so for now remove > only the flag for anything but the hypervisor. We can decide at later > time whether we want to relax. > > Also remove the -Wno-declaration-after-statement in some subdirectory > as the flag is now unnecessary. > > Part of the commit message was take from Alexander's first proposal: > > Link: https://lore.kernel.org/xen-devel/20231128174729.3880113-1-alex@linutronix.de/ > Reported-by: Alexander Kanavin <alex@linutronix.de> > Signed-off-by: Julien Grall <jgrall@amazon.com> Tested-by: Jason Andryuk <jandryuk@gmail.com> Also needed for compiling on Fedora 39. Thanks, Jason
Hi Jan, On 06/12/2023 11:08, Jan Beulich wrote: > On 06.12.2023 12:07, Julien Grall wrote: >> Hi Jan, >> >> On 06/12/2023 11:05, Jan Beulich wrote: >>> On 06.12.2023 11:52, Julien Grall wrote: >>>> Hi Jan, >>>> >>>> On 06/12/2023 08:52, Jan Beulich wrote: >>>>> On 05.12.2023 19:32, Julien Grall wrote: >>>>>> From: Julien Grall <jgrall@amazon.com> >>>>>> >>>>>> Right now, all tools and hypervisor will be complied with the option >>>>>> -Wdeclaration-after-statement. While most of the code in the hypervisor >>>>>> is controlled by us, for tools we may import external libraries. >>>>>> >>>>>> The build will fail if one of them are using the construct we are >>>>>> trying to prevent. This is the case when building against Python 3.12 >>>>>> and Yocto: >>>>>> >>>>>> | In file included from /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/Python.h:44, >>>>>> | from xen/lowlevel/xc/xc.c:8: >>>>>> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/object.h: In function 'Py_SIZE': >>>>>> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/object.h:233:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] >>>>>> | 233 | PyVarObject *var_ob = _PyVarObject_CAST(ob); >>>>>> | | ^~~~~~~~~~~ >>>>>> | In file included from /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/Python.h:53: >>>>>> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/cpython/longintrepr.h: In function '_PyLong_CompactValue': >>>>>> | /srv/storage/alex/yocto/build-virt/tmp/work/core2-64-poky-linux/xen-tools/4.17+stable/recipe-sysroot/usr/include/python3.12/cpython/longintrepr.h:121:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] >>>>>> | 121 | Py_ssize_t sign = 1 - (op->long_value.lv_tag & _PyLong_SIGN_MASK); >>>>>> | | ^~~~~~~~~~ >>>>>> | cc1: all warnings being treated as errors >>>>>> >>>>>> Looking at the tools directory, a fair few directory already add >>>>>> -Wno-declaration-after-statement to inhibit the default behavior. >>>>>> >>>>>> We have always build the hypervisor with the flag, so for now remove >>>>>> only the flag for anything but the hypervisor. We can decide at later >>>>>> time whether we want to relax. >>>>>> >>>>>> Also remove the -Wno-declaration-after-statement in some subdirectory >>>>>> as the flag is now unnecessary. >>>>> >>>>> With all these removals, don't you need to add the option centrally >>>>> somewhere? Or else are you sure that no compiler version, including >>>>> distro-customized ones, would ever come with the warning enabled by >>>>> default? >>>> >>>> I can't really go through the dozens of different built compilers... But >>>> I would find odd that a compiler will force this option. I view it as a >>>> style enforcement option and that's not up to a distro to decide what >>>> every projects do. >>>> >>>> Also, Allowing your thinking, we would need to add -Wno-switch-default & >>>> co just in case a compiler decide to enable it. So where would we stop >>>> adding -Wno-*? >>>> >>>> I don't think this is very scalable. >>> >>> I agree on this point, but: With the change you do there's a (slim) risk >>> of introducing build breakage. With other -W* / -Wno-* options we haven't >>> had reports of build issues. >> >> The chance is very unlikely here. So I am not in favor of doing this. I >> would like the opinion from the others. > > Well, it's Anthony anyway who has the final say. I'm not going to insist, > I merely wanted to point out a possible issue. Anthony approved it. So I have committed the patch. Cheers,
diff --git a/Config.mk b/Config.mk index 2c43702958eb..7e67b91de293 100644 --- a/Config.mk +++ b/Config.mk @@ -177,8 +177,6 @@ CFLAGS += -std=gnu99 CFLAGS += -Wall -Wstrict-prototypes -$(call cc-option-add,HOSTCFLAGS,HOSTCC,-Wdeclaration-after-statement) -$(call cc-option-add,CFLAGS,CC,-Wdeclaration-after-statement) $(call cc-option-add,CFLAGS,CC,-Wno-unused-but-set-variable) $(call cc-option-add,CFLAGS,CC,-Wno-unused-local-typedefs) diff --git a/stubdom/Makefile b/stubdom/Makefile index 71c9b2200e68..8c503c2bf8de 100644 --- a/stubdom/Makefile +++ b/stubdom/Makefile @@ -245,7 +245,7 @@ tpm_emulator-$(XEN_TARGET_ARCH): tpm_emulator-$(TPMEMU_VERSION).tar.gz patch -d $@ -p1 < vtpm-command-duration.patch patch -d $@ -p1 < vtpm-tpm_bn_t-addr.patch mkdir $@/build - cd $@/build; CC=${CC} $(CMAKE) .. -DCMAKE_C_FLAGS:STRING="-std=c99 -DTPM_NO_EXTERN $(TARGET_CPPFLAGS) $(TARGET_CFLAGS) -Wno-declaration-after-statement" + cd $@/build; CC=${CC} $(CMAKE) .. -DCMAKE_C_FLAGS:STRING="-std=c99 -DTPM_NO_EXTERN $(TARGET_CPPFLAGS) $(TARGET_CFLAGS)" touch $@ TPMEMU_STAMPFILE=$(CROSS_ROOT)/$(GNU_TARGET_ARCH)-xen-elf/lib/libtpm.a diff --git a/stubdom/vtpmmgr/Makefile b/stubdom/vtpmmgr/Makefile index 6dae034a0778..c29bb498381c 100644 --- a/stubdom/vtpmmgr/Makefile +++ b/stubdom/vtpmmgr/Makefile @@ -17,7 +17,7 @@ OBJS += vtpm_disk.o disk_tpm.o disk_io.o disk_crypto.o disk_read.o disk_write.o OBJS += mgmt_authority.o CFLAGS+=-Werror -Iutil -Icrypto -Itcs -CFLAGS+=-Wno-declaration-after-statement -Wno-unused-label +CFLAGS+=-Wno-unused-label build: $(TARGET) $(TARGET): $(OBJS) diff --git a/tools/libs/light/Makefile b/tools/libs/light/Makefile index ba4c1b79336f..37e4d1670986 100644 --- a/tools/libs/light/Makefile +++ b/tools/libs/light/Makefile @@ -38,8 +38,7 @@ vpath static_tables.c $(ACPI_PATH)/ OBJS-$(CONFIG_X86) += $(ACPI_OBJS) -CFLAGS += -Wno-format-zero-length -Wmissing-declarations \ - -Wno-declaration-after-statement -Wformat-nonliteral +CFLAGS += -Wno-format-zero-length -Wmissing-declarations -Wformat-nonliteral CFLAGS-$(CONFIG_X86) += -DCONFIG_PCI_SUPP_LEGACY_IRQ diff --git a/tools/libs/util/Makefile b/tools/libs/util/Makefile index c3b21875dcd8..936ec90a3153 100644 --- a/tools/libs/util/Makefile +++ b/tools/libs/util/Makefile @@ -9,8 +9,7 @@ OBJS-y += libxlu_disk.o OBJS-y += libxlu_vif.o OBJS-y += libxlu_pci.o -CFLAGS += -Wno-format-zero-length -Wmissing-declarations \ - -Wno-declaration-after-statement -Wformat-nonliteral +CFLAGS += -Wno-format-zero-length -Wmissing-declarations -Wformat-nonliteral CFLAGS += $(CFLAGS_libxenctrl) CFLAGS += $(PTHREAD_CFLAGS) diff --git a/tools/tests/depriv/Makefile b/tools/tests/depriv/Makefile index 7d9e3b01bbd4..5404a12f4780 100644 --- a/tools/tests/depriv/Makefile +++ b/tools/tests/depriv/Makefile @@ -1,8 +1,6 @@ XEN_ROOT=$(CURDIR)/../../.. include $(XEN_ROOT)/tools/Rules.mk -CFLAGS += -Wno-declaration-after-statement - CFLAGS += $(CFLAGS_xeninclude) CFLAGS += $(CFLAGS_libxenctrl) CFLAGS += $(CFLAGS_libxencall) diff --git a/tools/xl/Makefile b/tools/xl/Makefile index 5f7aa5f46c87..d742e96a5b3e 100644 --- a/tools/xl/Makefile +++ b/tools/xl/Makefile @@ -5,8 +5,7 @@ XEN_ROOT = $(CURDIR)/../.. include $(XEN_ROOT)/tools/Rules.mk -CFLAGS += -Wno-format-zero-length -Wmissing-declarations \ - -Wno-declaration-after-statement -Wformat-nonliteral +CFLAGS += -Wno-format-zero-length -Wmissing-declarations -Wformat-nonliteral CFLAGS += -fPIC CFLAGS += $(PTHREAD_CFLAGS) diff --git a/xen/Makefile b/xen/Makefile index ca571103c868..360fb6dcae57 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -393,6 +393,7 @@ CFLAGS-$(CONFIG_CC_SPLIT_SECTIONS) += -ffunction-sections -fdata-sections CFLAGS += -nostdinc -fno-builtin -fno-common CFLAGS += -Werror -Wredundant-decls -Wwrite-strings -Wno-pointer-arith +$(call cc-option-add,CFLAGS,CC,-Wdeclaration-after-statement) $(call cc-option-add,CFLAGS,CC,-Wvla) CFLAGS += -pipe -D__XEN__ -include $(srctree)/include/xen/config.h CFLAGS-$(CONFIG_DEBUG_INFO) += -g