Overriding -Werror

Overriding -Werror

[Brian Norris]

Hi all,

I'm interested in being able to build-test kernels on various
architectures while enabling extra warnings (make W=[123]). I'd like to
be able to finish the builds and see all warnings, rather than seeing a
failed build. However, GCC's -Werror is incompatible with this. There is
plenty of code that will produce at least one warning, when warning
verbosity is turned up. And GCC's -Werror is not guaranteed to remain
stable over time; new versions may develop new warnings that may or may
not be legitimate.

It seems that there are a few problem ARCHes that enable -Werror by
default: SuperH (orphaned), SPARC, and MIPS. There are also a few
scattered Makefiles throughout the build tree. Developers have
previously tried to remove some of the worst offenders [1], but were
mostly rejected [2]. It doesn't seem like we can fully prevent
maintainers from enabling -Werror on their code--or even on their entire
ARCH build, as with MIPS--for better or worse, so I look to other
alternatives.

For the easiest approach, I considered how one might add -Wno-error to
the CFLAGS. 'make KCFLAGS=-Wno-error' looked promising, but
KBUILD_CFLAGS is applied before the sub-directory Makefiles add their
own options to ccflags-y. So it seems like others have come to the same
conclusion as me: that Kbuild doesn't seem to provide a way to override
the -Werror behvaior from the top level. [3][4]

So, how can we fix this? -Werror may be useful in some cases to
encourage developers to fix up their code immediately, but it is
decidedly unhelpful when running code through analysis tools.

Possibilities include:

1. make -Werror be applied only when we do not have W=[123]. [5]

2. develop a top-level override for CFLAGS that is applied *after* all
   sub-directory modifications

3. make -Werror opt-in / configurable, like PPC's CONFIG_PPC_WERROR
   (maybe make it a generic CONFIG_WERROR?), and prevent its
   unconditional use in Makefiles

4. better ideas?

Regards,
Brian

[1] http://www.linux-mips.org/archives/linux-mips/2012-04/msg00179.html
    http://patchwork.ozlabs.org/patch/146297/

[2] http://www.linux-mips.org/archives/linux-mips/2012-05/msg00064.html

[3] http://lists.linaro.org/pipermail/linaro-toolchain/2011-November/001869.html

[4] http://lists.linaro.org/pipermail/linaro-dev/2011-December/008880.html

[5] http://www.linux-mips.org/archives/linux-mips/2012-05/msg00070.html

Re: Overriding -Werror

[Jeff Kirsher]

On Thu, Aug 14, 2014 at 3:21 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> Hi all,
>
> I'm interested in being able to build-test kernels on various
> architectures while enabling extra warnings (make W=[123]). I'd like to
> be able to finish the builds and see all warnings, rather than seeing a
> failed build. However, GCC's -Werror is incompatible with this. There is
> plenty of code that will produce at least one warning, when warning
> verbosity is turned up. And GCC's -Werror is not guaranteed to remain
> stable over time; new versions may develop new warnings that may or may
> not be legitimate.
>
> It seems that there are a few problem ARCHes that enable -Werror by
> default: SuperH (orphaned), SPARC, and MIPS. There are also a few
> scattered Makefiles throughout the build tree. Developers have
> previously tried to remove some of the worst offenders [1], but were
> mostly rejected [2]. It doesn't seem like we can fully prevent
> maintainers from enabling -Werror on their code--or even on their entire
> ARCH build, as with MIPS--for better or worse, so I look to other
> alternatives.
>
> For the easiest approach, I considered how one might add -Wno-error to
> the CFLAGS. 'make KCFLAGS=-Wno-error' looked promising, but
> KBUILD_CFLAGS is applied before the sub-directory Makefiles add their
> own options to ccflags-y. So it seems like others have come to the same
> conclusion as me: that Kbuild doesn't seem to provide a way to override
> the -Werror behvaior from the top level. [3][4]
>
> So, how can we fix this? -Werror may be useful in some cases to
> encourage developers to fix up their code immediately, but it is
> decidedly unhelpful when running code through analysis tools.
>
> Possibilities include:
>
> 1. make -Werror be applied only when we do not have W=[123]. [5]
>
> 2. develop a top-level override for CFLAGS that is applied *after* all
>    sub-directory modifications
>
> 3. make -Werror opt-in / configurable, like PPC's CONFIG_PPC_WERROR
>    (maybe make it a generic CONFIG_WERROR?), and prevent its
>    unconditional use in Makefiles
>
> 4. better ideas?
>
> Regards,
> Brian
>
> [1] http://www.linux-mips.org/archives/linux-mips/2012-04/msg00179.html
>     http://patchwork.ozlabs.org/patch/146297/
>
> [2] http://www.linux-mips.org/archives/linux-mips/2012-05/msg00064.html
>
> [3] http://lists.linaro.org/pipermail/linaro-toolchain/2011-November/001869.html
>
> [4] http://lists.linaro.org/pipermail/linaro-dev/2011-December/008880.html
>
> [5] http://www.linux-mips.org/archives/linux-mips/2012-05/msg00070.html
> --

Funny that you bring this up because I have ~60 patches in my queue to
resolve several thousand of these warnings.  Half of the patches
actually resolve warnings that can be resolved and the other half
implement compiler diagnostic control macros to help silence warnings.
All these were the work of a co-worker Mark Rustad, below is the patch
he put together to implement the compiler diagnostic control macros.

commit 7b9aace02b2405f0714bc08c424b72e6962f1c2e
Author: Mark Rustad <mark.d.rustad@intel.com>
Date:   Fri Aug 15 01:43:44 2014 -0700

    compiler: Add diagnostic control macros

    Add macros to control diagnostic messages where needed. These
    are used to silence warning messages that are expected, normal
    and do not indicate any sort of problem. Reducing the stream
    of messages in this way helps possible problems to stand out.

    The macros provided are:
        DIAG_PUSH() - to save diagnostic settings
        DIAG_POP() - to restore diagnostic settings
        DIAG_IGNORE(option) - to ignore a particular warning
        DIAG_GCC_IGNORE(option) - DIAG_IGNORE for gcc only
        DIAG_CLANG_IGNORE(option) - DIAG_IGNORE for clang only

    Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>

diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
index d1e49d5..039b112 100644
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -10,3 +10,29 @@
 #undef uninitialized_var
 #define uninitialized_var(x) x = *(&(x))
 #endif
+
+/*
+ * Provide macros to manipulate diagnostic messages when possible.
+ * DIAG_PUSH pushes the diagnostic settings
+ * DIAG_POP pops the diagnostic settings
+ * DIAG_IGNORE(x) changes the given diagnostic setting to ignore
+ *
+ * Example:
+ *    DIAG_PUSH DIAG_IGNORE(aggregate-return)
+ *    struct timespec ns_to_timespec(const s64 nsec)
+ *    {
+ *        ...
+ *    }
+ *    DIAG_POP
+ *
+ * Will prevent the warning on compilation of the function. Other
+ * steps are necessary to do the same thing for the call sites.
+ */
+#define DIAG_STR(x) #x
+#define DIAG_CAT(x, y) DIAG_STR(x##y)
+#define DIAG_DO_PRAGMA(x) _Pragma(#x)
+#define DIAG_PRAGMA(x) DIAG_DO_PRAGMA(clang diagnostic x)
+#define DIAG_IGNORE(x) DIAG_PRAGMA(ignored DIAG_CAT(-W, x))
+#define DIAG_PUSH DIAG_PRAGMA(push)
+#define DIAG_POP DIAG_PRAGMA(pop)
+#define DIAG_CLANG_IGNORE(x) DIAG_IGNORE(x)
diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h
index 2507fd2..78387f6 100644
--- a/include/linux/compiler-gcc4.h
+++ b/include/linux/compiler-gcc4.h
@@ -86,3 +86,34 @@
 #define __HAVE_BUILTIN_BSWAP16__
 #endif
 #endif /* CONFIG_ARCH_USE_BUILTIN_BSWAP */
+
+/*
+ * Provide macros to manipulate diagnostic messages when possible.
+ * DIAG_PUSH pushes the diagnostic settings
+ * DIAG_POP pops the diagnostic settings
+ * DIAG_IGNORE(x) changes the given diagnostic setting to ignore
+ *
+ * Example:
+ *    DIAG_PUSH DIAG_IGNORE(aggregate-return)
+ *    struct timespec ns_to_timespec(const s64 nsec)
+ *    {
+ *        ...
+ *    }
+ *    DIAG_POP
+ *
+ * Will prevent the warning on compilation of the function. Other
+ * steps are necessary to do the same thing for the call sites.
+ */
+#if GCC_VERSION >= 40600
+#define DIAG_STR(x) #x
+#define DIAG_CAT(x, y) DIAG_STR(x##y)
+#define DIAG_DO_PRAGMA(x) _Pragma(#x)
+#define DIAG_PRAGMA(x) DIAG_DO_PRAGMA(GCC diagnostic x)
+#define DIAG_IGNORE(x) DIAG_PRAGMA(ignored DIAG_CAT(-W, x))
+/* GCC 4.8 - 4.8.2 has issues with pop, so do not define push or pop */
+#if GCC_VERSION < 40800 || GCC_VERSION >= 40803
+#define DIAG_PUSH DIAG_PRAGMA(push)
+#define DIAG_POP DIAG_PRAGMA(pop)
+#endif /* GCC_VERSION < 40800 || GCC_VERSION >= 40803 */
+#define DIAG_GCC_IGNORE(x) DIAG_IGNORE(x)
+#endif /* GCC_VERSION >= 40600 */
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index d5ad7b1..fde7b21 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -94,6 +94,26 @@ struct ftrace_branch_data {
 };

 /*
+ * Provide null definitions for diagnostic manipulation macros not provided
+ * for a particular compiler.
+ */
+#ifndef DIAG_PUSH
+#define DIAG_PUSH
+#endif
+#ifndef DIAG_POP
+#define DIAG_POP
+#endif
+#ifndef DIAG_IGNORE
+#define DIAG_IGNORE(x)
+#endif
+#ifndef DIAG_CLANG_IGNORE
+#define DIAG_CLANG_IGNORE(x)
+#endif
+#ifndef DIAG_GCC_IGNORE
+#define DIAG_GCC_IGNORE(x)
+#endif
+
+/*
  * Note: DISABLE_BRANCH_PROFILING can be used by special lowlevel code
  * to disable branch tracing on a per file basis.
  */

