1. Patchew
  2. Xen
  3. View series

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

Julien Grall posted 1 patch 4 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20231205183226.26636-1-julien@xen.org
Config.mk                   | 2 --
stubdom/Makefile            | 2 +-
stubdom/vtpmmgr/Makefile    | 2 +-
tools/libs/light/Makefile   | 3 +--
tools/libs/util/Makefile    | 3 +--
tools/tests/depriv/Makefile | 2 --
tools/xl/Makefile           | 3 +--
xen/Makefile                | 1 +
8 files changed, 6 insertions(+), 12 deletions(-)
[PATCH] Only compile the hypervisor with -Wdeclaration-after-statement
Posted by Julien Grall 4 months, 3 weeks ago 
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>

---

I think the decision to remove the flag for the hypervisor is a separate
discussion. Personally I am on the fence.

We could also re-enable the flags to some of the tools directory
if wanted. I chose the most convenience approach for now.
---
 Config.mk                   | 2 --
 stubdom/Makefile            | 2 +-
 stubdom/vtpmmgr/Makefile    | 2 +-
 tools/libs/light/Makefile   | 3 +--
 tools/libs/util/Makefile    | 3 +--
 tools/tests/depriv/Makefile | 2 --
 tools/xl/Makefile           | 3 +--
 xen/Makefile                | 1 +
 8 files changed, 6 insertions(+), 12 deletions(-)

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
-- 
2.40.1
Re: [PATCH] Only compile the hypervisor with -Wdeclaration-after-statement
Posted by Jason Andryuk 4 months, 3 weeks ago 
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
Re: [PATCH] Only compile the hypervisor with -Wdeclaration-after-statement
Posted by Anthony PERARD 4 months, 3 weeks ago 
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,

-- 
Anthony PERARD
Re: [PATCH] Only compile the hypervisor with -Wdeclaration-after-statement
Posted by Jan Beulich 4 months, 3 weeks ago 
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
Re: [PATCH] Only compile the hypervisor with -Wdeclaration-after-statement
Posted by Andrew Cooper 4 months, 3 weeks ago 
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
Re: [PATCH] Only compile the hypervisor with -Wdeclaration-after-statement
Posted by Julien Grall 4 months, 3 weeks ago 
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,

-- 
Julien Grall
Re: [PATCH] Only compile the hypervisor with -Wdeclaration-after-statement
Posted by Jan Beulich 4 months, 3 weeks ago 
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
Re: [PATCH] Only compile the hypervisor with -Wdeclaration-after-statement
Posted by Julien Grall 4 months, 3 weeks ago 
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,

-- 
Julien Grall
Re: [PATCH] Only compile the hypervisor with -Wdeclaration-after-statement
Posted by Jan Beulich 4 months, 3 weeks ago 
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
Re: [PATCH] Only compile the hypervisor with -Wdeclaration-after-statement
Posted by Julien Grall 4 months, 3 weeks ago 
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,

-- 
Julien Grall
Re: [PATCH] Only compile the hypervisor with -Wdeclaration-after-statement
Posted by Andrew Cooper 4 months, 3 weeks ago 
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>
Re: [PATCH] Only compile the hypervisor with -Wdeclaration-after-statement
Posted by Julien Grall 4 months, 3 weeks ago 
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,

-- 
Julien Grall

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.