| Message ID | 10821353.mAlCTdTQBC@wuerfel (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi Arnd, Thanks for your patch. Acked-by: Daniel Axtens <dja@axtens.net> > Alternatively, remove the -Werror. We occasionally get people that add this > flag to a Makefile, but it tends to cause more trouble whenever a new > gcc version arrives. Speaking up as the person who added -Werror to cxl, I'd really rather it stayed. There are a number of reasons I think this. Here's the first three that came to mind. - cxl is powerpc specific (and always will be for deep seated hardware reasons), and is handled through the powerpc tree. arch/powerpc compiles with -Werror, and as part of the powerpc ecosystem, cxl should too. - It forces cxl developers to a higher standard. cxl has already had more than it's fair share of incredibly difficult to debug issues, so any way we can reduce the risk of errors going in makes our lives (and our end-users lives) better. - I am (and I'm quite confident the other cxl people are) quite happy to send patches to fix build-breaking issues such as this. Indeed, I would have, except you sent it during the Australian night :) If it's really super-duper important we can consider putting it behind a config guard, but I'd really rather not. Regards, Daniel
On Fri, Jan 08, 2016 at 09:51:35AM +1100, Daniel Axtens wrote: > > > Alternatively, remove the -Werror. We occasionally get people that add this > > flag to a Makefile, but it tends to cause more trouble whenever a new > > gcc version arrives. ^^ Your reasons below don't really address this point. No matter how well you patch a later kernel release, you can't fix a problem in an existing kernel release that is triggered by a new warning in a new compiler. This shouldn't cause a build failure. > Speaking up as the person who added -Werror to cxl, I'd really rather > it stayed. There are a number of reasons I think this. Here's the first > three that came to mind. > > - cxl is powerpc specific (and always will be for deep seated hardware > reasons), and is handled through the powerpc tree. arch/powerpc > compiles with -Werror, and as part of the powerpc ecosystem, cxl > should too. > > - It forces cxl developers to a higher standard. cxl has already had > more than it's fair share of incredibly difficult to debug issues, > so any way we can reduce the risk of errors going in makes our lives > (and our end-users lives) better. One problem with this point: not all warnings are under the purview of cxl developers. For instance, if I turn up warning verbosity (W=1), then the *header* files start producing plenty of warnings. Should this break the build? Your code didn't change, and you can't fix those errors. That is a real use case for me daily: I turn the warning verbosity up on my compile tests, then (smart)diff the build logs before and after new patches. That way, I can see what new warnings (even potentially false positive ones) are introduced. I can't do that if every random developer wants to stick -Werror in their Makefile. > - I am (and I'm quite confident the other cxl people are) quite happy to > send patches to fix build-breaking issues such as this. Indeed, I > would have, except you sent it during the Australian night :) > > If it's really super-duper important we can consider putting it behind a > config guard, but I'd really rather not. I think there are plenty of reasons to either remove -Werror, or make it configurable. Some of them are detailed above. Maybe you can gate the -Werror on CONFIG_PPC_WERROR, just like the rest of PowerPC? Brian
Excerpts from Brian Norris's message of 2016-01-08 10:02:25 +1100: > > - It forces cxl developers to a higher standard. cxl has already had > > more than it's fair share of incredibly difficult to debug issues, > > so any way we can reduce the risk of errors going in makes our lives > > (and our end-users lives) better. > > One problem with this point: not all warnings are under the purview of > cxl developers. For instance, if I turn up warning verbosity (W=1), then > the *header* files start producing plenty of warnings. Should this break > the build? Your code didn't change, and you can't fix those errors. That's a good point, but the specific warnings that we suppressed in the new compiler are in drivers/misc/cxl/cxl.h, which is an internal header that should only ever be included by the cxl driver. We do have some headers elsewhere which are included by other drivers, the generic ppc architecture code and userspace, but these are all warning free and won't be affected by the -Werror when included from elsewhere. > That is a real use case for me daily: I turn the warning verbosity up on > my compile tests, then (smart)diff the build logs before and after > new patches. That way, I can see what new warnings (even potentially > false positive ones) are introduced. I can't do that if every random > developer wants to stick -Werror in their Makefile. Makes sense. > I think there are plenty of reasons to either remove -Werror, or make it > configurable. Some of them are detailed above. > > Maybe you can gate the -Werror on CONFIG_PPC_WERROR, just like the rest > of PowerPC? Agreed. @Daniel - since you added the -Werror do you want to do this, or shall I? Cheers, -Ian
Acked-by: Ian Munsie <imunsie@au1.ibm.com>
As suggested by Brian we might also gate the -Werror with
CONFIG_PPC_WERROR, but that can be in a separate commit.
On Fri, Jan 08, 2016 at 12:31:54PM +1100, Ian Munsie wrote: > Excerpts from Brian Norris's message of 2016-01-08 10:02:25 +1100: > > > - It forces cxl developers to a higher standard. cxl has already had > > > more than it's fair share of incredibly difficult to debug issues, > > > so any way we can reduce the risk of errors going in makes our lives > > > (and our end-users lives) better. > > > > One problem with this point: not all warnings are under the purview of > > cxl developers. For instance, if I turn up warning verbosity (W=1), then (BTW, I think most of these files are currently clean with W=1, but not W=2) > > the *header* files start producing plenty of warnings. Should this break > > the build? Your code didn't change, and you can't fix those errors. > > That's a good point, but the specific warnings that we suppressed in the > new compiler are in drivers/misc/cxl/cxl.h, which is an internal header > that should only ever be included by the cxl driver. We do have some > headers elsewhere which are included by other drivers, the generic ppc > architecture code and userspace, but these are all warning free and > won't be affected by the -Werror when included from elsewhere. I was referring to when extra warnings are turned on, not just the default. So this fails spectacularly: $ export ARCH=powerpc $ make ppc64_defconfig $ make drivers/misc/cxl/api.o W=2 CC [M] drivers/misc/cxl/api.o In file included from include/linux/bitops.h:36:0, from include/linux/kernel.h:10, from include/linux/list.h:8, from include/linux/pci.h:25, from drivers/misc/cxl/api.c:10: ./arch/powerpc/include/asm/bitops.h:226:94: error: declaration of 'ffs' shadows a built-in function [-Werror=shadow] In file included from include/linux/atomic.h:562:0, from include/linux/mutex.h:18, from include/linux/kernfs.h:13, from include/linux/sysfs.h:15, from include/linux/kobject.h:21, from include/linux/pci.h:28, from drivers/misc/cxl/api.c:10: include/asm-generic/atomic-long.h: In function 'atomic_long_read_acquire': include/asm-generic/atomic-long.h:45:231: error: nested extern declaration of '__compiletime_assert_45' [-Werror=nested-externs] In file included from include/linux/atomic.h:562:0, from include/linux/mutex.h:18, from include/linux/kernfs.h:13, from include/linux/sysfs.h:15, from include/linux/kobject.h:21, from include/linux/pci.h:28, from drivers/misc/cxl/api.c:10: include/asm-generic/atomic-long.h: In function 'atomic_long_set_release': include/asm-generic/atomic-long.h:57:1: error: nested extern declaration of '__compiletime_assert_57' [-Werror=nested-externs] In file included from include/linux/ktime.h:25:0, from include/linux/rcupdate.h:47, from include/linux/idr.h:18, from include/linux/kernfs.h:14, from include/linux/sysfs.h:15, from include/linux/kobject.h:21, from include/linux/pci.h:28, from drivers/misc/cxl/api.c:10: include/linux/jiffies.h: In function 'jiffies_to_timespec': include/linux/jiffies.h:422:131: error: declaration of 'jiffies' shadows a global declaration [-Werror=shadow] include/linux/jiffies.h:78:65: error: shadowed declaration is here [-Werror=shadow] In file included from include/linux/kernfs.h:16:0, from include/linux/sysfs.h:15, from include/linux/kobject.h:21, from include/linux/pci.h:28, from drivers/misc/cxl/api.c:10: [...many more failures...] cc1: all warnings being treated as errors make[1]: *** [drivers/misc/cxl/api.o] Error 1 make: *** [drivers/misc/cxl/api.o] Error 2 I doubt you plan to fix all those. So making -Werror configurable seems like the only way forward, then. (Glad you agreed!) Regards, Brian
On Thu, 2016-01-07 at 18:07 -0800, Brian Norris wrote: > On Fri, Jan 08, 2016 at 12:31:54PM +1100, Ian Munsie wrote: > > Excerpts from Brian Norris's message of 2016-01-08 10:02:25 +1100: > > > > - It forces cxl developers to a higher standard. cxl has already had > > > > more than it's fair share of incredibly difficult to debug issues, > > > > so any way we can reduce the risk of errors going in makes our lives > > > > (and our end-users lives) better. > > > > > > One problem with this point: not all warnings are under the purview of > > > cxl developers. For instance, if I turn up warning verbosity (W=1), then > > (BTW, I think most of these files are currently clean with W=1, but not W=2) > > > the *header* files start producing plenty of warnings. Should this break > > > the build? Your code didn't change, and you can't fix those errors. > > > > That's a good point, but the specific warnings that we suppressed in the > > new compiler are in drivers/misc/cxl/cxl.h, which is an internal header > > that should only ever be included by the cxl driver. We do have some > > headers elsewhere which are included by other drivers, the generic ppc > > architecture code and userspace, but these are all warning free and > > won't be affected by the -Werror when included from elsewhere. > > I was referring to when extra warnings are turned on, not just the > default. So this fails spectacularly: > > $ export ARCH=powerpc > $ make ppc64_defconfig > $ make drivers/misc/cxl/api.o W=2 > CC [M] drivers/misc/cxl/api.o > In file included from include/linux/bitops.h:36:0, > from include/linux/kernel.h:10, > from include/linux/list.h:8, > from include/linux/pci.h:25, > from drivers/misc/cxl/api.c:10: > ./arch/powerpc/include/asm/bitops.h:226:94: error: declaration of 'ffs' shadows a built-in function [-Werror=shadow] > > I doubt you plan to fix all those. So making -Werror configurable seems like > the only way forward, then. (Glad you agreed!) Yep, I'm happy to make Werror configurable. cxl can probably just use the existing PPC_WERROR. cheers
From: Michael Ellerman > Sent: 08 January 2016 02:17 > > I doubt you plan to fix all those. So making -Werror configurable seems like > > the only way forward, then. (Glad you agreed!) > > Yep, I'm happy to make Werror configurable. > > cxl can probably just use the existing PPC_WERROR. Personally I think the development builds should be done with both error -Werror and higher levels of warnings, even if the ones that users tend to do ignore more warnings. Clearly some warnings that some versions of gcc have included in -Wall are just a PITA (like warning for __DATE__ and __TIME__). The kernel headers do need fixing so that they pass compilation with options like -Wwrite-strings (linux/mm.h doesn't). David
diff --git a/drivers/misc/cxl/Makefile b/drivers/misc/cxl/Makefile index 6982f603fadc..add2cc17ed91 100644 --- a/drivers/misc/cxl/Makefile +++ b/drivers/misc/cxl/Makefile @@ -1,4 +1,4 @@ -ccflags-y := -Werror -Wno-unused-const-variable +ccflags-y := -Werror $(call cc-disable-warning,unused-const-variable) cxl-y += main.o file.o irq.o fault.o native.o cxl-y += context.o sysfs.o debugfs.o pci.o trace.o