[kvm-unit-tests PATCH] Replace -Wextra with a saner list of warning flags

[kvm-unit-tests PATCH] Replace -Wextra with a saner list of warning flags

[Thomas Huth]

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
-- 
1.8.3.1

Re: [kvm-unit-tests PATCH] Replace -Wextra with a saner list of warning flags

[Thomas Huth]

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

Re: [kvm-unit-tests PATCH] Replace -Wextra with a saner list of warning flags

[Andrew Jones]

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

Re: [kvm-unit-tests PATCH] Replace -Wextra with a saner list of warning flags

[Laurent Vivier]

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>

Re: [kvm-unit-tests PATCH] Replace -Wextra with a saner list of warning flags

[Paolo Bonzini]

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

Re: [kvm-unit-tests PATCH] Replace -Wextra with a saner list of warning flags

[Paolo Bonzini]

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

Re: [kvm-unit-tests PATCH] Replace -Wextra with a saner list of warning flags

[Thomas Huth]

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

Re: [kvm-unit-tests PATCH] Replace -Wextra with a saner list of warning flags

[Paolo Bonzini]

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