Re: [PATCH v3] Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK

Re: [PATCH v3] Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK

[Nick Desaulniers]

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
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v3] Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK

[Miguel Ojeda]

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

Re: [PATCH v3] Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK

[Miguel Ojeda]

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

Re: [PATCH v3] Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK

[Guenter Roeck]

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

Re: [PATCH v3] Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK

[Miguel Ojeda]

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

Re: [PATCH v3] Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK

[Greg KH]

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

Re: [PATCH v3] Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK

[Miguel Ojeda]

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

Re: [PATCH v3] Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK

[Matthias Urlichs]

[-- Attachment #1.1: Type: text/plain, Size: 744 bytes --]

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.

-- 
-- Matthias Urlichs



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v3] Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK

[Miguel Ojeda]

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

Re: [PATCH v3] Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK

[Masahiro Yamada]

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.

-- 
Best Regards
Masahiro Yamada

Re: [PATCH v3] Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK

[Guenter Roeck]

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

Re: [PATCH v3] Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK

[Miguel Ojeda]

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

Re: [PATCH v3] Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK

[Miguel Ojeda]

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

Re: [PATCH v3] Compiler Attributes: remove CONFIG_ENABLE_MUST_CHECK

[Nathan Chancellor]

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>