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

[Thomas Huth <thuth@redhat.com>]

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