xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] xen/Makefile: drop -Werror
@ 2021-07-02 17:06 Fabrice Fontaine
  2021-07-02 17:34 ` Andrew Cooper
  0 siblings, 1 reply; 7+ messages in thread
From: Fabrice Fontaine @ 2021-07-02 17:06 UTC (permalink / raw)
  To: xen-devel; +Cc: Fabrice Fontaine

Drop -Werror to avoid the following build failure with -DNDEBUG:

In file included from <command-line>:0:0:
/usr/lfs/hdd_v1/rc-buildroot-test/scripts/instance-0/output-1/build/xen-4.14.2/xen/include/xen/config.h:94:0: error: "NDEBUG" redefined [-Werror]
 #define NDEBUG

<command-line>:0:0: note: this is the location of the previous definition

Fixes:
 - http://autobuild.buildroot.org/results/66573ad0abc4244c0dfeac8b684a7bfcc31c0d4d

Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
---
 xen/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/Makefile b/xen/Makefile
index 89879fad4c..cf9f83b1fb 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -210,7 +210,7 @@ CFLAGS += -fomit-frame-pointer
 endif
 
 CFLAGS += -nostdinc -fno-builtin -fno-common
-CFLAGS += -Werror -Wredundant-decls -Wno-pointer-arith
+CFLAGS += -Wredundant-decls -Wno-pointer-arith
 $(call cc-option-add,CFLAGS,CC,-Wvla)
 CFLAGS += -pipe -D__XEN__ -include $(BASEDIR)/include/xen/config.h
 CFLAGS-$(CONFIG_DEBUG_INFO) += -g
-- 
2.30.2



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] xen/Makefile: drop -Werror
  2021-07-02 17:06 [PATCH] xen/Makefile: drop -Werror Fabrice Fontaine
@ 2021-07-02 17:34 ` Andrew Cooper
  2021-07-02 17:51   ` Fabrice Fontaine
  0 siblings, 1 reply; 7+ messages in thread
From: Andrew Cooper @ 2021-07-02 17:34 UTC (permalink / raw)
  To: Fabrice Fontaine, xen-devel

On 02/07/2021 18:06, Fabrice Fontaine wrote:
> Drop -Werror to avoid the following build failure with -DNDEBUG:
>
> In file included from <command-line>:0:0:
> /usr/lfs/hdd_v1/rc-buildroot-test/scripts/instance-0/output-1/build/xen-4.14.2/xen/include/xen/config.h:94:0: error: "NDEBUG" redefined [-Werror]
>  #define NDEBUG
>
> <command-line>:0:0: note: this is the location of the previous definition
>
> Fixes:
>  - http://autobuild.buildroot.org/results/66573ad0abc4244c0dfeac8b684a7bfcc31c0d4d
>
> Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>

For better or worse, It is Xen's policy that -Werror will remain.  95%
of the time, it is the right thing.  We will however build failures
whenever they crop up.

This one is weird though.  How is NDEBUG getting in twice?  What does
the rest of this build environment look like?

~Andrew


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] xen/Makefile: drop -Werror
  2021-07-02 17:34 ` Andrew Cooper
@ 2021-07-02 17:51   ` Fabrice Fontaine
  2021-07-02 18:52     ` Elliott Mitchell
  2021-07-05  8:16     ` Jan Beulich
  0 siblings, 2 replies; 7+ messages in thread
From: Fabrice Fontaine @ 2021-07-02 17:51 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: xen-devel

Le ven. 2 juil. 2021 à 19:34, Andrew Cooper
<andrew.cooper3@citrix.com> a écrit :
>
> On 02/07/2021 18:06, Fabrice Fontaine wrote:
> > Drop -Werror to avoid the following build failure with -DNDEBUG:
> >
> > In file included from <command-line>:0:0:
> > /usr/lfs/hdd_v1/rc-buildroot-test/scripts/instance-0/output-1/build/xen-4.14.2/xen/include/xen/config.h:94:0: error: "NDEBUG" redefined [-Werror]
> >  #define NDEBUG
> >
> > <command-line>:0:0: note: this is the location of the previous definition
> >
> > Fixes:
> >  - http://autobuild.buildroot.org/results/66573ad0abc4244c0dfeac8b684a7bfcc31c0d4d
> >
> > Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
>
> For better or worse, It is Xen's policy that -Werror will remain.  95%
> of the time, it is the right thing.  We will however build failures
> whenever they crop up.
>
> This one is weird though.  How is NDEBUG getting in twice?  What does
> the rest of this build environment look like?
NDEBUG is added by buildroot in the command line if the user sets
BR2_ENABLE_RUNTIME_DEBUG to false since
https://git.buildroot.net/buildroot/commit/?id=5a8c50fe05afacc3cbe8e7347e238da9f242fab0

I do agree that setting -Werror is generally perfectly valid for upstream.
However, for downstream packager, it is generally seen as an issue as
it will always raise unexepected build failures with older, newer, or
exotic toolchains, see
https://embeddedartistry.com/blog/2017/05/22/werror-is-not-your-friend.
It would be good to, at least, have an option to disable -Werror for
example through a XEN_DISABLE_WERROR.
>
> ~Andrew
Best Regards,

Fabrice


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] xen/Makefile: drop -Werror
  2021-07-02 17:51   ` Fabrice Fontaine
