From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Huth Subject: Re: [kvm-unit-tests PATCH] Replace -Wextra with a saner list of warning flags Date: Thu, 29 Jun 2017 19:19:42 +0200 Message-ID: References: <1498750826-7902-1-git-send-email-thuth@redhat.com> <03cc1365-c175-53c1-3c92-7ff8b9a29190@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: David Hildenbrand , =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , Laurent Vivier , Drew Jones To: Paolo Bonzini , kvm@vger.kernel.org Return-path: Received: from mx1.redhat.com ([209.132.183.28]:34912 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751893AbdF2RTt (ORCPT ); Thu, 29 Jun 2017 13:19:49 -0400 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 615C97AE97 for ; Thu, 29 Jun 2017 17:19:49 +0000 (UTC) In-Reply-To: <03cc1365-c175-53c1-3c92-7ff8b9a29190@redhat.com> Content-Language: en-US Sender: kvm-owner@vger.kernel.org List-ID: 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 >> --- >> 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