From: Stephen Rothwell <sfr@canb.auug.org.au>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@elte.hu>, "H. Peter Anvin" <hpa@zytor.com>,
Peter Zijlstra <peterz@infradead.org>,
Linux Next Mailing List <linux-next@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Al Viro <viro@zeniv.linux.org.uk>,
Russell King <rmk+kernel@arm.linux.org.uk>,
Arnd Bergmann <arnd@arndb.de>,
linux-arm-kernel@lists.infradead.org
Subject: Re: ARM: futex: Address build warning
Date: Thu, 7 May 2020 08:19:23 +1000 [thread overview]
Message-ID: <20200507081924.7c77bfc9@canb.auug.org.au> (raw)
In-Reply-To: <87pncao2ph.fsf@nanos.tec.linutronix.de>
[-- Attachment #1: Type: text/plain, Size: 2719 bytes --]
Hi all,
On Tue, 14 Apr 2020 11:07:22 +0200 Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Stephen reported the following build warning on a ARM multi_v7_defconfig
> build with GCC 9.2.1:
>
> kernel/futex.c: In function 'do_futex':
> kernel/futex.c:1676:17: warning: 'oldval' may be used uninitialized in this function [-Wmaybe-uninitialized]
> 1676 | return oldval == cmparg;
> | ~~~~~~~^~~~~~~~~
> kernel/futex.c:1652:6: note: 'oldval' was declared here
> 1652 | int oldval, ret;
> | ^~~~~~
>
> introduced by commit a08971e9488d ("futex: arch_futex_atomic_op_inuser()
> calling conventions change").
>
> While that change should not make any difference it confuses GCC which
> fails to work out that oldval is not referenced when the return value is
> not zero.
>
> GCC fails to properly analyze arch_futex_atomic_op_inuser(). It's not the
> early return, the issue is with the assembly macros. GCC fails to detect
> that those either set 'ret' to 0 and set oldval or set 'ret' to -EFAULT
> which makes oldval uninteresting. The store to the callsite supplied oldval
> pointer is conditional on ret == 0.
>
> The straight forward way to solve this is to make the store unconditional.
>
> Aside of addressing the build warning this makes sense anyway because it
> removes the conditional from the fastpath. In the error case the stored
> value is uninteresting and the extra store does not matter at all.
>
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Link: https://lore.kernel.org/r/874ku2q18k.fsf@nanos.tec.linutronix.de
> ---
> arch/arm/include/asm/futex.h | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> --- a/arch/arm/include/asm/futex.h
> +++ b/arch/arm/include/asm/futex.h
> @@ -165,8 +165,13 @@ arch_futex_atomic_op_inuser(int op, int
> preempt_enable();
> #endif
>
> - if (!ret)
> - *oval = oldval;
> + /*
> + * Store unconditionally. If ret != 0 the extra store is the least
> + * of the worries but GCC cannot figure out that __futex_atomic_op()
> + * is either setting ret to -EFAULT or storing the old value in
> + * oldval which results in a uninitialized warning at the call site.
> + */
> + *oval = oldval;
>
> return ret;
> }
Any response to this? I am still getting the warning ...
The warning was introduced by commit
a08971e9488d ("futex: arch_futex_atomic_op_inuser() calling conventions change")
Which has been in Linus' tree since before v5.7-rc1. Should this go in
via the tip tree, the arm tree, or will I just send ti to Linus myself?
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
prev parent reply other threads:[~2020-05-06 22:19 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-30 2:47 linux-next: build warning after merge of the tip tree Stephen Rothwell
2020-03-31 21:57 ` Stephen Rothwell
2020-04-01 10:25 ` Thomas Gleixner
2020-04-01 22:00 ` Stephen Rothwell
2020-04-01 22:39 ` Thomas Gleixner
2020-04-13 0:01 ` Stephen Rothwell
2020-04-14 8:42 ` Thomas Gleixner
2020-04-14 9:07 ` ARM: futex: Address build warning Thomas Gleixner
2020-05-06 22:19 ` Stephen Rothwell [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200507081924.7c77bfc9@canb.auug.org.au \
--to=sfr@canb.auug.org.au \
--cc=arnd@arndb.de \
--cc=hpa@zytor.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-next@vger.kernel.org \
--cc=mingo@elte.hu \
--cc=peterz@infradead.org \
--cc=rmk+kernel@arm.linux.org.uk \
--cc=tglx@linutronix.de \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).