@ 2021-07-02 18:52     ` Elliott Mitchell
  2021-07-05  8:09       ` Jan Beulich
  2021-07-05  8:16     ` Jan Beulich
  1 sibling, 1 reply; 7+ messages in thread
From: Elliott Mitchell @ 2021-07-02 18:52 UTC (permalink / raw)
  To: Fabrice Fontaine; +Cc: Andrew Cooper, xen-devel

On Fri, Jul 02, 2021 at 07:51:55PM +0200, Fabrice Fontaine wrote:
> 
> I do agree that setting -Werror is generally perfectly valid for upstream.
> However, for downstream packager, it is generally seen as an issue as
> it will always raise unexepected build failures with older, newer, or
> exotic toolchains, see
> https://embeddedartistry.com/blog/2017/05/22/werror-is-not-your-friend.
> It would be good to, at least, have an option to disable -Werror for
> example through a XEN_DISABLE_WERROR.

Two people don't make it a majority opinion, but if this was a meeting
this opinion would get a second.

I don't know where everyone is on the spectrum, but I also strongly
dislike -Werror yet do like -Wall and tend to get rid of warnings.
-Werror is good for continuous integration systems, not so great for
releases or active development.

-Werror kind of seems like Stack Ranking, good for use during brief
periods, but poor for long term continuous use.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445




^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] xen/Makefile: drop -Werror
  2021-07-02 18:52     ` Elliott Mitchell
@ 2021-07-05  8:09       ` Jan Beulich
  0 siblings, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2021-07-05  8:09 UTC (permalink / raw)
  To: Elliott Mitchell; +Cc: Andrew Cooper, xen-devel, Fabrice Fontaine

On 02.07.2021 20:52, Elliott Mitchell wrote:
> On Fri, Jul 02, 2021 at 07:51:55PM +0200, Fabrice Fontaine wrote:
>>
>> I do agree that setting -Werror is generally perfectly valid for upstream.
>> However, for downstream packager, it is generally seen as an issue as
>> it will always raise unexepected build failures with older, newer, or
>> exotic toolchains, see
>> https://embeddedartistry.com/blog/2017/05/22/werror-is-not-your-friend.
>> It would be good to, at least, have an option to disable -Werror for
>> example through a XEN_DISABLE_WERROR.
> 
> Two people don't make it a majority opinion, but if this was a meeting
> this opinion would get a second.
> 
> I don't know where everyone is on the spectrum, but I also strongly
> dislike -Werror yet do like -Wall and tend to get rid of warnings.
> -Werror is good for continuous integration systems, not so great for
> releases or active development.

Well, my experience with Linux (when I started working there alongside
working on Xen, many years ago) was that many people don't care at all
about compiler warnings their code changes introduce. While Linux has
improved some, I'm still carrying a fair size patch to silence all the
warnings that I observe on various build systems (i.e. with various
compiler versions). I do this because in a build with (perhaps many)
pre-existing warnings it is far easier to miss the one you accidentally
introduce with some code change. -Werror is an imo very appropriate
measure to get people to at least address the warnings they can easily
observe about everywhere. IOW I've always been appreciating Xen being
different from Linux in this regard.

Jan



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] xen/Makefile: drop -Werror
  2021-07-02 17:51   ` Fabrice Fontaine
  2021-07-02 18:52     ` Elliott Mitchell
@ 2021-07-05  8:16     ` Jan Beulich
  2021-07-05  9:27       ` Fabrice Fontaine
  1 sibling, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2021-07-05  8:16 UTC (permalink / raw)
  To: Fabrice Fontaine; +Cc: xen-devel, Andrew Cooper

On 02.07.2021 19:51, Fabrice Fontaine wrote:
> Le ven. 2 juil. 2021 à 19:34, Andrew Cooper
> <andrew.cooper3@citrix.com> a écrit :
>>
>> On 02/07/2021 18:06, Fabrice Fontaine wrote:
>>> Drop -Werror to avoid the following build failure with -DNDEBUG:
>>>
>>> In file included from <command-line>:0:0:
>>> /usr/lfs/hdd_v1/rc-buildroot-test/scripts/instance-0/output-1/build/xen-4.14.2/xen/include/xen/config.h:94:0: error: "NDEBUG" redefined [-Werror]
>>>  #define NDEBUG
>>>
>>> <command-line>:0:0: note: this is the location of the previous definition
>>>
>>> Fixes:
>>>  - http://autobuild.buildroot.org/results/66573ad0abc4244c0dfeac8b684a7bfcc31c0d4d
>>>
>>> Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
>>
>> For better or worse, It is Xen's policy that -Werror will remain.  95%
>> of the time, it is the right thing.  We will however build failures
>> whenever they crop up.
>>
>> This one is weird though.  How is NDEBUG getting in twice?  What does
>> the rest of this build environment look like?
> NDEBUG is added by buildroot in the command line if the user sets
> BR2_ENABLE_RUNTIME_DEBUG to false since
> https://git.buildroot.net/buildroot/commit/?id=5a8c50fe05afacc3cbe8e7347e238da9f242fab0

I suppose the build environment setting is really intended for user mode
code. I question its applicability to the building of kernels or
hypervisors, but I can see that opinions may vary here. If we wanted to
honor a pre-existing NDEBUG, how about simply making xen/config.h have

#if !defined(CONFIG_DEBUG) && !defined(NDEBUG)
#define NDEBUG
#endif

? This then raises the question though how an external environment could
achieve the opposite effect of suppressing NDEBUG's definition despite
CONFIG_DEBUG being set. (The main point - hence my view expressed above -
is that we switched to Kconfig to centralize where settings get
established, moving away from taking ones from environment or make
command line.)

Jan



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] xen/Makefile: drop -Werror
  2021-07-05  8:16     ` Jan Beulich
