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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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