| Message ID | 20201128193335.219395-1-masahiroy@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK | expand |
On Sun, Nov 29, 2020 at 04:33:35AM +0900, Masahiro Yamada wrote: > Revert commit cebc04ba9aeb ("add CONFIG_ENABLE_MUST_CHECK"). > > A lot of warn_unused_result warnings existed in 2006, but until now > they have been fixed thanks to people doing allmodconfig tests. > > Our goal is to always enable __must_check where appropriate, so this > CONFIG option is no longer needed. > > I see a lot of defconfig (arch/*/configs/*_defconfig) files having: > > # CONFIG_ENABLE_MUST_CHECK is not set > > I did not touch them for now since it would be a big churn. If arch > maintainers want to clean them up, please go ahead. > > While I was here, I also moved __must_check to compiler_attributes.h > from compiler_types.h > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> > Acked-by: Jason A. Donenfeld <Jason@zx2c4.com> Acked-by: Nathan Chancellor <natechancellor@gmail.com>
On Sat, Nov 28, 2020 at 11:34 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > Revert commit cebc04ba9aeb ("add CONFIG_ENABLE_MUST_CHECK"). > > A lot of warn_unused_result warnings existed in 2006, but until now > they have been fixed thanks to people doing allmodconfig tests. > > Our goal is to always enable __must_check where appropriate, so this > CONFIG option is no longer needed. > > I see a lot of defconfig (arch/*/configs/*_defconfig) files having: > > # CONFIG_ENABLE_MUST_CHECK is not set > > I did not touch them for now since it would be a big churn. If arch > maintainers want to clean them up, please go ahead. > > While I was here, I also moved __must_check to compiler_attributes.h > from compiler_types.h > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> > Acked-by: Jason A. Donenfeld <Jason@zx2c4.com> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > --- > > Changes in v3: > - Fix a typo > > Changes in v2: > - Move __must_check to compiler_attributes.h > > include/linux/compiler_attributes.h | 7 +++++++ > include/linux/compiler_types.h | 6 ------ > lib/Kconfig.debug | 8 -------- > tools/testing/selftests/wireguard/qemu/debug.config | 1 - > 4 files changed, 7 insertions(+), 15 deletions(-) > > diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h > index b2a3f4f641a7..5f3b7edad1a7 100644 > --- a/include/linux/compiler_attributes.h > +++ b/include/linux/compiler_attributes.h > @@ -171,6 +171,13 @@ > */ > #define __mode(x) __attribute__((__mode__(x))) > > +/* > + * gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-warn_005funused_005fresult-function-attribute > + * clang: https://clang.llvm.org/docs/AttributeReference.html#nodiscard-warn-unused-result > + * > + */ > +#define __must_check __attribute__((__warn_unused_result__)) > + > /* > * Optional: only supported since gcc >= 7 > * > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h > index ac3fa37a84f9..7ef20d1a6c28 100644 > --- a/include/linux/compiler_types.h > +++ b/include/linux/compiler_types.h > @@ -110,12 +110,6 @@ struct ftrace_likely_data { > unsigned long constant; > }; > > -#ifdef CONFIG_ENABLE_MUST_CHECK > -#define __must_check __attribute__((__warn_unused_result__)) > -#else > -#define __must_check > -#endif > - > #if defined(CC_USING_HOTPATCH) > #define notrace __attribute__((hotpatch(0, 0))) > #elif defined(CC_USING_PATCHABLE_FUNCTION_ENTRY) > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index c789b39ed527..cb8ef4fd0d02 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -286,14 +286,6 @@ config GDB_SCRIPTS > > endif # DEBUG_INFO > > -config ENABLE_MUST_CHECK > - bool "Enable __must_check logic" > - default y > - help > - Enable the __must_check logic in the kernel build. Disable this to > - suppress the "warning: ignoring return value of 'foo', declared with > - attribute warn_unused_result" messages. > - > config FRAME_WARN > int "Warn for stack frames larger than" > range 0 8192 > diff --git a/tools/testing/selftests/wireguard/qemu/debug.config b/tools/testing/selftests/wireguard/qemu/debug.config > index b50c2085c1ac..fe07d97df9fa 100644 > --- a/tools/testing/selftests/wireguard/qemu/debug.config > +++ b/tools/testing/selftests/wireguard/qemu/debug.config > @@ -1,5 +1,4 @@ > CONFIG_LOCALVERSION="-debug" > -CONFIG_ENABLE_MUST_CHECK=y > CONFIG_FRAME_POINTER=y > CONFIG_STACK_VALIDATION=y > CONFIG_DEBUG_KERNEL=y > -- > 2.27.0 >
On Sat, Nov 28, 2020 at 8:34 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > > Revert commit cebc04ba9aeb ("add CONFIG_ENABLE_MUST_CHECK"). > > A lot of warn_unused_result warnings existed in 2006, but until now > they have been fixed thanks to people doing allmodconfig tests. > > Our goal is to always enable __must_check where appropriate, so this > CONFIG option is no longer needed. > > I see a lot of defconfig (arch/*/configs/*_defconfig) files having: > > # CONFIG_ENABLE_MUST_CHECK is not set > > I did not touch them for now since it would be a big churn. If arch > maintainers want to clean them up, please go ahead. > > While I was here, I also moved __must_check to compiler_attributes.h > from compiler_types.h > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> > Acked-by: Jason A. Donenfeld <Jason@zx2c4.com> Picked this new version with the Acks etc., plus I moved it within compiler_attributes.h to keep it sorted (it's sorted by the second column, rather than the first). Thanks a lot! Cheers, Miguel
On Sat, Dec 12, 2020 at 5:18 PM Guenter Roeck <linux@roeck-us.net> wrote: > > This patch results in: > > arch/sh/kernel/cpu/sh4a/smp-shx3.c: In function 'shx3_prepare_cpus': > arch/sh/kernel/cpu/sh4a/smp-shx3.c:76:3: error: ignoring return value of 'request_irq' declared with attribute 'warn_unused_result' > > when building sh:defconfig. Checking for calls to request_irq() > suggests that there will be other similar errors in various builds. > Reverting the patch fixes the problem. Which ones? From a quick grep and some filtering I could only find one file with wrong usage apart from this one: drivers/net/ethernet/lantiq_etop.c: request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv); drivers/net/ethernet/lantiq_etop.c: request_irq(irq, ltq_etop_dma_irq, 0, "etop_rx", priv); Of course, this does not cover other functions, but it means there aren't many issues and/or people building the code if nobody complains within a few weeks. So I think we can fix them as they come. Cheers, Miguel
On 12/12/20 9:04 PM, Miguel Ojeda wrote: > On Sat, Dec 12, 2020 at 5:18 PM Guenter Roeck <linux@roeck-us.net> wrote: >> >> This patch results in: >> >> arch/sh/kernel/cpu/sh4a/smp-shx3.c: In function 'shx3_prepare_cpus': >> arch/sh/kernel/cpu/sh4a/smp-shx3.c:76:3: error: ignoring return value of 'request_irq' declared with attribute 'warn_unused_result' >> >> when building sh:defconfig. Checking for calls to request_irq() >> suggests that there will be other similar errors in various builds. >> Reverting the patch fixes the problem. > > Which ones? From a quick grep and some filtering I could only find one > file with wrong usage apart from this one: > > drivers/net/ethernet/lantiq_etop.c: > request_irq(irq, ltq_etop_dma_irq, 0, "etop_tx", priv); > drivers/net/ethernet/lantiq_etop.c: > request_irq(irq, ltq_etop_dma_irq, 0, "etop_rx", priv); > > Of course, this does not cover other functions, but it means there > aren't many issues and/or people building the code if nobody complains > within a few weeks. So I think we can fix them as they come. > Witz komm raus, Du bist umzingelt. The key here is "if nobody complains". I would argue that it is _your_ responsibility to do those builds, and not the reponsibility of others to do it for you. But, sure, your call. Please feel free to ignore my report. Guenter
On Sun, Dec 13, 2020 at 1:55 PM Guenter Roeck <linux@roeck-us.net> wrote: > > Witz komm raus, Du bist umzingelt. Please, explain this reference. :-) > The key here is "if nobody complains". I would argue that it is _your_ > responsibility to do those builds, and not the reponsibility of others > to do it for you. Testing allmodconfig for a popular architecture, agreed, it is due diligence to avoid messing -next that day. Testing a matrix of configs * arches * gcc/clang * compiler versions? No, sorry, that is what CI/-next/-rcs are for and that is where the "if nobody complains" comes from. If you think building a set of code for a given arch/config/etc. is particularly important, then it is _your_ responsibility to build it once in a while in -next (as you have done). If it is not that important, somebody will speak up in one -rc. If not, is anyone actually building that code at all? Otherwise, changing core/shared code would be impossible. Please don't blame the author for making a sensible change that will improve code quality for everyone. > But, sure, your call. Please feel free to ignore my report. I'm not ignoring the report, quite the opposite. I am trying to understand why you think reverting is needed for something that has been more than a week in -next without any major breakage and still has a long road to v5.11. Cheers, Miguel
On Sun, Dec 13, 2020 at 03:58:20PM +0100, Miguel Ojeda wrote: > > The key here is "if nobody complains". I would argue that it is _your_ > > responsibility to do those builds, and not the reponsibility of others > > to do it for you. > > Testing allmodconfig for a popular architecture, agreed, it is due > diligence to avoid messing -next that day. > > Testing a matrix of configs * arches * gcc/clang * compiler versions? > No, sorry, that is what CI/-next/-rcs are for and that is where the > "if nobody complains" comes from. > > If you think building a set of code for a given arch/config/etc. is > particularly important, then it is _your_ responsibility to build it > once in a while in -next (as you have done). If it is not that > important, somebody will speak up in one -rc. If not, is anyone > actually building that code at all? > > Otherwise, changing core/shared code would be impossible. Please don't > blame the author for making a sensible change that will improve code > quality for everyone. > > > But, sure, your call. Please feel free to ignore my report. > > I'm not ignoring the report, quite the opposite. I am trying to > understand why you think reverting is needed for something that has > been more than a week in -next without any major breakage and still > has a long road to v5.11. Because if you get a report of something breaking for your change, you need to work to resolve it, not argue about it. Otherwise it needs to be dropped/reverted. Please fix. thanks, greg k-h
On Sun, Dec 13, 2020 at 4:16 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > Because if you get a report of something breaking for your change, you > need to work to resolve it, not argue about it. Otherwise it needs to > be dropped/reverted. Nobody has argued that. In fact, I explicitly said the opposite: "So I think we can fix them as they come.". I am expecting Masahiro to follow up. It has been less than 24 hours since the report, on a weekend. Cheers, Miguel
Miguel Ojeda wrote:
> I think we can fix them as they come.
If your change to a function breaks its callers, it's your job to fix
the callers proactively instead of waiting for "as they come" bug
reports. (Assuming, of course, that you know about the breakage. Which
you do when you tell us that the bad pattern can simply be grepped for.)
If nothing else, that's far more efficient than [number_of_callers]
separate patches by other people who each need to find the offending
change, figure out what to change and/or who to report the problem to,
and so on until the fix lands in the kernel.
Moreover, this wouldn't leave the kernel sources in a non-bisect-able
state during that time.
On Sun, Dec 13, 2020 at 4:38 PM 'Matthias Urlichs' via Clang Built Linux <clang-built-linux@googlegroups.com> wrote: > > If your change to a function breaks its callers, it's your job to fix No function has changed. This patch enables a warning (that for some reason is an error in the case of Guenter). Even if this was a hard error, the same applies: the function hasn't changed. It just means callers never tested with `CONFIG_ENABLE_MUST_CHECK` for *years*. > the callers proactively instead of waiting for "as they come" bug > reports. (Assuming, of course, that you know about the breakage. Which > you do when you tell us that the bad pattern can simply be grepped for.) No, *we don't know about the breakage*. The grep was for the particular function Guenter reported, and done to validate his concern. If you want to manually inspect every caller of every `__must_check` function, or to write a cocci patch or a clang-tidy check or similar (that would be obsolete as soon as `__must_check` is enabled), you are welcome to do so. But a much better usage of our time would be letting machines do their job. > If nothing else, that's far more efficient than [number_of_callers] > separate patches by other people who each need to find the offending > change, figure out what to change and/or who to report the problem to, > and so on until the fix lands in the kernel. This change is not in Linus' tree, it is on -next. > Moreover, this wouldn't leave the kernel sources in a non-bisect-able > state during that time. Again, the change is in -next. That is the point: to do integration testing and let the bots run against it. Cheers, Miguel
On Mon, Dec 14, 2020 at 12:27 AM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Sun, Dec 13, 2020 at 4:16 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > Because if you get a report of something breaking for your change, you > > need to work to resolve it, not argue about it. Otherwise it needs to > > be dropped/reverted. > > Nobody has argued that. In fact, I explicitly said the opposite: "So I > think we can fix them as they come.". > > I am expecting Masahiro to follow up. It has been less than 24 hours > since the report, on a weekend. > > Cheers, > Miguel Sorry for the delay. Now I sent out the fix for lantiq_etop.c https://lore.kernel.org/patchwork/patch/1355595/ The reason of the complication was I was trying to merge the following patch in the same development cycle: https://patchwork.kernel.org/project/linux-kbuild/patch/20201117104736.24997-1-olaf@aepfle.de/ -Werror=return-type gives a bigger impact because any instance of __must_check violation results in build breakage. So, I just dropped it from my tree (and, I will aim for 5.12). The removal of CONFIG_ENABLE_MUST_CHECK is less impactive, because we are still able to build with some warnings. Tomorrow's linux-next should be OK and, you can send my patch in this merge window.
On 12/20/20 10:18 PM, Masahiro Yamada wrote: > On Mon, Dec 14, 2020 at 12:27 AM Miguel Ojeda > <miguel.ojeda.sandonis@gmail.com> wrote: >> >> On Sun, Dec 13, 2020 at 4:16 PM Greg KH <gregkh@linuxfoundation.org> wrote: >>> >>> Because if you get a report of something breaking for your change, you >>> need to work to resolve it, not argue about it. Otherwise it needs to >>> be dropped/reverted. >> >> Nobody has argued that. In fact, I explicitly said the opposite: "So I >> think we can fix them as they come.". >> >> I am expecting Masahiro to follow up. It has been less than 24 hours >> since the report, on a weekend. >> >> Cheers, >> Miguel > > > Sorry for the delay. > > Now I sent out the fix for lantiq_etop.c > > https://lore.kernel.org/patchwork/patch/1355595/ > next-20201218, alpha:allmodconfig: fs/binfmt_em86.c: In function 'load_em86': fs/binfmt_em86.c:66:2: error: ignoring return value of 'remove_arg_zero' declared with attribute 'warn_unused_result' With a change like this, I'd have expected that there is a coccinelle script or similar to ensure that claims made in the commit message are true. Guenter
On Mon, Dec 21, 2020 at 7:20 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > Sorry for the delay. No problem! > Now I sent out the fix for lantiq_etop.c > > https://lore.kernel.org/patchwork/patch/1355595/ I saw it, thanks for the Cc! > The reason of the complication was > I was trying to merge the following patch in the same development cycle: > https://patchwork.kernel.org/project/linux-kbuild/patch/20201117104736.24997-1-olaf@aepfle.de/ Ah, then that explains why Guenter's had an error instead of a warning. > Tomorrow's linux-next should be OK > and, you can send my patch in this merge window. Thanks! Cheers, Miguel
On Mon, Dec 21, 2020 at 11:02 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On 12/20/20 10:18 PM, Masahiro Yamada wrote: > With a change like this, I'd have expected that there is a coccinelle > script or similar to ensure that claims made in the commit message > are true. It is only a warning -- the compiler already tells us what is wrong. Cheers, Miguel
diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h index b2a3f4f641a7..5f3b7edad1a7 100644 --- a/include/linux/compiler_attributes.h +++ b/include/linux/compiler_attributes.h @@ -171,6 +171,13 @@ */ #define __mode(x) __attribute__((__mode__(x))) +/* + * gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-warn_005funused_005fresult-function-attribute + * clang: https://clang.llvm.org/docs/AttributeReference.html#nodiscard-warn-unused-result + * + */ +#define __must_check __attribute__((__warn_unused_result__)) + /* * Optional: only supported since gcc >= 7 * diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h index ac3fa37a84f9..7ef20d1a6c28 100644 --- a/include/linux/compiler_types.h +++ b/include/linux/compiler_types.h @@ -110,12 +110,6 @@ struct ftrace_likely_data { unsigned long constant; }; -#ifdef CONFIG_ENABLE_MUST_CHECK -#define __must_check __attribute__((__warn_unused_result__)) -#else -#define __must_check -#endif - #if defined(CC_USING_HOTPATCH) #define notrace __attribute__((hotpatch(0, 0))) #elif defined(CC_USING_PATCHABLE_FUNCTION_ENTRY) diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index c789b39ed527..cb8ef4fd0d02 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -286,14 +286,6 @@ config GDB_SCRIPTS endif # DEBUG_INFO -config ENABLE_MUST_CHECK - bool "Enable __must_check logic" - default y - help - Enable the __must_check logic in the kernel build. Disable this to - suppress the "warning: ignoring return value of 'foo', declared with - attribute warn_unused_result" messages. - config FRAME_WARN int "Warn for stack frames larger than" range 0 8192 diff --git a/tools/testing/selftests/wireguard/qemu/debug.config b/tools/testing/selftests/wireguard/qemu/debug.config index b50c2085c1ac..fe07d97df9fa 100644 --- a/tools/testing/selftests/wireguard/qemu/debug.config +++ b/tools/testing/selftests/wireguard/qemu/debug.config @@ -1,5 +1,4 @@ CONFIG_LOCALVERSION="-debug" -CONFIG_ENABLE_MUST_CHECK=y CONFIG_FRAME_POINTER=y CONFIG_STACK_VALIDATION=y CONFIG_DEBUG_KERNEL=y
Revert commit cebc04ba9aeb ("add CONFIG_ENABLE_MUST_CHECK"). A lot of warn_unused_result warnings existed in 2006, but until now they have been fixed thanks to people doing allmodconfig tests. Our goal is to always enable __must_check where appropriate, so this CONFIG option is no longer needed. I see a lot of defconfig (arch/*/configs/*_defconfig) files having: # CONFIG_ENABLE_MUST_CHECK is not set I did not touch them for now since it would be a big churn. If arch maintainers want to clean them up, please go ahead. While I was here, I also moved __must_check to compiler_attributes.h from compiler_types.h Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> Acked-by: Jason A. Donenfeld <Jason@zx2c4.com> --- Changes in v3: - Fix a typo Changes in v2: - Move __must_check to compiler_attributes.h include/linux/compiler_attributes.h | 7 +++++++ include/linux/compiler_types.h | 6 ------ lib/Kconfig.debug | 8 -------- tools/testing/selftests/wireguard/qemu/debug.config | 1 - 4 files changed, 7 insertions(+), 15 deletions(-)