Re: [PATCH] Only compile the hypervisor with -Wdeclaration-after-statement

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxxxxx>
> > > > > >
> > > > > > 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,

--
Julien Grall