| Message ID | 1426631862-19563-1-git-send-email-jtoppins@cumulusnetworks.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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(+)