Linux-Next Archive on lore.kernel.org
 help / color / Atom feed
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
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 --]

      reply index

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

Linux-Next Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-next/0 linux-next/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-next linux-next/ https://lore.kernel.org/linux-next \
		linux-next@vger.kernel.org
	public-inbox-index linux-next

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-next


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git