@ 2021-07-05  9:27       ` Fabrice Fontaine
  0 siblings, 0 replies; 7+ messages in thread
From: Fabrice Fontaine @ 2021-07-05  9:27 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Andrew Cooper

Dear all,

Le lun. 5 juil. 2021 à 10:16, Jan Beulich <jbeulich@suse.com> a écrit :
>
> On 02.07.2021 19:51, Fabrice Fontaine wrote:
> > Le ven. 2 juil. 2021 à 19:34, Andrew Cooper
> > <andrew.cooper3@citrix.com> a écrit :
> >>
> >> On 02/07/2021 18:06, Fabrice Fontaine wrote:
> >>> Drop -Werror to avoid the following build failure with -DNDEBUG:
> >>>
> >>> In file included from <command-line>:0:0:
> >>> /usr/lfs/hdd_v1/rc-buildroot-test/scripts/instance-0/output-1/build/xen-4.14.2/xen/include/xen/config.h:94:0: error: "NDEBUG" redefined [-Werror]
> >>>  #define NDEBUG
> >>>
> >>> <command-line>:0:0: note: this is the location of the previous definition
> >>>
> >>> Fixes:
> >>>  - http://autobuild.buildroot.org/results/66573ad0abc4244c0dfeac8b684a7bfcc31c0d4d
> >>>
> >>> Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
> >>
> >> For better or worse, It is Xen's policy that -Werror will remain.  95%
> >> of the time, it is the right thing.  We will however build failures
> >> whenever they crop up.
> >>
> >> This one is weird though.  How is NDEBUG getting in twice?  What does
> >> the rest of this build environment look like?
> > NDEBUG is added by buildroot in the command line if the user sets
> > BR2_ENABLE_RUNTIME_DEBUG to false since
> > https://git.buildroot.net/buildroot/commit/?id=5a8c50fe05afacc3cbe8e7347e238da9f242fab0
>
> I suppose the build environment setting is really intended for user mode
> code. I question its applicability to the building of kernels or
> hypervisors, but I can see that opinions may vary here. If we wanted to
> honor a pre-existing NDEBUG, how about simply making xen/config.h have
>
> #if !defined(CONFIG_DEBUG) && !defined(NDEBUG)
> #define NDEBUG
> #endif
>
> ? This then raises the question though how an external environment could
> achieve the opposite effect of suppressing NDEBUG's definition despite
> CONFIG_DEBUG being set. (The main point - hence my view expressed above -
> is that we switched to Kconfig to centralize where settings get
> established, moving away from taking ones from environment or make
> command line.)
FYI, we have reverted the commit that allowed the user to set -DNDEBUG
as it was raising too many build failures:
https://git.buildroot.net/buildroot/commit/?id=a1c7cff1a081765c082c196bd9e6c1e72ceee797
So, this patch is not needed anymore on buildroot side.
>
> Jan
>
Best Regards,

Fabrice


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-07-05  9:28 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-02 17:06 [PATCH] xen/Makefile: drop -Werror Fabrice Fontaine
2021-07-02 17:34 ` Andrew Cooper
2021-07-02 17:51   ` Fabrice Fontaine
2021-07-02 18:52     ` Elliott Mitchell
2021-07-05  8:09       ` Jan Beulich
2021-07-05  8:16     ` Jan Beulich
2021-07-05  9:27       ` Fabrice Fontaine

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).