| Message ID | 1482263220-3233-2-git-send-email-alistair.francis@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/20/16 1:46 PM, Alistair Francis wrote: > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > --- > Config.mk | 2 +- > tools/blktap2/drivers/Makefile | 1 - > tools/libxl/Makefile | 2 +- > tools/xentrace/Makefile | 2 -- > 4 files changed, 2 insertions(+), 5 deletions(-) > > diff --git a/Config.mk b/Config.mk > index 3ec7367..e3cda81 100644 > --- a/Config.mk > +++ b/Config.mk > @@ -34,7 +34,7 @@ CONFIG_$(XEN_OS) := y > SHELL ?= /bin/sh > > # Tools to run on system hosting the build > -HOSTCFLAGS = -Wall -Werror -Wstrict-prototypes -O2 -fomit-frame-pointer > +HOSTCFLAGS = -Wall -Wstrict-prototypes -O2 -fomit-frame-pointer > HOSTCFLAGS += -fno-strict-aliasing > > DISTDIR ?= $(XEN_ROOT)/dist > diff --git a/tools/blktap2/drivers/Makefile b/tools/blktap2/drivers/Makefile > index 5328c40..7a62a3f 100644 > --- a/tools/blktap2/drivers/Makefile > +++ b/tools/blktap2/drivers/Makefile > @@ -9,7 +9,6 @@ QCOW_UTIL = img2qcow qcow-create qcow2raw > LOCK_UTIL = lock-util > INST_DIR = $(sbindir) > > -CFLAGS += -Werror > CFLAGS += -Wno-unused > CFLAGS += -fno-strict-aliasing > CFLAGS += -I$(BLKTAP_ROOT)/include -I$(BLKTAP_ROOT)/drivers > diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile > index 91e2f97..e8a37ef 100644 > --- a/tools/libxl/Makefile > +++ b/tools/libxl/Makefile > @@ -11,7 +11,7 @@ MINOR = 0 > XLUMAJOR = 4.9 > XLUMINOR = 0 > > -CFLAGS += -Werror -Wno-format-zero-length -Wmissing-declarations \ > +CFLAGS += -Wno-format-zero-length -Wmissing-declarations \ > -Wno-declaration-after-statement -Wformat-nonliteral > CFLAGS += -I. -fPIC > > diff --git a/tools/xentrace/Makefile b/tools/xentrace/Makefile > index c8c36a8..ac5c534 100644 > --- a/tools/xentrace/Makefile > +++ b/tools/xentrace/Makefile > @@ -1,8 +1,6 @@ > XEN_ROOT=$(CURDIR)/../.. > include $(XEN_ROOT)/tools/Rules.mk > > -CFLAGS += -Werror > - > CFLAGS += $(CFLAGS_libxenevtchn) > CFLAGS += $(CFLAGS_libxenctrl) > LDLIBS += $(LDLIBS_libxenevtchn) > Can you pass -Wno-error in EXTRA_CFLAGS from 2/5? Wish I could take credit for the idea but it was all Andrew Cooper's.
On 20/12/2016 20:06, Doug Goldstein wrote: > On 12/20/16 1:46 PM, Alistair Francis wrote: >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >> --- >> Config.mk | 2 +- >> tools/blktap2/drivers/Makefile | 1 - >> tools/libxl/Makefile | 2 +- >> tools/xentrace/Makefile | 2 -- >> 4 files changed, 2 insertions(+), 5 deletions(-) >> >> diff --git a/Config.mk b/Config.mk >> index 3ec7367..e3cda81 100644 >> --- a/Config.mk >> +++ b/Config.mk >> @@ -34,7 +34,7 @@ CONFIG_$(XEN_OS) := y >> SHELL ?= /bin/sh >> >> # Tools to run on system hosting the build >> -HOSTCFLAGS = -Wall -Werror -Wstrict-prototypes -O2 -fomit-frame-pointer >> +HOSTCFLAGS = -Wall -Wstrict-prototypes -O2 -fomit-frame-pointer >> HOSTCFLAGS += -fno-strict-aliasing >> >> DISTDIR ?= $(XEN_ROOT)/dist >> diff --git a/tools/blktap2/drivers/Makefile b/tools/blktap2/drivers/Makefile >> index 5328c40..7a62a3f 100644 >> --- a/tools/blktap2/drivers/Makefile >> +++ b/tools/blktap2/drivers/Makefile >> @@ -9,7 +9,6 @@ QCOW_UTIL = img2qcow qcow-create qcow2raw >> LOCK_UTIL = lock-util >> INST_DIR = $(sbindir) >> >> -CFLAGS += -Werror >> CFLAGS += -Wno-unused >> CFLAGS += -fno-strict-aliasing >> CFLAGS += -I$(BLKTAP_ROOT)/include -I$(BLKTAP_ROOT)/drivers >> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile >> index 91e2f97..e8a37ef 100644 >> --- a/tools/libxl/Makefile >> +++ b/tools/libxl/Makefile >> @@ -11,7 +11,7 @@ MINOR = 0 >> XLUMAJOR = 4.9 >> XLUMINOR = 0 >> >> -CFLAGS += -Werror -Wno-format-zero-length -Wmissing-declarations \ >> +CFLAGS += -Wno-format-zero-length -Wmissing-declarations \ >> -Wno-declaration-after-statement -Wformat-nonliteral >> CFLAGS += -I. -fPIC >> >> diff --git a/tools/xentrace/Makefile b/tools/xentrace/Makefile >> index c8c36a8..ac5c534 100644 >> --- a/tools/xentrace/Makefile >> +++ b/tools/xentrace/Makefile >> @@ -1,8 +1,6 @@ >> XEN_ROOT=$(CURDIR)/../.. >> include $(XEN_ROOT)/tools/Rules.mk >> >> -CFLAGS += -Werror >> - >> CFLAGS += $(CFLAGS_libxenevtchn) >> CFLAGS += $(CFLAGS_libxenctrl) >> LDLIBS += $(LDLIBS_libxenevtchn) >> > Can you pass -Wno-error in EXTRA_CFLAGS from 2/5? Wish I could take > credit for the idea but it was all Andrew Cooper's. The point is that, especially with kernel-level development, almost all warnings are relevant to correctness. I have only seen 2? false positives in 5 years, and have lost count of how many issues -Werror has caught before the code actually got committed. ~Andrew
On Tue, Dec 20, 2016 at 12:16 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 20/12/2016 20:06, Doug Goldstein wrote: >> On 12/20/16 1:46 PM, Alistair Francis wrote: >>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >>> --- >>> Config.mk | 2 +- >>> tools/blktap2/drivers/Makefile | 1 - >>> tools/libxl/Makefile | 2 +- >>> tools/xentrace/Makefile | 2 -- >>> 4 files changed, 2 insertions(+), 5 deletions(-) >>> >>> diff --git a/Config.mk b/Config.mk >>> index 3ec7367..e3cda81 100644 >>> --- a/Config.mk >>> +++ b/Config.mk >>> @@ -34,7 +34,7 @@ CONFIG_$(XEN_OS) := y >>> SHELL ?= /bin/sh >>> >>> # Tools to run on system hosting the build >>> -HOSTCFLAGS = -Wall -Werror -Wstrict-prototypes -O2 -fomit-frame-pointer >>> +HOSTCFLAGS = -Wall -Wstrict-prototypes -O2 -fomit-frame-pointer >>> HOSTCFLAGS += -fno-strict-aliasing >>> >>> DISTDIR ?= $(XEN_ROOT)/dist >>> diff --git a/tools/blktap2/drivers/Makefile b/tools/blktap2/drivers/Makefile >>> index 5328c40..7a62a3f 100644 >>> --- a/tools/blktap2/drivers/Makefile >>> +++ b/tools/blktap2/drivers/Makefile >>> @@ -9,7 +9,6 @@ QCOW_UTIL = img2qcow qcow-create qcow2raw >>> LOCK_UTIL = lock-util >>> INST_DIR = $(sbindir) >>> >>> -CFLAGS += -Werror >>> CFLAGS += -Wno-unused >>> CFLAGS += -fno-strict-aliasing >>> CFLAGS += -I$(BLKTAP_ROOT)/include -I$(BLKTAP_ROOT)/drivers >>> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile >>> index 91e2f97..e8a37ef 100644 >>> --- a/tools/libxl/Makefile >>> +++ b/tools/libxl/Makefile >>> @@ -11,7 +11,7 @@ MINOR = 0 >>> XLUMAJOR = 4.9 >>> XLUMINOR = 0 >>> >>> -CFLAGS += -Werror -Wno-format-zero-length -Wmissing-declarations \ >>> +CFLAGS += -Wno-format-zero-length -Wmissing-declarations \ >>> -Wno-declaration-after-statement -Wformat-nonliteral >>> CFLAGS += -I. -fPIC >>> >>> diff --git a/tools/xentrace/Makefile b/tools/xentrace/Makefile >>> index c8c36a8..ac5c534 100644 >>> --- a/tools/xentrace/Makefile >>> +++ b/tools/xentrace/Makefile >>> @@ -1,8 +1,6 @@ >>> XEN_ROOT=$(CURDIR)/../.. >>> include $(XEN_ROOT)/tools/Rules.mk >>> >>> -CFLAGS += -Werror >>> - >>> CFLAGS += $(CFLAGS_libxenevtchn) >>> CFLAGS += $(CFLAGS_libxenctrl) >>> LDLIBS += $(LDLIBS_libxenevtchn) >>> >> Can you pass -Wno-error in EXTRA_CFLAGS from 2/5? Wish I could take >> credit for the idea but it was all Andrew Cooper's. I tried that, I still see errors when building lower levels like tools/xentrace. > > The point is that, especially with kernel-level development, almost all > warnings are relevant to correctness. I have only seen 2? false > positives in 5 years, and have lost count of how many issues -Werror has > caught before the code actually got committed. I agree, but the problem is that as warnings change these cause all sorts of build issues. Thanks, Alistair > > ~Andrew >
On 12/20/16 2:16 PM, Andrew Cooper wrote: > On 20/12/2016 20:06, Doug Goldstein wrote: >> On 12/20/16 1:46 PM, Alistair Francis wrote: >>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >>> --- >>> Config.mk | 2 +- >>> tools/blktap2/drivers/Makefile | 1 - >>> tools/libxl/Makefile | 2 +- >>> tools/xentrace/Makefile | 2 -- >>> 4 files changed, 2 insertions(+), 5 deletions(-) >>> >>> diff --git a/Config.mk b/Config.mk >>> index 3ec7367..e3cda81 100644 >>> --- a/Config.mk >>> +++ b/Config.mk >>> @@ -34,7 +34,7 @@ CONFIG_$(XEN_OS) := y >>> SHELL ?= /bin/sh >>> >>> # Tools to run on system hosting the build >>> -HOSTCFLAGS = -Wall -Werror -Wstrict-prototypes -O2 -fomit-frame-pointer >>> +HOSTCFLAGS = -Wall -Wstrict-prototypes -O2 -fomit-frame-pointer >>> HOSTCFLAGS += -fno-strict-aliasing >>> >>> DISTDIR ?= $(XEN_ROOT)/dist >>> diff --git a/tools/blktap2/drivers/Makefile b/tools/blktap2/drivers/Makefile >>> index 5328c40..7a62a3f 100644 >>> --- a/tools/blktap2/drivers/Makefile >>> +++ b/tools/blktap2/drivers/Makefile >>> @@ -9,7 +9,6 @@ QCOW_UTIL = img2qcow qcow-create qcow2raw >>> LOCK_UTIL = lock-util >>> INST_DIR = $(sbindir) >>> >>> -CFLAGS += -Werror >>> CFLAGS += -Wno-unused >>> CFLAGS += -fno-strict-aliasing >>> CFLAGS += -I$(BLKTAP_ROOT)/include -I$(BLKTAP_ROOT)/drivers >>> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile >>> index 91e2f97..e8a37ef 100644 >>> --- a/tools/libxl/Makefile >>> +++ b/tools/libxl/Makefile >>> @@ -11,7 +11,7 @@ MINOR = 0 >>> XLUMAJOR = 4.9 >>> XLUMINOR = 0 >>> >>> -CFLAGS += -Werror -Wno-format-zero-length -Wmissing-declarations \ >>> +CFLAGS += -Wno-format-zero-length -Wmissing-declarations \ >>> -Wno-declaration-after-statement -Wformat-nonliteral >>> CFLAGS += -I. -fPIC >>> >>> diff --git a/tools/xentrace/Makefile b/tools/xentrace/Makefile >>> index c8c36a8..ac5c534 100644 >>> --- a/tools/xentrace/Makefile >>> +++ b/tools/xentrace/Makefile >>> @@ -1,8 +1,6 @@ >>> XEN_ROOT=$(CURDIR)/../.. >>> include $(XEN_ROOT)/tools/Rules.mk >>> >>> -CFLAGS += -Werror >>> - >>> CFLAGS += $(CFLAGS_libxenevtchn) >>> CFLAGS += $(CFLAGS_libxenctrl) >>> LDLIBS += $(LDLIBS_libxenevtchn) >>> >> Can you pass -Wno-error in EXTRA_CFLAGS from 2/5? Wish I could take >> credit for the idea but it was all Andrew Cooper's. > > The point is that, especially with kernel-level development, almost all > warnings are relevant to correctness. I have only seen 2? false > positives in 5 years, and have lost count of how many issues -Werror has > caught before the code actually got committed. > > ~Andrew > I agree with you Andrew that its good for kernel/hypervisor development. I also agree its good for userspace code prior to being committed or part of a CI loop. Where I wish there was a switch to flip it off is on user space code that pulls in headers from third party packages like tools/.
Doug Goldstein writes ("Re: [Xen-devel] [PATCH v2 1/5] Remove hardcoded strict -Werror checking"):
> I agree with you Andrew that its good for kernel/hypervisor development.
> I also agree its good for userspace code prior to being committed or
> part of a CI loop. Where I wish there was a switch to flip it off is on
> user space code that pulls in headers from third party packages like tools/.
APPEND_CFLAGS=-Wno-error
Ian.
>>> On 20.12.16 at 20:46, <alistair.francis@xilinx.com> wrote: > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> Without some rationale given I don't think such changes are acceptable at all. And then, as already pointed out others, the use of -Werror is there not just for fun. If anything I think an override to that default could be acceptable. Jan
On Thu, Dec 22, 2016 at 12:41 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 20.12.16 at 20:46, <alistair.francis@xilinx.com> wrote: >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> > > Without some rationale given I don't think such changes are > acceptable at all. And then, as already pointed out others, the > use of -Werror is there not just for fun. If anything I think an > override to that default could be acceptable. Unfortunately the APPEND_CFLAGS=-Wno-error doesn't fix all the issues as I still see warnings/errors when building: tools/kconfig/conf.c. I understand that you want it in by default. Everyone seems fairly open to an override. Is a environment variable, which if set will disable Werror acceptable? Something like NO_ERROR=Y which will result in no -Werror being appended. Thanks, Alistair > > Jan >
Alistair Francis writes ("Re: [Xen-devel] [PATCH v2 1/5] Remove hardcoded strict -Werror checking"):
> On Thu, Dec 22, 2016 at 12:41 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> On 20.12.16 at 20:46, <alistair.francis@xilinx.com> wrote:
> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> >
> > Without some rationale given I don't think such changes are
> > acceptable at all. And then, as already pointed out others, the
> > use of -Werror is there not just for fun. If anything I think an
> > override to that default could be acceptable.
>
> Unfortunately the APPEND_CFLAGS=-Wno-error doesn't fix all the issues
> as I still see warnings/errors when building: tools/kconfig/conf.c.
That sounds like a bug to me. Do you know why it's not effective
there ?
> Everyone seems fairly open to an override. Is a environment variable,
> which if set will disable Werror acceptable? Something like NO_ERROR=Y
> which will result in no -Werror being appended.
Yes, an environment variable would be acceptable, but it should have
the right name and semantics and ideally we could reuse an existing
variable or fix it if it is broken.
How about `APPEND_CFLAGS=-Wno-error' ? :-)
Thanks,
Ian.
On Thu, Dec 22, 2016 at 11:22 AM, Ian Jackson <ian.jackson@eu.citrix.com> wrote: > Alistair Francis writes ("Re: [Xen-devel] [PATCH v2 1/5] Remove hardcoded strict -Werror checking"): >> On Thu, Dec 22, 2016 at 12:41 AM, Jan Beulich <JBeulich@suse.com> wrote: >>> On 20.12.16 at 20:46, <alistair.francis@xilinx.com> wrote: >> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >> > >> > Without some rationale given I don't think such changes are >> > acceptable at all. And then, as already pointed out others, the >> > use of -Werror is there not just for fun. If anything I think an >> > override to that default could be acceptable. >> >> Unfortunately the APPEND_CFLAGS=-Wno-error doesn't fix all the issues >> as I still see warnings/errors when building: tools/kconfig/conf.c. > > That sounds like a bug to me. Do you know why it's not effective > there ? It actually might be an issue in the way buildroot is handling the arguments. I'll look into it and see what I find after the holidays. Thanks, Alistair > >> Everyone seems fairly open to an override. Is a environment variable, >> which if set will disable Werror acceptable? Something like NO_ERROR=Y >> which will result in no -Werror being appended. > > Yes, an environment variable would be acceptable, but it should have > the right name and semantics and ideally we could reuse an existing > variable or fix it if it is broken. > > How about `APPEND_CFLAGS=-Wno-error' ? :-) > > Thanks, > Ian. > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > https://lists.xen.org/xen-devel
On Thu, Dec 22, 2016 at 1:12 PM, Alistair Francis <alistair.francis@xilinx.com> wrote: > On Thu, Dec 22, 2016 at 11:22 AM, Ian Jackson <ian.jackson@eu.citrix.com> wrote: >> Alistair Francis writes ("Re: [Xen-devel] [PATCH v2 1/5] Remove hardcoded strict -Werror checking"): >>> On Thu, Dec 22, 2016 at 12:41 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 20.12.16 at 20:46, <alistair.francis@xilinx.com> wrote: >>> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >>> > >>> > Without some rationale given I don't think such changes are >>> > acceptable at all. And then, as already pointed out others, the >>> > use of -Werror is there not just for fun. If anything I think an >>> > override to that default could be acceptable. >>> >>> Unfortunately the APPEND_CFLAGS=-Wno-error doesn't fix all the issues >>> as I still see warnings/errors when building: tools/kconfig/conf.c. >> >> That sounds like a bug to me. Do you know why it's not effective >> there ? > > It actually might be an issue in the way buildroot is handling the arguments. > > I'll look into it and see what I find after the holidays. Nope, it does look like a Xen build issue. I included the full failing log below: PATH="/work/alistai/software/buildroot/output/host/bin:/work/alistai/software/buildroot/output/host/sbin:/work/alistai/software/buildroot/output/host/usr/bin:/work/alistai/software/buildroot/output/host/usr/sbin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin" XEN_TARGET_ARCH=arm32 CROSS_COMPILE=/work/alistai/software/buildroot/output/host/usr/bin/arm-linux- PATH="/work/alistai/software/buildroot/output/host/bin:/work/alistai/software/buildroot/output/host/sbin:/work/alistai/software/buildroot/output/host/usr/bin:/work/alistai/software/buildroot/output/host/usr/sbin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin" AR="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-ar" AS="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-as" LD="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-ld" NM="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-nm" CC="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-gcc" GCC="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-gcc" CPP="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-cpp" CXX="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-g++" FC="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-gfortran" F77="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-gfortran" RANLIB="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-ranlib" READELF="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-readelf" STRIP="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-strip" OBJCOPY="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-objcopy" OBJDUMP="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-objdump" AR_FOR_BUILD="/usr/bin/ar" AS_FOR_BUILD="/usr/bin/as" CC_FOR_BUILD="/usr/bin/gcc" GCC_FOR_BUILD="/usr/bin/gcc" CXX_FOR_BUILD="/usr/bin/g++" LD_FOR_BUILD="/usr/bin/ld" CPPFLAGS_FOR_BUILD="-I/work/alistai/software/buildroot/output/host/usr/include" CFLAGS_FOR_BUILD="-O2 -I/work/alistai/software/buildroot/output/host/usr/include" CXXFLAGS_FOR_BUILD="-O2 -I/work/alistai/software/buildroot/output/host/usr/include" LDFLAGS_FOR_BUILD="-L/work/alistai/software/buildroot/output/host/lib -L/work/alistai/software/buildroot/output/host/usr/lib -Wl,-rpath,/work/alistai/software/buildroot/output/host/usr/lib" FCFLAGS_FOR_BUILD="" DEFAULT_ASSEMBLER="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-as" DEFAULT_LINKER="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-ld" CPPFLAGS="-D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64" APPEND_CFLAGS="-Wno-error -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -Os " CXXFLAGS="-D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -Os " LDFLAGS="" FAPPEND_CFLAGS="-Wno-error -Os " FFLAGS=" -Os " PKG_CONFIG="/work/alistai/software/buildroot/output/host/usr/bin/pkg-config" STAGING_DIR="/work/alistai/software/buildroot/output/host/usr/arm-buildroot-linux-musleabihf/sysroot" INTLTOOL_PERL=/usr/bin/perl /usr/bin/make -j11 dist-xen dist-tools -C /work/alistai/software/buildroot/output/build/xen-4.8.0/ make[1]: Entering directory '/work/alistai/software/buildroot/output/build/xen-4.8.0' /usr/bin/make -C xen install /usr/bin/make -C tools install make[2]: Entering directory '/work/alistai/software/buildroot/output/build/xen-4.8.0/xen' make[2]: Entering directory '/work/alistai/software/buildroot/output/build/xen-4.8.0/tools' /usr/bin/make -f /work/alistai/software/buildroot/output/build/xen-4.8.0/xen/tools/kconfig/Makefile.kconfig ARCH=arm32 SRCARCH=arm HOSTCC="/usr/bin/gcc" HOSTCXX="/usr/bin/g++" defconfig make[3]: Entering directory '/work/alistai/software/buildroot/output/build/xen-4.8.0/xen' make[3]: Entering directory '/work/alistai/software/buildroot/output/build/xen-4.8.0/tools' /usr/bin/gcc -Wp,-MD,tools/kconfig/.conf.o.d -Wall -Werror -Wstrict-prototypes -O2 -fomit-frame-pointer -fno-strict-aliasing -Wdeclaration-after-statement -DCURSES_LOC="<ncurses.h>" -DNCURSES_WIDECHAR=1 -DLOCALE -DKBUILD_NO_NLS -c -o tools/kconfig/conf.o tools/kconfig/conf.c /usr/bin/gcc -Wp,-MD,tools/kconfig/.zconf.tab.o.d -Wall -Werror -Wstrict-prototypes -O2 -fomit-frame-pointer -fno-strict-aliasing -Wdeclaration-after-statement -DCURSES_LOC="<ncurses.h>" -DNCURSES_WIDECHAR=1 -DLOCALE -DKBUILD_NO_NLS -Itools/kconfig -c -o tools/kconfig/zconf.tab.o tools/kconfig/zconf.tab.c tools/kconfig/conf.c: In function ‘check_stdin’: tools/kconfig/conf.c:77:3: error: format not a string literal and no format arguments [-Werror=format-security] printf(_("aborted!\n\n")); ^ tools/kconfig/conf.c:78:3: error: format not a string literal and no format arguments [-Werror=format-security] printf(_("Console input/output is redirected. ")); ^ tools/kconfig/conf.c:79:3: error: format not a string literal and no format arguments [-Werror=format-security] printf(_("Run 'make oldconfig' to update configuration.\n\n")); ^ tools/kconfig/conf.c: In function ‘conf_askvalue’: tools/kconfig/conf.c:89:3: error: format not a string literal and no format arguments [-Werror=format-security] printf(_("(NEW) ")); ^ tools/kconfig/conf.c: In function ‘conf_choice’: tools/kconfig/conf.c:290:5: error: format not a string literal and no format arguments [-Werror=format-security] printf(_(" (NEW)")); ^ tools/kconfig/conf.c: In function ‘check_conf’: tools/kconfig/conf.c:438:6: error: format not a string literal and no format arguments [-Werror=format-security] printf(_("*\n* Restart config...\n*\n")); ^ tools/kconfig/conf.c: In function ‘main’: tools/kconfig/conf.c:640:6: error: format not a string literal and no format arguments [-Werror=format-security] _("\n*** The configuration requires explicit update.\n\n")); ^ tools/kconfig/conf.c:693:4: error: format not a string literal and no format arguments [-Werror=format-security] fprintf(stderr, _("\n*** Error during writing of the configuration.\n\n")); ^ tools/kconfig/conf.c:697:4: error: format not a string literal and no format arguments [-Werror=format-security] fprintf(stderr, _("\n*** Error during update of the configuration.\n\n")); ^ tools/kconfig/conf.c:708:4: error: format not a string literal and no format arguments [-Werror=format-security] fprintf(stderr, _("\n*** Error during writing of the configuration.\n\n")); > > Thanks, > > Alistair > >> >>> Everyone seems fairly open to an override. Is a environment variable, >>> which if set will disable Werror acceptable? Something like NO_ERROR=Y >>> which will result in no -Werror being appended. >> >> Yes, an environment variable would be acceptable, but it should have >> the right name and semantics and ideally we could reuse an existing >> variable or fix it if it is broken. >> >> How about `APPEND_CFLAGS=-Wno-error' ? :-) >> >> Thanks, >> Ian. >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> https://lists.xen.org/xen-devel
On Thu, Dec 22, 2016 at 1:15 PM, Alistair Francis <alistair.francis@xilinx.com> wrote: > On Thu, Dec 22, 2016 at 1:12 PM, Alistair Francis > <alistair.francis@xilinx.com> wrote: >> On Thu, Dec 22, 2016 at 11:22 AM, Ian Jackson <ian.jackson@eu.citrix.com> wrote: >>> Alistair Francis writes ("Re: [Xen-devel] [PATCH v2 1/5] Remove hardcoded strict -Werror checking"): >>>> On Thu, Dec 22, 2016 at 12:41 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 20.12.16 at 20:46, <alistair.francis@xilinx.com> wrote: >>>> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >>>> > >>>> > Without some rationale given I don't think such changes are >>>> > acceptable at all. And then, as already pointed out others, the >>>> > use of -Werror is there not just for fun. If anything I think an >>>> > override to that default could be acceptable. >>>> >>>> Unfortunately the APPEND_CFLAGS=-Wno-error doesn't fix all the issues >>>> as I still see warnings/errors when building: tools/kconfig/conf.c. >>> >>> That sounds like a bug to me. Do you know why it's not effective >>> there ? >> >> It actually might be an issue in the way buildroot is handling the arguments. >> >> I'll look into it and see what I find after the holidays. I dug into this a little more. Adding the APPEND_CFLAGS="-Wno-error" fixes almost everything. The only problem I see is in the log below, where tools/kconfig/conf.c fails to build as the -Wno-error doesn't propagate down. If I manage to find a fix today I'll send it, otherwise this can wait until next year. Thanks, Alistair > > Nope, it does look like a Xen build issue. I included the full failing > log below: > > PATH="/work/alistai/software/buildroot/output/host/bin:/work/alistai/software/buildroot/output/host/sbin:/work/alistai/software/buildroot/output/host/usr/bin:/work/alistai/software/buildroot/output/host/usr/sbin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin" > XEN_TARGET_ARCH=arm32 > CROSS_COMPILE=/work/alistai/software/buildroot/output/host/usr/bin/arm-linux- > PATH="/work/alistai/software/buildroot/output/host/bin:/work/alistai/software/buildroot/output/host/sbin:/work/alistai/software/buildroot/output/host/usr/bin:/work/alistai/software/buildroot/output/host/usr/sbin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin" > AR="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-ar" > AS="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-as" > LD="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-ld" > NM="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-nm" > CC="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-gcc" > GCC="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-gcc" > CPP="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-cpp" > CXX="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-g++" > FC="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-gfortran" > F77="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-gfortran" > RANLIB="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-ranlib" > READELF="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-readelf" > STRIP="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-strip" > OBJCOPY="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-objcopy" > OBJDUMP="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-objdump" > AR_FOR_BUILD="/usr/bin/ar" AS_FOR_BUILD="/usr/bin/as" > CC_FOR_BUILD="/usr/bin/gcc" GCC_FOR_BUILD="/usr/bin/gcc" > CXX_FOR_BUILD="/usr/bin/g++" LD_FOR_BUILD="/usr/bin/ld" > CPPFLAGS_FOR_BUILD="-I/work/alistai/software/buildroot/output/host/usr/include" > CFLAGS_FOR_BUILD="-O2 > -I/work/alistai/software/buildroot/output/host/usr/include" > CXXFLAGS_FOR_BUILD="-O2 > -I/work/alistai/software/buildroot/output/host/usr/include" > LDFLAGS_FOR_BUILD="-L/work/alistai/software/buildroot/output/host/lib > -L/work/alistai/software/buildroot/output/host/usr/lib > -Wl,-rpath,/work/alistai/software/buildroot/output/host/usr/lib" > FCFLAGS_FOR_BUILD="" > DEFAULT_ASSEMBLER="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-as" > DEFAULT_LINKER="/work/alistai/software/buildroot/output/host/usr/bin/arm-linux-ld" > CPPFLAGS="-D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE > -D_FILE_OFFSET_BITS=64" APPEND_CFLAGS="-Wno-error -D_LARGEFILE_SOURCE > -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -Os " > CXXFLAGS="-D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE > -D_FILE_OFFSET_BITS=64 -Os " LDFLAGS="" FAPPEND_CFLAGS="-Wno-error > -Os " FFLAGS=" -Os " > PKG_CONFIG="/work/alistai/software/buildroot/output/host/usr/bin/pkg-config" > STAGING_DIR="/work/alistai/software/buildroot/output/host/usr/arm-buildroot-linux-musleabihf/sysroot" > INTLTOOL_PERL=/usr/bin/perl /usr/bin/make -j11 dist-xen dist-tools -C > /work/alistai/software/buildroot/output/build/xen-4.8.0/ > make[1]: Entering directory > '/work/alistai/software/buildroot/output/build/xen-4.8.0' > /usr/bin/make -C xen install > /usr/bin/make -C tools install > make[2]: Entering directory > '/work/alistai/software/buildroot/output/build/xen-4.8.0/xen' > make[2]: Entering directory > '/work/alistai/software/buildroot/output/build/xen-4.8.0/tools' > /usr/bin/make -f > /work/alistai/software/buildroot/output/build/xen-4.8.0/xen/tools/kconfig/Makefile.kconfig > ARCH=arm32 SRCARCH=arm HOSTCC="/usr/bin/gcc" HOSTCXX="/usr/bin/g++" > defconfig > make[3]: Entering directory > '/work/alistai/software/buildroot/output/build/xen-4.8.0/xen' > make[3]: Entering directory > '/work/alistai/software/buildroot/output/build/xen-4.8.0/tools' > /usr/bin/gcc -Wp,-MD,tools/kconfig/.conf.o.d -Wall -Werror > -Wstrict-prototypes -O2 -fomit-frame-pointer -fno-strict-aliasing > -Wdeclaration-after-statement -DCURSES_LOC="<ncurses.h>" > -DNCURSES_WIDECHAR=1 -DLOCALE -DKBUILD_NO_NLS -c -o > tools/kconfig/conf.o tools/kconfig/conf.c > /usr/bin/gcc -Wp,-MD,tools/kconfig/.zconf.tab.o.d -Wall -Werror > -Wstrict-prototypes -O2 -fomit-frame-pointer -fno-strict-aliasing > -Wdeclaration-after-statement -DCURSES_LOC="<ncurses.h>" > -DNCURSES_WIDECHAR=1 -DLOCALE -DKBUILD_NO_NLS -Itools/kconfig -c -o > tools/kconfig/zconf.tab.o tools/kconfig/zconf.tab.c > tools/kconfig/conf.c: In function ‘check_stdin’: > tools/kconfig/conf.c:77:3: error: format not a string literal and no > format arguments [-Werror=format-security] > printf(_("aborted!\n\n")); > ^ > tools/kconfig/conf.c:78:3: error: format not a string literal and no > format arguments [-Werror=format-security] > printf(_("Console input/output is redirected. ")); > ^ > tools/kconfig/conf.c:79:3: error: format not a string literal and no > format arguments [-Werror=format-security] > printf(_("Run 'make oldconfig' to update configuration.\n\n")); > ^ > tools/kconfig/conf.c: In function ‘conf_askvalue’: > tools/kconfig/conf.c:89:3: error: format not a string literal and no > format arguments [-Werror=format-security] > printf(_("(NEW) ")); > ^ > tools/kconfig/conf.c: In function ‘conf_choice’: > tools/kconfig/conf.c:290:5: error: format not a string literal and no > format arguments [-Werror=format-security] > printf(_(" (NEW)")); > ^ > tools/kconfig/conf.c: In function ‘check_conf’: > tools/kconfig/conf.c:438:6: error: format not a string literal and no > format arguments [-Werror=format-security] > printf(_("*\n* Restart config...\n*\n")); > ^ > tools/kconfig/conf.c: In function ‘main’: > tools/kconfig/conf.c:640:6: error: format not a string literal and no > format arguments [-Werror=format-security] > _("\n*** The configuration requires explicit update.\n\n")); > ^ > tools/kconfig/conf.c:693:4: error: format not a string literal and no > format arguments [-Werror=format-security] > fprintf(stderr, _("\n*** Error during writing of the configuration.\n\n")); > ^ > tools/kconfig/conf.c:697:4: error: format not a string literal and no > format arguments [-Werror=format-security] > fprintf(stderr, _("\n*** Error during update of the configuration.\n\n")); > ^ > tools/kconfig/conf.c:708:4: error: format not a string literal and no > format arguments [-Werror=format-security] > fprintf(stderr, _("\n*** Error during writing of the configuration.\n\n")); > > >> >> Thanks, >> >> Alistair >> >>> >>>> Everyone seems fairly open to an override. Is a environment variable, >>>> which if set will disable Werror acceptable? Something like NO_ERROR=Y >>>> which will result in no -Werror being appended. >>> >>> Yes, an environment variable would be acceptable, but it should have >>> the right name and semantics and ideally we could reuse an existing >>> variable or fix it if it is broken. >>> >>> How about `APPEND_CFLAGS=-Wno-error' ? :-) >>> >>> Thanks, >>> Ian. >>> >>> _______________________________________________ >>> Xen-devel mailing list >>> Xen-devel@lists.xen.org >>> https://lists.xen.org/xen-devel
>>> Alistair Francis <alistair.francis@xilinx.com> 12/22/16 8:14 PM >>> >On Thu, Dec 22, 2016 at 12:41 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 20.12.16 at 20:46, <alistair.francis@xilinx.com> wrote: >>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> >> >> Without some rationale given I don't think such changes are >> acceptable at all. And then, as already pointed out others, the >> use of -Werror is there not just for fun. If anything I think an >> override to that default could be acceptable. > >Unfortunately the APPEND_CFLAGS=-Wno-error doesn't fix all the issues >as I still see warnings/errors when building: tools/kconfig/conf.c. Well, it's quite obvious that the same set of options (and hence overrides) can't possibly fit both the build of target binaries and the build of build helper tools (i.e. build host binaries). >I understand that you want it in by default. > >Everyone seems fairly open to an override. Is a environment variable, >which if set will disable Werror acceptable? Something like NO_ERROR=Y >which will result in no -Werror being appended. I dislike environment variables for such purposes, and would prefer requiring such to be added as make options. If it was an environment variable, it should start with XEN_. And its name should fully reflect the purpose, i.e. I shouldn't have to guess what kinds of errors would be suppressed. Perhaps WARN_NO_ERROR? Jan
>>> "Jan Beulich" <jbeulich@suse.com> 12/27/16 4:42 PM >>> >>> Alistair Francis <alistair.francis@xilinx.com> 12/22/16 8:14 PM >>> >>Everyone seems fairly open to an override. Is a environment variable, >>which if set will disable Werror acceptable? Something like NO_ERROR=Y >>which will result in no -Werror being appended. > >I dislike environment variables for such purposes, and would prefer requiring >such to be added as make options. If it was an environment variable, it >should start with XEN_. And its name should fully reflect the purpose, i.e. I >shouldn't have to guess what kinds of errors would be suppressed. Perhaps >WARN_NO_ERROR? That said, I'm not sure everyone agreed on putting an override in place. I think Andrew had made it quite clear that there is a reason for -Werror to be in use, and we shouldn't encourage people weakening code by tolerating warnings (even if for the purpose of upstream integration no warnings will be permitted anyway, due to -Werror remaining the default). Jan
On 12/27/16 9:53 AM, Jan Beulich wrote: >>>> "Jan Beulich" <jbeulich@suse.com> 12/27/16 4:42 PM >>> >>>> Alistair Francis <alistair.francis@xilinx.com> 12/22/16 8:14 PM >>> >>> Everyone seems fairly open to an override. Is a environment variable, >>> which if set will disable Werror acceptable? Something like NO_ERROR=Y >>> which will result in no -Werror being appended. >> >> I dislike environment variables for such purposes, and would prefer requiring >> such to be added as make options. If it was an environment variable, it >> should start with XEN_. And its name should fully reflect the purpose, i.e. I >> shouldn't have to guess what kinds of errors would be suppressed. Perhaps >> WARN_NO_ERROR? > > That said, I'm not sure everyone agreed on putting an override in place. I think > Andrew had made it quite clear that there is a reason for -Werror to be in use, > and we shouldn't encourage people weakening code by tolerating warnings > (even if for the purpose of upstream integration no warnings will be permitted > anyway, due to -Werror remaining the default). > > Jan > That was for the hypervisor bits. Anything that relies on user space code and headers that will be provided by the distro should be able to build with -Werror. That's something all distros have to patch out and is downstream hostile to do since a different toolchain than what the developers use can generate warnings or slight different dependencies an generate them.
On 27/12/16 15:53, Jan Beulich wrote: >>>> "Jan Beulich" <jbeulich@suse.com> 12/27/16 4:42 PM >>> >>>> Alistair Francis <alistair.francis@xilinx.com> 12/22/16 8:14 PM >>> >>> Everyone seems fairly open to an override. Is a environment variable, >>> which if set will disable Werror acceptable? Something like NO_ERROR=Y >>> which will result in no -Werror being appended. >> I dislike environment variables for such purposes, and would prefer requiring >> such to be added as make options. If it was an environment variable, it >> should start with XEN_. And its name should fully reflect the purpose, i.e. I >> shouldn't have to guess what kinds of errors would be suppressed. Perhaps >> WARN_NO_ERROR? > That said, I'm not sure everyone agreed on putting an override in place. I think > Andrew had made it quite clear that there is a reason for -Werror to be in use, > and we shouldn't encourage people weakening code by tolerating warnings > (even if for the purpose of upstream integration no warnings will be permitted > anyway, due to -Werror remaining the default). For development, -Werror should remain the default. For downstream integration, an ability to override -Werror is useful for distros, especially in cases of using a newer compiler than the code was ever developed against. However, it should definitely be the case that a positive choice needs to be taken to disable -Werror, which should hopefully make people thing twice about doing so. ~Andrew
On Tue, Dec 27, 2016 at 5:07 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > On 27/12/16 15:53, Jan Beulich wrote: >>>>> >>>>> "Jan Beulich" <jbeulich@suse.com> 12/27/16 4:42 PM >>> >>>>> Alistair Francis <alistair.francis@xilinx.com> 12/22/16 8:14 PM >>> >>>> >>>> Everyone seems fairly open to an override. Is a environment variable, >>>> which if set will disable Werror acceptable? Something like NO_ERROR=Y >>>> which will result in no -Werror being appended. >>> >>> I dislike environment variables for such purposes, and would prefer >>> requiring >>> such to be added as make options. If it was an environment variable, it >>> should start with XEN_. And its name should fully reflect the purpose, >>> i.e. I >>> shouldn't have to guess what kinds of errors would be suppressed. Perhaps >>> WARN_NO_ERROR? >> >> That said, I'm not sure everyone agreed on putting an override in place. I >> think >> Andrew had made it quite clear that there is a reason for -Werror to be in >> use, >> and we shouldn't encourage people weakening code by tolerating warnings >> (even if for the purpose of upstream integration no warnings will be >> permitted >> anyway, due to -Werror remaining the default). > > > For development, -Werror should remain the default. > > For downstream integration, an ability to override -Werror is useful for > distros, especially in cases of using a newer compiler than the code was > ever developed against. > > However, it should definitely be the case that a positive choice needs to be > taken to disable -Werror, which should hopefully make people thing twice > about doing so. Wouldn't it make more sense to disable -Werror for the release? This seems similar to ASSERT(). The idea behind having ASSERT() enabled during development and disabled for releases is that there are different goals for each. During development you expect things to be unstable and crash, and you want to fix anything that comes up, and you definitely want to catch as many bugs as you possibly can. Once something is released and put into production, the priority changes -- the goal now is to keep the system up and running, not to find bugs; so everything for which an incorrect assumption is *less bad* than a crash should not crash. Similarly, during development we definitely want all the help we can get to find programmer mistakes; and so having the build fail every time gcc detects something funny is a big help for that. But with a release, the priorities are different. Releases are not for developers, but for downstreams and for users. They're not really in a position to fix the issue. And in any case, whatever the issue is, it's pretty certain that dozens of other places with older compilers are *already* running with that issue and just don't know about it. So running without -Werror *just for the release* makes the project as a whole more useful for downstreams and users, without hurting development, it seems to me. -George
On 29/12/2016 17:10, George Dunlap wrote: > On Tue, Dec 27, 2016 at 5:07 PM, Andrew Cooper > <andrew.cooper3@citrix.com> wrote: >> On 27/12/16 15:53, Jan Beulich wrote: >>>>>> "Jan Beulich" <jbeulich@suse.com> 12/27/16 4:42 PM >>> >>>>>> Alistair Francis <alistair.francis@xilinx.com> 12/22/16 8:14 PM >>> >>>>> Everyone seems fairly open to an override. Is a environment variable, >>>>> which if set will disable Werror acceptable? Something like NO_ERROR=Y >>>>> which will result in no -Werror being appended. >>>> I dislike environment variables for such purposes, and would prefer >>>> requiring >>>> such to be added as make options. If it was an environment variable, it >>>> should start with XEN_. And its name should fully reflect the purpose, >>>> i.e. I >>>> shouldn't have to guess what kinds of errors would be suppressed. Perhaps >>>> WARN_NO_ERROR? >>> That said, I'm not sure everyone agreed on putting an override in place. I >>> think >>> Andrew had made it quite clear that there is a reason for -Werror to be in >>> use, >>> and we shouldn't encourage people weakening code by tolerating warnings >>> (even if for the purpose of upstream integration no warnings will be >>> permitted >>> anyway, due to -Werror remaining the default). >> >> For development, -Werror should remain the default. >> >> For downstream integration, an ability to override -Werror is useful for >> distros, especially in cases of using a newer compiler than the code was >> ever developed against. >> >> However, it should definitely be the case that a positive choice needs to be >> taken to disable -Werror, which should hopefully make people thing twice >> about doing so. > Wouldn't it make more sense to disable -Werror for the release? -1. This is not a sensible suggestion IMO. The reason for switching off debugging for a release is because there is a runtime overhead of the debugging, and we don't provide security support in case the ASSERT()s are a little too aggressive. The reason behind using -Werror doesn't change at the point of a release. A warning on a release branch is just as important to fix as a warning on master. ~Andrew
>>> On 29.12.16 at 18:30, <andrew.cooper3@citrix.com> wrote: > On 29/12/2016 17:10, George Dunlap wrote: >> On Tue, Dec 27, 2016 at 5:07 PM, Andrew Cooper >> <andrew.cooper3@citrix.com> wrote: >>> On 27/12/16 15:53, Jan Beulich wrote: >>>>>>> "Jan Beulich" <jbeulich@suse.com> 12/27/16 4:42 PM >>> >>>>>>> Alistair Francis <alistair.francis@xilinx.com> 12/22/16 8:14 PM >>> >>>>>> Everyone seems fairly open to an override. Is a environment variable, >>>>>> which if set will disable Werror acceptable? Something like NO_ERROR=Y >>>>>> which will result in no -Werror being appended. >>>>> I dislike environment variables for such purposes, and would prefer >>>>> requiring >>>>> such to be added as make options. If it was an environment variable, it >>>>> should start with XEN_. And its name should fully reflect the purpose, >>>>> i.e. I >>>>> shouldn't have to guess what kinds of errors would be suppressed. Perhaps >>>>> WARN_NO_ERROR? >>>> That said, I'm not sure everyone agreed on putting an override in place. I >>>> think >>>> Andrew had made it quite clear that there is a reason for -Werror to be in >>>> use, >>>> and we shouldn't encourage people weakening code by tolerating warnings >>>> (even if for the purpose of upstream integration no warnings will be >>>> permitted >>>> anyway, due to -Werror remaining the default). >>> >>> For development, -Werror should remain the default. >>> >>> For downstream integration, an ability to override -Werror is useful for >>> distros, especially in cases of using a newer compiler than the code was >>> ever developed against. >>> >>> However, it should definitely be the case that a positive choice needs to be >>> taken to disable -Werror, which should hopefully make people thing twice >>> about doing so. >> Wouldn't it make more sense to disable -Werror for the release? > > -1. This is not a sensible suggestion IMO. > > The reason for switching off debugging for a release is because there is > a runtime overhead of the debugging, and we don't provide security > support in case the ASSERT()s are a little too aggressive. > > The reason behind using -Werror doesn't change at the point of a > release. A warning on a release branch is just as important to fix as a > warning on master. The more that, with changed optimization settings, warnings occasionally change too (potentially pointing out so far overlooked issues). If _all_ our downstreams participated in at least RC testing, the whole situation might be a little different, but without that I think we should rather hope for them to report issues with compiler versions no-one of us tried to build with. Jan
Jan Beulich writes ("Re: [Xen-devel] [PATCH v2 1/5] Remove hardcoded strict -Werror checking"):
> Alistair Francis <alistair.francis@xilinx.com> 12/22/16 8:14 PM >>>
> >Unfortunately the APPEND_CFLAGS=-Wno-error doesn't fix all the issues
> >as I still see warnings/errors when building: tools/kconfig/conf.c.
>
> Well, it's quite obvious that the same set of options (and hence overrides)
> can't possibly fit both the build of target binaries and the build of build
> helper tools (i.e. build host binaries).
I think that the example of `-Wno-error' shows that this is, in fact,
false. There are some overrides that, if desirable, apply equally
everywhere.
-Wno-error is a good example because it is mostly needed when a new
compiler throws up new warnings for existing code. Admittedly,
-Wno-error may not be the best response, but it is often the most
expedient, and we shouldn't make it unnecessarily hard for
downstreams to support it.
For now I think the proposed approach, to make kconf build honour
APPEND_CFLAGS, is good. Do you agree, Jan ?
If someone wants to introduce HYPERVISOR_APPEND_CFLAGS and
TOOLS_APPEND_CFLAGS, I don't object, but that should be a separate
project.
Ian.
>>> On 03.01.17 at 15:38, <ian.jackson@eu.citrix.com> wrote: > Jan Beulich writes ("Re: [Xen-devel] [PATCH v2 1/5] Remove hardcoded strict > -Werror checking"): >> Alistair Francis <alistair.francis@xilinx.com> 12/22/16 8:14 PM >>> >> >Unfortunately the APPEND_CFLAGS=-Wno-error doesn't fix all the issues >> >as I still see warnings/errors when building: tools/kconfig/conf.c. >> >> Well, it's quite obvious that the same set of options (and hence overrides) >> can't possibly fit both the build of target binaries and the build of build >> helper tools (i.e. build host binaries). > > I think that the example of `-Wno-error' shows that this is, in fact, > false. There are some overrides that, if desirable, apply equally > everywhere. I agree, but I don't think this is relevant here: How does one easily(!) know that APPEND_CFLAGS affects both host and target binaries? Just like there is HOSTCC and HOSTCFLAGS, there should be (e.g.) APPEND_HOSTCFLAGS. What if $(HOSTCC) doesn't even understand -Wno-error (or any other option put into such a shared variable)? > -Wno-error is a good example because it is mostly needed when a new > compiler throws up new warnings for existing code. Admittedly, > -Wno-error may not be the best response, but it is often the most > expedient, and we shouldn't make it unnecessarily hard for > downstreams to support it. > > For now I think the proposed approach, to make kconf build honour > APPEND_CFLAGS, is good. Do you agree, Jan ? > > If someone wants to introduce HYPERVISOR_APPEND_CFLAGS and > TOOLS_APPEND_CFLAGS, I don't object, but that should be a separate > project. As per above, I disagree. We shouldn't help people making mistakes by lumping together two disjoint sets of flags. Jan
diff --git a/Config.mk b/Config.mk index 3ec7367..e3cda81 100644 --- a/Config.mk +++ b/Config.mk @@ -34,7 +34,7 @@ CONFIG_$(XEN_OS) := y SHELL ?= /bin/sh # Tools to run on system hosting the build -HOSTCFLAGS = -Wall -Werror -Wstrict-prototypes -O2 -fomit-frame-pointer +HOSTCFLAGS = -Wall -Wstrict-prototypes -O2 -fomit-frame-pointer HOSTCFLAGS += -fno-strict-aliasing DISTDIR ?= $(XEN_ROOT)/dist diff --git a/tools/blktap2/drivers/Makefile b/tools/blktap2/drivers/Makefile index 5328c40..7a62a3f 100644 --- a/tools/blktap2/drivers/Makefile +++ b/tools/blktap2/drivers/Makefile @@ -9,7 +9,6 @@ QCOW_UTIL = img2qcow qcow-create qcow2raw LOCK_UTIL = lock-util INST_DIR = $(sbindir) -CFLAGS += -Werror CFLAGS += -Wno-unused CFLAGS += -fno-strict-aliasing CFLAGS += -I$(BLKTAP_ROOT)/include -I$(BLKTAP_ROOT)/drivers diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile index 91e2f97..e8a37ef 100644 --- a/tools/libxl/Makefile +++ b/tools/libxl/Makefile @@ -11,7 +11,7 @@ MINOR = 0 XLUMAJOR = 4.9 XLUMINOR = 0 -CFLAGS += -Werror -Wno-format-zero-length -Wmissing-declarations \ +CFLAGS += -Wno-format-zero-length -Wmissing-declarations \ -Wno-declaration-after-statement -Wformat-nonliteral CFLAGS += -I. -fPIC diff --git a/tools/xentrace/Makefile b/tools/xentrace/Makefile index c8c36a8..ac5c534 100644 --- a/tools/xentrace/Makefile +++ b/tools/xentrace/Makefile @@ -1,8 +1,6 @@ XEN_ROOT=$(CURDIR)/../.. include $(XEN_ROOT)/tools/Rules.mk -CFLAGS += -Werror - CFLAGS += $(CFLAGS_libxenevtchn) CFLAGS += $(CFLAGS_libxenctrl) LDLIBS += $(LDLIBS_libxenevtchn)
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> --- Config.mk | 2 +- tools/blktap2/drivers/Makefile | 1 - tools/libxl/Makefile | 2 +- tools/xentrace/Makefile | 2 -- 4 files changed, 2 insertions(+), 5 deletions(-)