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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan