[PATCH v1 kbuild for-next 1/2] makefiles: add config option to force all cc warnings to errors

[PATCH v1 kbuild for-next 1/2] makefiles: add config option to force all cc warnings to errors

[Jonathan Toppins]

Add a Kconfig option to force cc warnings to errors. Also, to promote
better code hygiene enable the configuration option by default to force
authors to take some action. If for some reason a compilation unit
cannot get rid of the error the author has the option to suppress the
warning for that compile unit or module, see
Documentation/kbuild/makefiles.txt for details.

Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
---
 Makefile          |    4 ++++
 lib/Kconfig.debug |   14 ++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/Makefile b/Makefile
index abf7bb1..eb47733 100644
--- a/Makefile
+++ b/Makefile
@@ -764,6 +764,10 @@ ifdef CONFIG_DEBUG_SECTION_MISMATCH
 KBUILD_CFLAGS += $(call cc-option, -fno-inline-functions-called-once)
 endif
 
+ifdef CONFIG_DEBUG_FORCE_CC_WARNINGS_TO_ERRORS
+KBUILD_CFLAGS += $(call cc-option, -Werror)
+endif
+
 # arch Makefile may override CC so keep this after arch Makefile is included
 NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include)
 CHECKFLAGS     += $(NOSTDINC_FLAGS)
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 3e0289e..0b288db 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -347,6 +347,20 @@ config DEBUG_FORCE_WEAK_PER_CPU
 	  To ensure that generic code follows the above rules, this
 	  option forces all percpu variables to be defined as weak.
 
+config DEBUG_FORCE_CC_WARNINGS_TO_ERRORS
+	bool "Force cc warnings to errors"
+	default y
+	help
+	  Simply enables the gcc compiler option -Werror for the entire
+	  build. If a compilation unit cannot handle -Werror by fixing the
+	  warning then that unit must suppress the cc warning using
+	  cc-disable-warning for that compilation unit in the unit's makefile.
+	  .
+	  This option is intended to be more in the developer's face and
+	  encourage effort of some kind to remove the compilation warning.
+	  .
+	  If unsure say y.
+
 endmenu # "Compiler options"
 
 config MAGIC_SYSRQ
-- 
1.7.10.4

[PATCH v1 kbuild for-next 2/2] modpost: add Kconfig option to report warnings as errors

[Jonathan Toppins]

Add a Kconfig option to cause modpost to report warnings as errors.
This is a simplistic implementation in that modpost only reports the
first warning as an error and subsequent unreported warnings that will
kill the build may still exist.

Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
---
 lib/Kconfig.debug     |   14 ++++++++++++++
 scripts/mod/modpost.c |    5 +++++
 2 files changed, 19 insertions(+)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 0b288db..c419138 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -361,6 +361,20 @@ config DEBUG_FORCE_CC_WARNINGS_TO_ERRORS
 	  .
 	  If unsure say y.
 
+config DEBUG_FORCE_MODPOST_WARNINGS_TO_ERRORS
+	bool "Force modpost warnings to errors"
+	default n
+	help
+	  Force warnings generated by modpost to be reported as errors.
+	  .
+	  Another build time setting to encourage some action be taken by
+	  the developer to fix the problem. Since there is not an easy
+	  suppression mechanism for things that are reported as warnings
+	  but are determined to be acceptable the default is to not enable
+	  this option.
+	  .
+	  If unsure say n.
+
 endmenu # "Compiler options"
 
 config MAGIC_SYSRQ
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index d439856..d430eb6 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -70,6 +70,11 @@ PRINTF void warn(const char *fmt, ...)
 	va_start(arglist, fmt);
 	vfprintf(stderr, fmt, arglist);
 	va_end(arglist);
+
+#ifdef CONFIG_DEBUG_FORCE_MODPOST_WARNINGS_TO_ERRORS
+	fprintf(stderr, "modpost: warnings treated as errors.\n");
+	exit(1);
+#endif
 }
 
 PRINTF void merror(const char *fmt, ...)
-- 
1.7.10.4

Re: [PATCH v1 kbuild for-next 1/2] makefiles: add config option to force all cc warnings to errors

[Paul Bolle]

On Tue, 2015-03-17 at 15:37 -0700, Jonathan Toppins wrote:
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> +config DEBUG_FORCE_CC_WARNINGS_TO_ERRORS
> +	bool "Force cc warnings to errors"
> +	default y

No way.

> +	help
> +	  Simply enables the gcc compiler option -Werror for the entire
> +	  build. If a compilation unit cannot handle -Werror by fixing the
> +	  warning then that unit must suppress the cc warning using
> +	  cc-disable-warning for that compilation unit in the unit's makefile.
> +	  .

(Why the dot?)

> +	  This option is intended to be more in the developer's face and
> +	  encourage effort of some kind to remove the compilation warning.
> +	  .

(Dot?)

> +	  If unsure say y.

Again, no way.

> +
>  endmenu # "Compiler options"
>  
>  config MAGIC_SYSRQ

Feel free to fix as many build warning as you can. I'd really appreciate
that. But my x86_64 build of v4.0-rc4 is _almost_ warning free. And
that's nice. And I find -Werror (and littering Makefiles with
cc-disable-warning) just to remove the few warnings I still see plain
silly. I'm sure the same holds for other people and their builds too. 

(This can be summarized as: NAK.)


Paul Bolle

Re: [PATCH v1 kbuild for-next 1/2] makefiles: add config option to force all cc warnings to errors

[Jonathan Toppins]

On 3/17/15 6:58 PM, Paul Bolle wrote:
> On Tue, 2015-03-17 at 15:37 -0700, Jonathan Toppins wrote:
>> --- a/lib/Kconfig.debug
>> +++ b/lib/Kconfig.debug
>> +config DEBUG_FORCE_CC_WARNINGS_TO_ERRORS
>> +	bool "Force cc warnings to errors"
>> +	default y
>
> No way.

So default to "n"?

>
>> +	help
>> +	  Simply enables the gcc compiler option -Werror for the entire
>> +	  build. If a compilation unit cannot handle -Werror by fixing the
>> +	  warning then that unit must suppress the cc warning using
>> +	  cc-disable-warning for that compilation unit in the unit's makefile.
>> +	  .
>
> (Why the dot?)

I am probably confusing the need for the dot in Debian control files 
with Kconfig files. Kconfig bases paragraph continuation on indention, 
correct?

>
>> +	  This option is intended to be more in the developer's face and
>> +	  encourage effort of some kind to remove the compilation warning.
>> +	  .
>
> (Dot?)
>
>> +	  If unsure say y.
>
> Again, no way.
>
>> +
>>   endmenu # "Compiler options"
>>
>>   config MAGIC_SYSRQ
>
> Feel free to fix as many build warning as you can. I'd really appreciate
> that. But my x86_64 build of v4.0-rc4 is _almost_ warning free. And
> that's nice. And I find -Werror (and littering Makefiles with
> cc-disable-warning) just to remove the few warnings I still see plain
> silly. I'm sure the same holds for other people and their builds too.
>
> (This can be summarized as: NAK.)
>
>
> Paul Bolle
>

Appreciate the review thanks.

-Jon

Re: [PATCH v1 kbuild for-next 1/2] makefiles: add config option to force all cc warnings to errors

[Jonathan Toppins]

On 3/17/15 6:58 PM, Paul Bolle wrote:
> On Tue, 2015-03-17 at 15:37 -0700, Jonathan Toppins wrote:
>> --- a/lib/Kconfig.debug
>> +++ b/lib/Kconfig.debug
>> +config DEBUG_FORCE_CC_WARNINGS_TO_ERRORS
>> +	bool "Force cc warnings to errors"
>> +	default y
>
> No way.
>
>> +	help
>> +	  Simply enables the gcc compiler option -Werror for the entire
>> +	  build. If a compilation unit cannot handle -Werror by fixing the
>> +	  warning then that unit must suppress the cc warning using
>> +	  cc-disable-warning for that compilation unit in the unit's makefile.
>> +	  .
>
> (Why the dot?)
>
>> +	  This option is intended to be more in the developer's face and
>> +	  encourage effort of some kind to remove the compilation warning.
>> +	  .
>
> (Dot?)
>
>> +	  If unsure say y.
>
> Again, no way.
>
>> +
>>   endmenu # "Compiler options"
>>
>>   config MAGIC_SYSRQ
>
> Feel free to fix as many build warning as you can. I'd really appreciate
> that. But my x86_64 build of v4.0-rc4 is _almost_ warning free. And
> that's nice. And I find -Werror (and littering Makefiles with
> cc-disable-warning) just to remove the few warnings I still see plain
> silly. I'm sure the same holds for other people and their builds too.

Please note, I was not trying to imply using cc-disable-warning was a 
first resort option, sorry if it seemed like that. In fact in my opinion 
cc-disable-warning should almost never be used. Do you have a suggestion 
for better wording of this?

Some slight background on these patches, they were born out of the team 
here wanting to have a simple way of easily catching warnings during 
driver development. This seemed like the least cumbersome way. I 
understand if defaulting to yes is not advisable.

Will be happy to submit another patch defaulting to no and clean-up the 
Dots in the paragraphs. From the comments provided so far this approach 
would seem to address them, unless I am misunderstanding and in fact the 
general idea of compiling with -Werror is not wanted.

Appreciate the discussion.

-Jon

Re: [PATCH v1 kbuild for-next 1/2] makefiles: add config option to force all cc warnings to errors

[Michal Marek]

On 2015-03-17 23:58, Paul Bolle wrote:
> Feel free to fix as many build warning as you can. I'd really appreciate
> that. But my x86_64 build of v4.0-rc4 is _almost_ warning free. And
> that's nice. And I find -Werror (and littering Makefiles with
> cc-disable-warning) just to remove the few warnings I still see plain
> silly. I'm sure the same holds for other people and their builds too. 

In addition to what Paul wrote, to review and fix warnings, it's IMO
much more practical to grep the build logs than have the build stop at
the first warning encountered.

Michal

Re: [PATCH v1 kbuild for-next 1/2] makefiles: add config option to force all cc warnings to errors

[Paul Bolle]

Jonathan Toppins schreef op di 17-03-2015 om 19:15 [-0400]:
> So default to "n"?

"n" is the default anyhow, so I think you never need it.

> Kconfig bases paragraph continuation on indention, 
> correct?

Yes. It's a syntax I quite dislike, but we're stuck with it.

> Appreciate the review thanks.

My review ended up a bit blunt. But the patch defaults to y for every
configuration (because, I think, all of them source lib/Kconfig.debug)
for an option that would stop the build at the first warning it sees.
That's not really a considerate thing to do.


Paul Bolle

Re: [PATCH v1 kbuild for-next 1/2] makefiles: add config option to force all cc warnings to errors

[Paul Bolle]

Jonathan Toppins schreef op di 17-03-2015 om 22:05 [-0400]:
> Some slight background on these patches, they were born out of the team 
> here wanting to have a simple way of easily catching warnings during 
> driver development. This seemed like the least cumbersome way. I 
> understand if defaulting to yes is not advisable.
> 
> Will be happy to submit another patch defaulting to no and clean-up the 
> Dots in the paragraphs. From the comments provided so far this approach 
> would seem to address them, unless I am misunderstanding and in fact the 
> general idea of compiling with -Werror is not wanted.
> 
> Appreciate the discussion.

The builds are rather silent as is. Warnings already tend to stand out.
But to follow up on Michal's suggestion, every now and then I do
    grep ":[[:digit:]]\+:" $LOG

to catch all well behaved warnings. (I typed that from memory, so the
pattern might be wrong.) The occasional
    make $SOME_TARGET > /dev/null

does wonders too.

And, on a general note, the warnings spotting real issues get fixed
quite quickly. The warnings that actually do make it into releases are
almost always false positives. They might only get fixed if someone
finds a way to redo the code in such a way that the compiler has less
trouble analyzing it.

Thanks,


Paul Bolle