-- 
Cheers,
Jeff

Re: Overriding -Werror

[Brian Norris]

Hi,

On Fri, Aug 15, 2014 at 02:30:49AM -0700, Jeff Kirsher wrote:
> Funny that you bring this up because I have ~60 patches in my queue to
> resolve several thousand of these warnings.  Half of the patches
> actually resolve warnings that can be resolved and the other half
> implement compiler diagnostic control macros to help silence warnings.
> All these were the work of a co-worker Mark Rustad, below is the patch
> he put together to implement the compiler diagnostic control macros.

While fixing warnings is usually a good thing (at least when done right;
there are plenty of ways to fight with the compiler over silly things,
but that's another discussion), I think that my issue is still
orthogonal to the one you're addressing. In my estimation, it is
impossible to guarantee that the entire kernel (including drivers) will
build without any warnings, across all levels of warning verbosity.
Thus, even with a valiant effort to fix or annotate all warnings, we
still won't get to the point where I can build 'make ARCH=mips W=1', if
-Werror is active.

Besides, when testing *new* code, it's even more likely to have new
warnings, and I'd like to see as many as possible, without -Werror
getting in the way.

So I still think -Werror is fundamentally wrong in some cases, and I
would like to pursue some approach like in my original post.

BTW, for a little more context: I realize the output of 'make W=[123]'
may not be very useful on its own, sometimes, but it's actually pretty
useful to quickly catch potential issues in new code, by diff'ing the
warnings in the before/after build logs. In this case, it's not helpful
at all if the first build "fails" due to dubious warnings. I'm doing
this in the context of Aiaiai [1]. Right now, I have to keep around a
few local patches to remove -Werror from arch/{mips,sh}.

> commit 7b9aace02b2405f0714bc08c424b72e6962f1c2e
> Author: Mark Rustad <mark.d.rustad@intel.com>
> Date:   Fri Aug 15 01:43:44 2014 -0700
> 
>     compiler: Add diagnostic control macros
> 
>     Add macros to control diagnostic messages where needed. These
>     are used to silence warning messages that are expected, normal
>     and do not indicate any sort of problem. Reducing the stream
>     of messages in this way helps possible problems to stand out.
> 
>     The macros provided are:
>         DIAG_PUSH() - to save diagnostic settings
>         DIAG_POP() - to restore diagnostic settings
>         DIAG_IGNORE(option) - to ignore a particular warning
>         DIAG_GCC_IGNORE(option) - DIAG_IGNORE for gcc only
>         DIAG_CLANG_IGNORE(option) - DIAG_IGNORE for clang only
> 
>     Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
> 
> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> index d1e49d5..039b112 100644
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> @@ -10,3 +10,29 @@
>  #undef uninitialized_var
>  #define uninitialized_var(x) x = *(&(x))
>  #endif
> +
> +/*
> + * Provide macros to manipulate diagnostic messages when possible.
> + * DIAG_PUSH pushes the diagnostic settings
> + * DIAG_POP pops the diagnostic settings
> + * DIAG_IGNORE(x) changes the given diagnostic setting to ignore
> + *
> + * Example:
> + *    DIAG_PUSH DIAG_IGNORE(aggregate-return)
> + *    struct timespec ns_to_timespec(const s64 nsec)
> + *    {
> + *        ...
> + *    }
> + *    DIAG_POP
> + *
> + * Will prevent the warning on compilation of the function. Other
> + * steps are necessary to do the same thing for the call sites.
> + */

While I do not want to disparage your/Mark's work here, my first thought
about this kind of annotation is that it seems to be a pretty big burden
to have to annotate all code with these sorts of things. While it could
help for some core parts of the kernel that can be closely scrutinized
and defended against false/useless warnings, from my perspective it
seems like people are bound to get it wrong when it comes to the long
tail of scattered device drivers.

But I'm not really the right voice for that topic. Feel free to send
your work and see what the rest of the community thinks.

Thanks,
Brian

[1] http://lwn.net/Articles/488992/
    http://git.infradead.org/aiaiai.git

Re: Overriding -Werror

[Mark D Rustad]

[-- Attachment #1: Type: text/plain, Size: 6629 bytes --]

Brian,

On Aug 15, 2014, at 12:33 PM, Brian Norris <computersforpeace@gmail.com> wrote:

> Hi,
> 
> On Fri, Aug 15, 2014 at 02:30:49AM -0700, Jeff Kirsher wrote:
>> Funny that you bring this up because I have ~60 patches in my queue to
>> resolve several thousand of these warnings.  Half of the patches
>> actually resolve warnings that can be resolved and the other half
>> implement compiler diagnostic control macros to help silence warnings.
>> All these were the work of a co-worker Mark Rustad, below is the patch
>> he put together to implement the compiler diagnostic control macros.
> 
> While fixing warnings is usually a good thing (at least when done right;
> there are plenty of ways to fight with the compiler over silly things,
> but that's another discussion),

I have said at some presentations on the subject that resolving warnings is not something you want an intern to do.

> I think that my issue is still
> orthogonal to the one you're addressing. In my estimation, it is
> impossible to guarantee that the entire kernel (including drivers) will
> build without any warnings, across all levels of warning verbosity.
> Thus, even with a valiant effort to fix or annotate all warnings, we
> still won't get to the point where I can build 'make ARCH=mips W=1', if
> -Werror is active.

Actually, some years ago, I got a MIPS Linux kernel to compile clean with even more warnings than W=12 provides. It can be done, but it certainly is not a state that is required and cannot be maintained across all configurations, architectures and compiler versions. This is the real world.

> Besides, when testing *new* code, it's even more likely to have new
> warnings, and I'd like to see as many as possible, without -Werror
> getting in the way.

I have to say that I rather like -Werror. One thing that not a lot of people are aware of is that you can selectively allow some warnings. -Wno-error=shadow would allow shadow warnings to be reported without being treated as errors.

> So I still think -Werror is fundamentally wrong in some cases, and I
> would like to pursue some approach like in my original post.
> 
> BTW, for a little more context: I realize the output of 'make W=[123]'
> may not be very useful on its own, sometimes, but it's actually pretty
> useful to quickly catch potential issues in new code, by diff'ing the
> warnings in the before/after build logs. In this case, it's not helpful
> at all if the first build "fails" due to dubious warnings. I'm doing
> this in the context of Aiaiai [1]. Right now, I have to keep around a
> few local patches to remove -Werror from arch/{mips,sh}.

The problem is that when a kernel build throws over 125,000 warnings, it just becomes completely useless. That was what kind of set me off. I did wind up pushing this rock further up the hill than I really meant to. Still, I got the build under 1,400 warnings, and I now know how to address most of them in a systematic way.

>> commit 7b9aace02b2405f0714bc08c424b72e6962f1c2e
>> Author: Mark Rustad <mark.d.rustad@intel.com>
>> Date:   Fri Aug 15 01:43:44 2014 -0700
>> 
>>    compiler: Add diagnostic control macros
>> 
>>    Add macros to control diagnostic messages where needed. These
>>    are used to silence warning messages that are expected, normal
>>    and do not indicate any sort of problem. Reducing the stream
>>    of messages in this way helps possible problems to stand out.
>> 
>>    The macros provided are:
>>        DIAG_PUSH() - to save diagnostic settings
>>        DIAG_POP() - to restore diagnostic settings
>>        DIAG_IGNORE(option) - to ignore a particular warning
>>        DIAG_GCC_IGNORE(option) - DIAG_IGNORE for gcc only
>>        DIAG_CLANG_IGNORE(option) - DIAG_IGNORE for clang only
>> 
>>    Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
>> 
>> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
>> index d1e49d5..039b112 100644
>> --- a/include/linux/compiler-clang.h
>> +++ b/include/linux/compiler-clang.h
>> @@ -10,3 +10,29 @@
>> #undef uninitialized_var
>> #define uninitialized_var(x) x = *(&(x))
>> #endif
>> +
>> +/*
>> + * Provide macros to manipulate diagnostic messages when possible.
>> + * DIAG_PUSH pushes the diagnostic settings
>> + * DIAG_POP pops the diagnostic settings
>> + * DIAG_IGNORE(x) changes the given diagnostic setting to ignore
>> + *
>> + * Example:
>> + *    DIAG_PUSH DIAG_IGNORE(aggregate-return)
>> + *    struct timespec ns_to_timespec(const s64 nsec)
>> + *    {
>> + *        ...
>> + *    }
>> + *    DIAG_POP
>> + *
>> + * Will prevent the warning on compilation of the function. Other
>> + * steps are necessary to do the same thing for the call sites.
>> + */
> 
> While I do not want to disparage your/Mark's work here, my first thought
> about this kind of annotation is that it seems to be a pretty big burden
> to have to annotate all code with these sorts of things.

I wouldn't suggest annotating everything. However note that the annotations can serve as a notice that something has been analyzed and deemed ok. That can be useful as long as that is really true. I wouldn't take new code from a new developer that included such annotations.

> While it could
> help for some core parts of the kernel that can be closely scrutinized
> and defended against false/useless warnings, from my perspective it
> seems like people are bound to get it wrong when it comes to the long
> tail of scattered device drivers.

Yes, it is VERY valuable for addressing warnings thrown by core kernel includes. Just a few static inlines with warnings produced about 50,000 of those 125,000 warnings.

> But I'm not really the right voice for that topic. Feel free to send
> your work and see what the rest of the community thinks.

I look forward to the discussion. I have no illusion about all my patches going in. In fact there are instances where I used the diagnostic control macros to silence a bunch of warnings because it made for a much smaller patch - a patch to resolve them would have had to replace an entire table, so I guess I hope to get a request to do that. The exchanges I have had with the few patches that have been sent thus far have been productive, so I hope for more like that.

I do think that the diagnostic control macros serve a useful purpose if used well. I agree that they can be abused, like anything else, but some rules about their use should help. Maybe checkpatch should warn on patches that add them...

-- 
Mark Rustad, MRustad@gmail.com


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: Overriding -Werror

[Brian Norris]

Hi Mark,

(BTW, your mailer is creating some pretty long, unwrapped lines. I've
rewrapped them when quoting below.)

On Fri, Aug 15, 2014 at 08:36:07PM -0700, Mark D Rustad wrote:
> On Aug 15, 2014, at 12:33 PM, Brian Norris <computersforpeace@gmail.com> wrote:
> > On Fri, Aug 15, 2014 at 02:30:49AM -0700, Jeff Kirsher wrote:
> >> Funny that you bring this up because I have ~60 patches in my queue to
> >> resolve several thousand of these warnings.  Half of the patches
> >> actually resolve warnings that can be resolved and the other half
> >> implement compiler diagnostic control macros to help silence warnings.
> >> All these were the work of a co-worker Mark Rustad, below is the patch
> >> he put together to implement the compiler diagnostic control macros.
> > 
> > While fixing warnings is usually a good thing (at least when done right;
> > there are plenty of ways to fight with the compiler over silly things,
> > but that's another discussion),
> 
> I have said at some presentations on the subject that resolving
> warnings is not something you want an intern to do.

Nice.

> > I think that my issue is still
> > orthogonal to the one you're addressing. In my estimation, it is
> > impossible to guarantee that the entire kernel (including drivers) will
> > build without any warnings, across all levels of warning verbosity.
> > Thus, even with a valiant effort to fix or annotate all warnings, we
> > still won't get to the point where I can build 'make ARCH=mips W=1', if
> > -Werror is active.
> 
> Actually, some years ago, I got a MIPS Linux kernel to compile clean
> with even more warnings than W=12 provides.

Congrats!

> It can be done, but it certainly is not a state that is required and
> cannot be maintained across all configurations, architectures and
> compiler versions. This is the real world.

Right, and this is the crux of why I would like to have the option of
disabling -Werror systematically.

> > Besides, when testing *new* code, it's even more likely to have new
> > warnings, and I'd like to see as many as possible, without -Werror
> > getting in the way.
> 
> I have to say that I rather like -Werror.

It has its uses. I think it can be a pretty good option whenever the
compiler's warning level is kept to a reasonable level.

-Wextra, for instance, enables a lot of warnings that can be problematic
for little practical benefit (like -Wsign-compare, which notably is
explicitly overridden for x86).

> One thing that not a lot of people are aware of is that you can
> selectively allow some warnings. -Wno-error=shadow would allow shadow
> warnings to be reported without being treated as errors.

That's interesting. I glazed over this option because I misinterpreted
the second sentence of this note in the gcc manpage:

  "Note that specifying -Werror=foo automatically implies -Wfoo.
  However, -Wno-error=foo does not imply anything."

[ Really, -Wno-error=foo doesn't imply anything? So why does it exist? ;) ]

But now that you mention it, I think it could work as a hack for my
builds right now, since it isn't overridden by a blanket -Werror found
later on the command linie. So if I do

  make ARCH=mips W=1 KCFLAGS="-Wno-error=...(long list of warnings that break the build)..."

then I can systematically perform the build-tests I'd like.

> > So I still think -Werror is fundamentally wrong in some cases, and I
> > would like to pursue some approach like in my original post.
> > 
> > BTW, for a little more context: I realize the output of 'make W=[123]'
> > may not be very useful on its own, sometimes, but it's actually pretty
> > useful to quickly catch potential issues in new code, by diff'ing the
> > warnings in the before/after build logs. In this case, it's not helpful
> > at all if the first build "fails" due to dubious warnings. I'm doing
> > this in the context of Aiaiai [1]. Right now, I have to keep around a
> > few local patches to remove -Werror from arch/{mips,sh}.
> 
> The problem is that when a kernel build throws over 125,000 warnings,
> it just becomes completely useless. That was what kind of set me off.

Yeah that's bad. But it *still* can provide a helpful diff for tools
like Aiaiai. The difference between 125,000 and 125,001 warnings still
can be determined automatically for new code, although it's not helpful
if every "new" warning is actually just because you used a new core
header that causes a lot of warnings.

> I did wind up pushing this rock further up the hill than I really
> meant to. Still, I got the build under 1,400 warnings, and I now know
> how to address most of them in a systematic way.
> 
> >> commit 7b9aace02b2405f0714bc08c424b72e6962f1c2e
> >> Author: Mark Rustad <mark.d.rustad@intel.com>
> >> Date:   Fri Aug 15 01:43:44 2014 -0700
> >> 
> >>    compiler: Add diagnostic control macros
> >> 
> >>    Add macros to control diagnostic messages where needed. These
> >>    are used to silence warning messages that are expected, normal
> >>    and do not indicate any sort of problem. Reducing the stream
> >>    of messages in this way helps possible problems to stand out.
> >> 
> >>    The macros provided are:
> >>        DIAG_PUSH() - to save diagnostic settings
> >>        DIAG_POP() - to restore diagnostic settings
> >>        DIAG_IGNORE(option) - to ignore a particular warning
> >>        DIAG_GCC_IGNORE(option) - DIAG_IGNORE for gcc only
> >>        DIAG_CLANG_IGNORE(option) - DIAG_IGNORE for clang only
> >> 
> >>    Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
> >> 
> >> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> >> index d1e49d5..039b112 100644
> >> --- a/include/linux/compiler-clang.h
> >> +++ b/include/linux/compiler-clang.h
> >> @@ -10,3 +10,29 @@
> >> #undef uninitialized_var
> >> #define uninitialized_var(x) x = *(&(x))
> >> #endif
> >> +
> >> +/*
> >> + * Provide macros to manipulate diagnostic messages when possible.
> >> + * DIAG_PUSH pushes the diagnostic settings
> >> + * DIAG_POP pops the diagnostic settings
> >> + * DIAG_IGNORE(x) changes the given diagnostic setting to ignore
> >> + *
> >> + * Example:
> >> + *    DIAG_PUSH DIAG_IGNORE(aggregate-return)
> >> + *    struct timespec ns_to_timespec(const s64 nsec)
> >> + *    {
> >> + *        ...
> >> + *    }
> >> + *    DIAG_POP
> >> + *
> >> + * Will prevent the warning on compilation of the function. Other
> >> + * steps are necessary to do the same thing for the call sites.
> >> + */
> > 
> > While I do not want to disparage your/Mark's work here, my first thought
> > about this kind of annotation is that it seems to be a pretty big burden
> > to have to annotate all code with these sorts of things.
> 
> I wouldn't suggest annotating everything. However note that the
> annotations can serve as a notice that something has been analyzed and
> deemed ok. That can be useful as long as that is really true. I
> wouldn't take new code from a new developer that included such
> annotations.

Good points. And I'd recommend framing your argument as such if/when you
post your patches for real. Maybe even add comments near the macro
definitions; "with great power comes great responsibility."

Thanks for your thoughts,
Brian

Re: Overriding -Werror

[Mark D Rustad]

[-- Attachment #1: Type: text/plain, Size: 9434 bytes --]

On Aug 15, 2014, at 9:34 PM, Brian Norris <computersforpeace@gmail.com> wrote:

> Hi Mark,
> 
> (BTW, your mailer is creating some pretty long, unwrapped lines. I've
> rewrapped them when quoting below.)

Sorry about that. I'll try to remember to deal with it on my end.

> On Fri, Aug 15, 2014 at 08:36:07PM -0700, Mark D Rustad wrote:
>> On Aug 15, 2014, at 12:33 PM, Brian Norris <computersforpeace@gmail.com> wrote:
>>> On Fri, Aug 15, 2014 at 02:30:49AM -0700, Jeff Kirsher wrote:
>>>> Funny that you bring this up because I have ~60 patches in my queue to
>>>> resolve several thousand of these warnings.  Half of the patches
>>>> actually resolve warnings that can be resolved and the other half
>>>> implement compiler diagnostic control macros to help silence warnings.
>>>> All these were the work of a co-worker Mark Rustad, below is the patch
>>>> he put together to implement the compiler diagnostic control macros.
>>> 
>>> While fixing warnings is usually a good thing (at least when done right;
>>> there are plenty of ways to fight with the compiler over silly things,
>>> but that's another discussion),
>> 
>> I have said at some presentations on the subject that resolving
>> warnings is not something you want an intern to do.
> 
> Nice.
> 
>>> I think that my issue is still
>>> orthogonal to the one you're addressing. In my estimation, it is
>>> impossible to guarantee that the entire kernel (including drivers) will
>>> build without any warnings, across all levels of warning verbosity.
>>> Thus, even with a valiant effort to fix or annotate all warnings, we
>>> still won't get to the point where I can build 'make ARCH=mips W=1', if
>>> -Werror is active.
>> 
>> Actually, some years ago, I got a MIPS Linux kernel to compile clean
>> with even more warnings than W=12 provides.
> 
> Congrats!
> 
>> It can be done, but it certainly is not a state that is required and
>> cannot be maintained across all configurations, architectures and
>> compiler versions. This is the real world.
> 
> Right, and this is the crux of why I would like to have the option of
> disabling -Werror systematically.
> 
>>> Besides, when testing *new* code, it's even more likely to have new
>>> warnings, and I'd like to see as many as possible, without -Werror
>>> getting in the way.
>> 
>> I have to say that I rather like -Werror.
> 
> It has its uses. I think it can be a pretty good option whenever the
> compiler's warning level is kept to a reasonable level.
> 
> -Wextra, for instance, enables a lot of warnings that can be problematic
> for little practical benefit (like -Wsign-compare, which notably is
> explicitly overridden for x86).

Hmm. I wasn't aware that x86 had that disabled. Sign-compare actually does
find some kinds of bugs. Those warnings are worth resolving for that reason.
Being explicit about whether you want the signed or unsigned comparison is
a good thing.

>> One thing that not a lot of people are aware of is that you can
>> selectively allow some warnings. -Wno-error=shadow would allow shadow
>> warnings to be reported without being treated as errors.
> 
> That's interesting. I glazed over this option because I misinterpreted
> the second sentence of this note in the gcc manpage:
> 
>  "Note that specifying -Werror=foo automatically implies -Wfoo.
>  However, -Wno-error=foo does not imply anything."
> 
> [ Really, -Wno-error=foo doesn't imply anything? So why does it exist? ;) ]

Yeah, it means that it doesn't imply anything about whether the warning
is enabled or not. It will neither enable nor disable the warning as a
side-effect, unlike -Werror= which will enable it. So you can add -Wno
error=... without regard to whether the given warning was enabled or
not. It kind of makes sense, but is not explained very well.

> But now that you mention it, I think it could work as a hack for my
> builds right now, since it isn't overridden by a blanket -Werror found
> later on the command linie. So if I do
> 
>  make ARCH=mips W=1 KCFLAGS="-Wno-error=...(long list of warnings that break the build)..."

And still be able to see them. Not bad.

> then I can systematically perform the build-tests I'd like.

Yes.

>>> So I still think -Werror is fundamentally wrong in some cases, and I
>>> would like to pursue some approach like in my original post.
>>> 
>>> BTW, for a little more context: I realize the output of 'make W=[123]'
>>> may not be very useful on its own, sometimes, but it's actually pretty
>>> useful to quickly catch potential issues in new code, by diff'ing the
>>> warnings in the before/after build logs. In this case, it's not helpful
>>> at all if the first build "fails" due to dubious warnings. I'm doing
>>> this in the context of Aiaiai [1]. Right now, I have to keep around a
>>> few local patches to remove -Werror from arch/{mips,sh}.
>> 
>> The problem is that when a kernel build throws over 125,000 warnings,
>> it just becomes completely useless. That was what kind of set me off.
> 
> Yeah that's bad. But it *still* can provide a helpful diff for tools
> like Aiaiai. The difference between 125,000 and 125,001 warnings still
> can be determined automatically for new code, although it's not helpful
> if every "new" warning is actually just because you used a new core
> header that causes a lot of warnings.

Yes, but it is still a bad situation to have so many. And a lot of them
are trivial things like nested externs in static inline functions in
include files. I'm not wild about the nested externs - it means that
there are multiple sources for the prototype of those functions - but
since that practice has now been used to disentangle include files, we
may as well silence the warnings for those usages. The annotations might
also be a bit of an ongoing reminder that there are issues with having
done that.

>> I did wind up pushing this rock further up the hill than I really
>> meant to. Still, I got the build under 1,400 warnings, and I now know
>> how to address most of them in a systematic way.
>> 
>>>> commit 7b9aace02b2405f0714bc08c424b72e6962f1c2e
>>>> Author: Mark Rustad <mark.d.rustad@intel.com>
>>>> Date:   Fri Aug 15 01:43:44 2014 -0700
>>>> 
>>>>   compiler: Add diagnostic control macros
>>>> 
>>>>   Add macros to control diagnostic messages where needed. These
>>>>   are used to silence warning messages that are expected, normal
>>>>   and do not indicate any sort of problem. Reducing the stream
>>>>   of messages in this way helps possible problems to stand out.
>>>> 
>>>>   The macros provided are:
>>>>       DIAG_PUSH() - to save diagnostic settings
>>>>       DIAG_POP() - to restore diagnostic settings
>>>>       DIAG_IGNORE(option) - to ignore a particular warning
>>>>       DIAG_GCC_IGNORE(option) - DIAG_IGNORE for gcc only
>>>>       DIAG_CLANG_IGNORE(option) - DIAG_IGNORE for clang only
>>>> 
>>>>   Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
>>>> 
>>>> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
>>>> index d1e49d5..039b112 100644
>>>> --- a/include/linux/compiler-clang.h
>>>> +++ b/include/linux/compiler-clang.h
>>>> @@ -10,3 +10,29 @@
>>>> #undef uninitialized_var
>>>> #define uninitialized_var(x) x = *(&(x))
>>>> #endif
>>>> +
>>>> +/*
>>>> + * Provide macros to manipulate diagnostic messages when possible.
>>>> + * DIAG_PUSH pushes the diagnostic settings
>>>> + * DIAG_POP pops the diagnostic settings
>>>> + * DIAG_IGNORE(x) changes the given diagnostic setting to ignore
>>>> + *
>>>> + * Example:
>>>> + *    DIAG_PUSH DIAG_IGNORE(aggregate-return)
>>>> + *    struct timespec ns_to_timespec(const s64 nsec)
>>>> + *    {
>>>> + *        ...
>>>> + *    }
>>>> + *    DIAG_POP
>>>> + *
>>>> + * Will prevent the warning on compilation of the function. Other
>>>> + * steps are necessary to do the same thing for the call sites.
>>>> + */
>>> 
>>> While I do not want to disparage your/Mark's work here, my first thought
>>> about this kind of annotation is that it seems to be a pretty big burden
>>> to have to annotate all code with these sorts of things.
>> 
>> I wouldn't suggest annotating everything. However note that the
>> annotations can serve as a notice that something has been analyzed and
>> deemed ok. That can be useful as long as that is really true. I
>> wouldn't take new code from a new developer that included such
>> annotations.
> 
> Good points. And I'd recommend framing your argument as such if/when you
> post your patches for real. Maybe even add comments near the macro
> definitions; "with great power comes great responsibility."

That is not a bad idea. And I do think that checkpatch ought to warn when
they are added. It certainly is a bigger deal than a too long line!

I am contemplating adding a DIAG_ERROR macro to enable an error on a warning.
The use case I have come up with is switch-enum. Then, switch statements
that are expected to itemize every value in an enum can force that error on
for just those switch statements, because generally that is not needed. At
least that is the best possible use I have come up with for that kind of
macro. Haven't tried it yet, but that seems potentially useful to me.

> Thanks for your thoughts,
> Brian

-- 
Mark Rustad, MRustad@gmail.com


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: Overriding -Werror

[Lennart Sorensen]

On Thu, Aug 14, 2014 at 03:21:19PM -0700, Brian Norris wrote:
> I'm interested in being able to build-test kernels on various
> architectures while enabling extra warnings (make W=[123]). I'd like to
> be able to finish the builds and see all warnings, rather than seeing a
> failed build. However, GCC's -Werror is incompatible with this. There is
> plenty of code that will produce at least one warning, when warning
> verbosity is turned up. And GCC's -Werror is not guaranteed to remain
> stable over time; new versions may develop new warnings that may or may
> not be legitimate.

What ever is wrong with using '-k' with your make command?

-- 
Len Sorensen

Re: Overriding -Werror

[Geert Uytterhoeven]

On Fri, Aug 15, 2014 at 5:33 PM, Lennart Sorensen
<lsorense@csclub.uwaterloo.ca> wrote:
> On Thu, Aug 14, 2014 at 03:21:19PM -0700, Brian Norris wrote:
>> I'm interested in being able to build-test kernels on various
>> architectures while enabling extra warnings (make W=[123]). I'd like to
>> be able to finish the builds and see all warnings, rather than seeing a
>> failed build. However, GCC's -Werror is incompatible with this. There is
>> plenty of code that will produce at least one warning, when warning
>> verbosity is turned up. And GCC's -Werror is not guaranteed to remain
>> stable over time; new versions may develop new warnings that may or may
>> not be legitimate.
>
> What ever is wrong with using '-k' with your make command?

While -k helps to get more compiled, and thus see more warnings, you'll still
be missing the report of missing symbols in the final link/modpost stage.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Overriding -Werror

[Andi Kleen]

Brian Norris <computersforpeace@gmail.com> writes:
>
> 4. better ideas?

Just send patches to remove -Werror in all architectures
as a tree sweep (and anywhere else where someone misguided add it)

Having -Werror anywhere in a shipping release is just plainly a bug,
as it makes it often impossible to build on newer gcc versions.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only

Re: Overriding -Werror

[Brian Norris]

On Tue, Aug 19, 2014 at 06:15:07AM -0700, Andi Kleen wrote:
> Brian Norris <computersforpeace@gmail.com> writes:
> >
> > 4. better ideas?
> 
> Just send patches to remove -Werror in all architectures
> as a tree sweep (and anywhere else where someone misguided add it)

I cited at least one example in which this was attempted but rejected.
(Or at least, it was semi-deliberately stalled.)

> Having -Werror anywhere in a shipping release is just plainly a bug,
> as it makes it often impossible to build on newer gcc versions.

I also feel it is misguided, for other reasons. But there are at least a
few who seem to disagree. They seem to feel it is important/necessary
for enforcing good practices during development. I'm not sure how to
bridge the disagreement, other than to provide workarounds for disabling
-Werror.

I could try sending -Werror removal patches, but I'm not confident that
will go over well.

Brian

Re: Overriding -Werror

[Sam Ravnborg]

On Tue, Aug 19, 2014 at 06:15:07AM -0700, Andi Kleen wrote:
> Brian Norris <computersforpeace@gmail.com> writes:
> >
> > 4. better ideas?
> 
> Just send patches to remove -Werror in all architectures
> as a tree sweep (and anywhere else where someone misguided add it)
In arch/sparc/ we have -Werror and this has never troubled us.
In fact more than once people had tried to provide sloppy code that
failed due to -Werror so it got fixed up front.

	Sam

Re: Overriding -Werror

[Andi Kleen]

On Tue, Aug 19, 2014 at 10:11:30PM +0200, Sam Ravnborg wrote:
> On Tue, Aug 19, 2014 at 06:15:07AM -0700, Andi Kleen wrote:
> > Brian Norris <computersforpeace@gmail.com> writes:
> > >
> > > 4. better ideas?
> > 
> > Just send patches to remove -Werror in all architectures
> > as a tree sweep (and anywhere else where someone misguided add it)
> In arch/sparc/ we have -Werror and this has never troubled us.
> In fact more than once people had tried to provide sloppy code that
> failed due to -Werror so it got fixed up front.

It's a major mistake to not support future gccs for released source.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: Overriding -Werror

[Sam Ravnborg]

On Wed, Aug 20, 2014 at 01:43:59AM +0200, Andi Kleen wrote:
> On Tue, Aug 19, 2014 at 10:11:30PM +0200, Sam Ravnborg wrote:
> > On Tue, Aug 19, 2014 at 06:15:07AM -0700, Andi Kleen wrote:
> > > Brian Norris <computersforpeace@gmail.com> writes:
> > > >
> > > > 4. better ideas?
> > > 
> > > Just send patches to remove -Werror in all architectures
> > > as a tree sweep (and anywhere else where someone misguided add it)
> > In arch/sparc/ we have -Werror and this has never troubled us.
> > In fact more than once people had tried to provide sloppy code that
> > failed due to -Werror so it got fixed up front.
> 
> It's a major mistake to not support future gccs for released source.
Until now this has not been an issue - for the sparc bits.

If newer gcc's turn out to give legit warnings then if someone provides
a little help (configure command-line to build gcc) the warnings will
be fixed. And that should hold true for any decent maintained piece of SW.

If the intent is to use bleeding edge gcc then -Werrror can be annoying
and the right fix would be to make it possible to diable it on the
command-line.

	Sam