| Message ID | 1498750826-7902-1-git-send-email-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 29.06.2017 17:40, Thomas Huth wrote: > Using -Wextra together with -Werror is troublesome - various versions > of GCC produce suspicious or even wrong warnings with -Wextra which > then become fatal errors with -Werror. For example, the current state > of the kvm-unit-tests does not compile anymore with GCC 4.8.1 for > s390x due to an inadequate -Wmissing-field-initializers warning. > That's annoying for users who just would like to compile the > kvm-unit-tests and cumbersome for the developers who have to work > around these problems in the source code. So let's replace -Wextra > by a saner lists of warning flags that are normally enabled by -Wextra. > Most of them are added to the architecture independent CFLAGS list, > so that x86 now benefits from these checks, too. The ones that > could not be added there are placed in the architecture specific > CFLAGS instead. BTW, I also dropped -Wunused-parameter on purpose. It's often a nuisance that you are forced to add "unused" attributes to parameters, just because you can not get rid of certain parameter since your function has to obey a certain API. That means we could now finally also get rid of the ugly "__unused" tags in the code in the lib folder again, if we like ;-) Thomas
On Thu, Jun 29, 2017 at 06:30:47PM +0200, Thomas Huth wrote: > On 29.06.2017 17:40, Thomas Huth wrote: > > Using -Wextra together with -Werror is troublesome - various versions > > of GCC produce suspicious or even wrong warnings with -Wextra which > > then become fatal errors with -Werror. For example, the current state > > of the kvm-unit-tests does not compile anymore with GCC 4.8.1 for > > s390x due to an inadequate -Wmissing-field-initializers warning. > > That's annoying for users who just would like to compile the > > kvm-unit-tests and cumbersome for the developers who have to work > > around these problems in the source code. So let's replace -Wextra > > by a saner lists of warning flags that are normally enabled by -Wextra. > > Most of them are added to the architecture independent CFLAGS list, > > so that x86 now benefits from these checks, too. The ones that > > could not be added there are placed in the architecture specific > > CFLAGS instead. > > BTW, I also dropped -Wunused-parameter on purpose. It's often a nuisance > that you are forced to add "unused" attributes to parameters, just > because you can not get rid of certain parameter since your function has > to obey a certain API. That means we could now finally also get rid of > the ugly "__unused" tags in the code in the lib folder again, if we like ;-) Fine by me. Adding __unused gets tiresome and ugly. I even recently wrote a patch where I needed to introduce __maybe_unused... Thanks, drew
On 29/06/2017 17:40, Thomas Huth wrote: > Using -Wextra together with -Werror is troublesome - various versions > of GCC produce suspicious or even wrong warnings with -Wextra which > then become fatal errors with -Werror. For example, the current state > of the kvm-unit-tests does not compile anymore with GCC 4.8.1 for > s390x due to an inadequate -Wmissing-field-initializers warning. > That's annoying for users who just would like to compile the > kvm-unit-tests and cumbersome for the developers who have to work > around these problems in the source code. So let's replace -Wextra > by a saner lists of warning flags that are normally enabled by -Wextra. > Most of them are added to the architecture independent CFLAGS list, > so that x86 now benefits from these checks, too. The ones that > could not be added there are placed in the architecture specific > CFLAGS instead. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > Makefile | 5 +++-- > arm/Makefile.common | 3 ++- > powerpc/Makefile.common | 3 ++- > s390x/Makefile | 3 ++- > 4 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/Makefile b/Makefile > index e79cf93..56d2fd7 100644 > --- a/Makefile > +++ b/Makefile > @@ -50,8 +50,9 @@ include $(SRCDIR)/$(TEST_DIR)/Makefile > cc-option = $(shell if $(CC) $(1) -S -o /dev/null -xc /dev/null \ > > /dev/null 2>&1; then echo "$(1)"; else echo "$(2)"; fi ;) > > -CFLAGS += -g > -CFLAGS += $(autodepend-flags) -Wall -Wwrite-strings -Werror > +CFLAGS += -g $(autodepend-flags) > +CFLAGS += -Wall -Wwrite-strings -Wclobbered -Wempty-body -Wignored-qualifiers > +CFLAGS += -Wtype-limits -Wuninitialized -Wunused-but-set-parameter -Werror > frame-pointer-flag=-f$(if $(KEEP_FRAME_POINTER),no-,)omit-frame-pointer > fomit_frame_pointer := $(call cc-option, $(frame-pointer-flag), "") > fnostack_protector := $(call cc-option, -fno-stack-protector, "") > diff --git a/arm/Makefile.common b/arm/Makefile.common > index 03b497b..2840c2a 100644 > --- a/arm/Makefile.common > +++ b/arm/Makefile.common > @@ -24,7 +24,8 @@ phys_base = $(LOADADDR) > > CFLAGS += -std=gnu99 > CFLAGS += -ffreestanding > -CFLAGS += -Wextra > +CFLAGS += -Wmissing-parameter-type -Wold-style-declaration -Woverride-init > +CFLAGS += -Wsign-compare > CFLAGS += -O2 > CFLAGS += -I $(SRCDIR)/lib -I $(SRCDIR)/lib/libfdt -I lib > > diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common > index db5ba62..50c4b24 100644 > --- a/powerpc/Makefile.common > +++ b/powerpc/Makefile.common > @@ -19,7 +19,8 @@ all: directories $(TEST_DIR)/boot_rom.bin $(tests-all) > > CFLAGS += -std=gnu99 > CFLAGS += -ffreestanding > -CFLAGS += -Wextra > +CFLAGS += -Wmissing-parameter-type -Wold-style-declaration -Woverride-init > +CFLAGS += -Wsign-compare > CFLAGS += -O2 > CFLAGS += -I $(SRCDIR)/lib -I $(SRCDIR)/lib/libfdt -I lib > CFLAGS += -Wa,-mregnames > diff --git a/s390x/Makefile b/s390x/Makefile > index 470cbba..3b8f5d9 100644 > --- a/s390x/Makefile > +++ b/s390x/Makefile > @@ -7,7 +7,8 @@ test_cases: $(tests) > > CFLAGS += -std=gnu99 > CFLAGS += -ffreestanding > -CFLAGS += -Wextra > +CFLAGS += -Wmissing-parameter-type -Wold-style-declaration -Woverride-init > +CFLAGS += -Wsign-compare > CFLAGS += -I $(SRCDIR)/lib > CFLAGS += -O2 > CFLAGS += -march=z900 > Reviewed-by: Laurent Vivier <lvivier@redhat.com>
On 29/06/2017 18:35, Andrew Jones wrote: > On Thu, Jun 29, 2017 at 06:30:47PM +0200, Thomas Huth wrote: >> On 29.06.2017 17:40, Thomas Huth wrote: >>> Using -Wextra together with -Werror is troublesome - various versions >>> of GCC produce suspicious or even wrong warnings with -Wextra which >>> then become fatal errors with -Werror. For example, the current state >>> of the kvm-unit-tests does not compile anymore with GCC 4.8.1 for >>> s390x due to an inadequate -Wmissing-field-initializers warning. >>> That's annoying for users who just would like to compile the >>> kvm-unit-tests and cumbersome for the developers who have to work >>> around these problems in the source code. So let's replace -Wextra >>> by a saner lists of warning flags that are normally enabled by -Wextra. >>> Most of them are added to the architecture independent CFLAGS list, >>> so that x86 now benefits from these checks, too. The ones that >>> could not be added there are placed in the architecture specific >>> CFLAGS instead. >> >> BTW, I also dropped -Wunused-parameter on purpose. It's often a nuisance >> that you are forced to add "unused" attributes to parameters, just >> because you can not get rid of certain parameter since your function has >> to obey a certain API. That means we could now finally also get rid of >> the ugly "__unused" tags in the code in the lib folder again, if we like ;-) > > Fine by me. Adding __unused gets tiresome and ugly. I even recently > wrote a patch where I needed to introduce __maybe_unused... Agreed, C should take inspiration from C++ and add int f(int x, int /* unused */) Paolo
On 29/06/2017 17:40, Thomas Huth wrote: > Using -Wextra together with -Werror is troublesome - various versions > of GCC produce suspicious or even wrong warnings with -Wextra which > then become fatal errors with -Werror. For example, the current state > of the kvm-unit-tests does not compile anymore with GCC 4.8.1 for > s390x due to an inadequate -Wmissing-field-initializers warning. > That's annoying for users who just would like to compile the > kvm-unit-tests and cumbersome for the developers who have to work > around these problems in the source code. So let's replace -Wextra > by a saner lists of warning flags that are normally enabled by -Wextra. > Most of them are added to the architecture independent CFLAGS list, > so that x86 now benefits from these checks, too. The ones that > could not be added there are placed in the architecture specific > CFLAGS instead. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > Makefile | 5 +++-- > arm/Makefile.common | 3 ++- > powerpc/Makefile.common | 3 ++- > s390x/Makefile | 3 ++- > 4 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/Makefile b/Makefile > index e79cf93..56d2fd7 100644 > --- a/Makefile > +++ b/Makefile > @@ -50,8 +50,9 @@ include $(SRCDIR)/$(TEST_DIR)/Makefile > cc-option = $(shell if $(CC) $(1) -S -o /dev/null -xc /dev/null \ > > /dev/null 2>&1; then echo "$(1)"; else echo "$(2)"; fi ;) > > -CFLAGS += -g > -CFLAGS += $(autodepend-flags) -Wall -Wwrite-strings -Werror > +CFLAGS += -g $(autodepend-flags) > +CFLAGS += -Wall -Wwrite-strings -Wclobbered -Wempty-body -Wignored-qualifiers > +CFLAGS += -Wtype-limits -Wuninitialized -Wunused-but-set-parameter -Werror > frame-pointer-flag=-f$(if $(KEEP_FRAME_POINTER),no-,)omit-frame-pointer > fomit_frame_pointer := $(call cc-option, $(frame-pointer-flag), "") > fnostack_protector := $(call cc-option, -fno-stack-protector, "") > diff --git a/arm/Makefile.common b/arm/Makefile.common > index 03b497b..2840c2a 100644 > --- a/arm/Makefile.common > +++ b/arm/Makefile.common > @@ -24,7 +24,8 @@ phys_base = $(LOADADDR) > > CFLAGS += -std=gnu99 > CFLAGS += -ffreestanding > -CFLAGS += -Wextra > +CFLAGS += -Wmissing-parameter-type -Wold-style-declaration -Woverride-init > +CFLAGS += -Wsign-compare > CFLAGS += -O2 > CFLAGS += -I $(SRCDIR)/lib -I $(SRCDIR)/lib/libfdt -I lib > > diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common > index db5ba62..50c4b24 100644 > --- a/powerpc/Makefile.common > +++ b/powerpc/Makefile.common > @@ -19,7 +19,8 @@ all: directories $(TEST_DIR)/boot_rom.bin $(tests-all) > > CFLAGS += -std=gnu99 > CFLAGS += -ffreestanding > -CFLAGS += -Wextra > +CFLAGS += -Wmissing-parameter-type -Wold-style-declaration -Woverride-init > +CFLAGS += -Wsign-compare > CFLAGS += -O2 > CFLAGS += -I $(SRCDIR)/lib -I $(SRCDIR)/lib/libfdt -I lib > CFLAGS += -Wa,-mregnames > diff --git a/s390x/Makefile b/s390x/Makefile > index 470cbba..3b8f5d9 100644 > --- a/s390x/Makefile > +++ b/s390x/Makefile > @@ -7,7 +7,8 @@ test_cases: $(tests) > > CFLAGS += -std=gnu99 > CFLAGS += -ffreestanding > -CFLAGS += -Wextra > +CFLAGS += -Wmissing-parameter-type -Wold-style-declaration -Woverride-init > +CFLAGS += -Wsign-compare > CFLAGS += -I $(SRCDIR)/lib > CFLAGS += -O2 > CFLAGS += -march=z900 I am not sure about -Wsign-compare, which can have a lot of false positives. Other opinions? x86 cannot use "Wmissing-parameter-type -Wold-style-declaration -Woverride-init" only because they're not valid in C++. Maybe we should split CFLAGS into COMMON_CCFLAGS and CFLAGS proper, so that CXXFLAGS and LDFLAGS can be assigned with CXXFLAGS += $(COMMON_CCFLAGS) LDFLAGS += $(COMMON_CCFLAGS) Then those three could go in CFLAGS, and the others in COMMON_CCFLAGS. Thanks, Paolo
On 29.06.2017 19:07, Paolo Bonzini wrote: > > > On 29/06/2017 17:40, Thomas Huth wrote: >> Using -Wextra together with -Werror is troublesome - various versions >> of GCC produce suspicious or even wrong warnings with -Wextra which >> then become fatal errors with -Werror. For example, the current state >> of the kvm-unit-tests does not compile anymore with GCC 4.8.1 for >> s390x due to an inadequate -Wmissing-field-initializers warning. >> That's annoying for users who just would like to compile the >> kvm-unit-tests and cumbersome for the developers who have to work >> around these problems in the source code. So let's replace -Wextra >> by a saner lists of warning flags that are normally enabled by -Wextra. >> Most of them are added to the architecture independent CFLAGS list, >> so that x86 now benefits from these checks, too. The ones that >> could not be added there are placed in the architecture specific >> CFLAGS instead. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> Makefile | 5 +++-- >> arm/Makefile.common | 3 ++- >> powerpc/Makefile.common | 3 ++- >> s390x/Makefile | 3 ++- >> 4 files changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/Makefile b/Makefile >> index e79cf93..56d2fd7 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -50,8 +50,9 @@ include $(SRCDIR)/$(TEST_DIR)/Makefile >> cc-option = $(shell if $(CC) $(1) -S -o /dev/null -xc /dev/null \ >> > /dev/null 2>&1; then echo "$(1)"; else echo "$(2)"; fi ;) >> >> -CFLAGS += -g >> -CFLAGS += $(autodepend-flags) -Wall -Wwrite-strings -Werror >> +CFLAGS += -g $(autodepend-flags) >> +CFLAGS += -Wall -Wwrite-strings -Wclobbered -Wempty-body -Wignored-qualifiers >> +CFLAGS += -Wtype-limits -Wuninitialized -Wunused-but-set-parameter -Werror >> frame-pointer-flag=-f$(if $(KEEP_FRAME_POINTER),no-,)omit-frame-pointer >> fomit_frame_pointer := $(call cc-option, $(frame-pointer-flag), "") >> fnostack_protector := $(call cc-option, -fno-stack-protector, "") >> diff --git a/arm/Makefile.common b/arm/Makefile.common >> index 03b497b..2840c2a 100644 >> --- a/arm/Makefile.common >> +++ b/arm/Makefile.common >> @@ -24,7 +24,8 @@ phys_base = $(LOADADDR) >> >> CFLAGS += -std=gnu99 >> CFLAGS += -ffreestanding >> -CFLAGS += -Wextra >> +CFLAGS += -Wmissing-parameter-type -Wold-style-declaration -Woverride-init >> +CFLAGS += -Wsign-compare >> CFLAGS += -O2 >> CFLAGS += -I $(SRCDIR)/lib -I $(SRCDIR)/lib/libfdt -I lib >> >> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common >> index db5ba62..50c4b24 100644 >> --- a/powerpc/Makefile.common >> +++ b/powerpc/Makefile.common >> @@ -19,7 +19,8 @@ all: directories $(TEST_DIR)/boot_rom.bin $(tests-all) >> >> CFLAGS += -std=gnu99 >> CFLAGS += -ffreestanding >> -CFLAGS += -Wextra >> +CFLAGS += -Wmissing-parameter-type -Wold-style-declaration -Woverride-init >> +CFLAGS += -Wsign-compare >> CFLAGS += -O2 >> CFLAGS += -I $(SRCDIR)/lib -I $(SRCDIR)/lib/libfdt -I lib >> CFLAGS += -Wa,-mregnames >> diff --git a/s390x/Makefile b/s390x/Makefile >> index 470cbba..3b8f5d9 100644 >> --- a/s390x/Makefile >> +++ b/s390x/Makefile >> @@ -7,7 +7,8 @@ test_cases: $(tests) >> >> CFLAGS += -std=gnu99 >> CFLAGS += -ffreestanding >> -CFLAGS += -Wextra >> +CFLAGS += -Wmissing-parameter-type -Wold-style-declaration -Woverride-init >> +CFLAGS += -Wsign-compare >> CFLAGS += -I $(SRCDIR)/lib >> CFLAGS += -O2 >> CFLAGS += -march=z900 > > I am not sure about -Wsign-compare, which can have a lot of false > positives. Other opinions? I'm fine if we drop it - I also had to do a lot of boring casting due to this option in other projects... sometimes it helps to find bugs, but often it's rather annoying. > x86 cannot use "Wmissing-parameter-type -Wold-style-declaration > -Woverride-init" only because they're not valid in C++. Maybe we should > split CFLAGS into COMMON_CCFLAGS and CFLAGS proper, so that CXXFLAGS and > LDFLAGS can be assigned with > > CXXFLAGS += $(COMMON_CCFLAGS) > LDFLAGS += $(COMMON_CCFLAGS) > > Then those three could go in CFLAGS, and the others in COMMON_CCFLAGS. That's of course a good idea. I'll send a v2 ... Thomas
On 29/06/2017 19:19, Thomas Huth wrote: > >> x86 cannot use "Wmissing-parameter-type -Wold-style-declaration >> -Woverride-init" only because they're not valid in C++. Maybe we should >> split CFLAGS into COMMON_CCFLAGS and CFLAGS proper, so that CXXFLAGS and >> LDFLAGS can be assigned with >> >> CXXFLAGS += $(COMMON_CCFLAGS) >> LDFLAGS += $(COMMON_CCFLAGS) >> >> Then those three could go in CFLAGS, and the others in COMMON_CCFLAGS. > That's of course a good idea. I'll send a v2 ... FWIW, I also removed -Wtype-limits when applying. Checking against symbolic values that turn out to be zero is pretty common. Paolo
diff --git a/Makefile b/Makefile index e79cf93..56d2fd7 100644 --- a/Makefile +++ b/Makefile @@ -50,8 +50,9 @@ include $(SRCDIR)/$(TEST_DIR)/Makefile cc-option = $(shell if $(CC) $(1) -S -o /dev/null -xc /dev/null \ > /dev/null 2>&1; then echo "$(1)"; else echo "$(2)"; fi ;) -CFLAGS += -g -CFLAGS += $(autodepend-flags) -Wall -Wwrite-strings -Werror +CFLAGS += -g $(autodepend-flags) +CFLAGS += -Wall -Wwrite-strings -Wclobbered -Wempty-body -Wignored-qualifiers +CFLAGS += -Wtype-limits -Wuninitialized -Wunused-but-set-parameter -Werror frame-pointer-flag=-f$(if $(KEEP_FRAME_POINTER),no-,)omit-frame-pointer fomit_frame_pointer := $(call cc-option, $(frame-pointer-flag), "") fnostack_protector := $(call cc-option, -fno-stack-protector, "") diff --git a/arm/Makefile.common b/arm/Makefile.common index 03b497b..2840c2a 100644 --- a/arm/Makefile.common +++ b/arm/Makefile.common @@ -24,7 +24,8 @@ phys_base = $(LOADADDR) CFLAGS += -std=gnu99 CFLAGS += -ffreestanding -CFLAGS += -Wextra +CFLAGS += -Wmissing-parameter-type -Wold-style-declaration -Woverride-init +CFLAGS += -Wsign-compare CFLAGS += -O2 CFLAGS += -I $(SRCDIR)/lib -I $(SRCDIR)/lib/libfdt -I lib diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common index db5ba62..50c4b24 100644 --- a/powerpc/Makefile.common +++ b/powerpc/Makefile.common @@ -19,7 +19,8 @@ all: directories $(TEST_DIR)/boot_rom.bin $(tests-all) CFLAGS += -std=gnu99 CFLAGS += -ffreestanding -CFLAGS += -Wextra +CFLAGS += -Wmissing-parameter-type -Wold-style-declaration -Woverride-init +CFLAGS += -Wsign-compare CFLAGS += -O2 CFLAGS += -I $(SRCDIR)/lib -I $(SRCDIR)/lib/libfdt -I lib CFLAGS += -Wa,-mregnames diff --git a/s390x/Makefile b/s390x/Makefile index 470cbba..3b8f5d9 100644 --- a/s390x/Makefile +++ b/s390x/Makefile @@ -7,7 +7,8 @@ test_cases: $(tests) CFLAGS += -std=gnu99 CFLAGS += -ffreestanding -CFLAGS += -Wextra +CFLAGS += -Wmissing-parameter-type -Wold-style-declaration -Woverride-init +CFLAGS += -Wsign-compare CFLAGS += -I $(SRCDIR)/lib CFLAGS += -O2 CFLAGS += -march=z900
Using -Wextra together with -Werror is troublesome - various versions of GCC produce suspicious or even wrong warnings with -Wextra which then become fatal errors with -Werror. For example, the current state of the kvm-unit-tests does not compile anymore with GCC 4.8.1 for s390x due to an inadequate -Wmissing-field-initializers warning. That's annoying for users who just would like to compile the kvm-unit-tests and cumbersome for the developers who have to work around these problems in the source code. So let's replace -Wextra by a saner lists of warning flags that are normally enabled by -Wextra. Most of them are added to the architecture independent CFLAGS list, so that x86 now benefits from these checks, too. The ones that could not be added there are placed in the architecture specific CFLAGS instead. Signed-off-by: Thomas Huth <thuth@redhat.com> --- Makefile | 5 +++-- arm/Makefile.common | 3 ++- powerpc/Makefile.common | 3 ++- s390x/Makefile | 3 ++- 4 files changed, 9 insertions(+), 5 deletions(-)