Linux-Next Archive on lore.kernel.org
 help / color / Atom feed
* linux-next: build warning after merge of the tip tree
@ 2020-03-30  2:47 Stephen Rothwell
  2020-03-31 21:57 ` Stephen Rothwell
  0 siblings, 1 reply; 9+ messages in thread
From: Stephen Rothwell @ 2020-03-30  2:47 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Peter Zijlstra
  Cc: Linux Next Mailing List, Linux Kernel Mailing List, Al Viro


[-- Attachment #1: Type: text/plain, Size: 639 bytes --]

Hi all,

After merging the tip tree, today's linux-next build (arm
multi_v7_defconfig) produced this warning:

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")

but I don't see how it makes this difference :-(

-- 
Cheers,
Stephen Rothwell

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: linux-next: build warning after merge of the tip tree
  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
  0 siblings, 1 reply; 9+ messages in thread
From: Stephen Rothwell @ 2020-03-31 21:57 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Peter Zijlstra
  Cc: Linux Next Mailing List, Linux Kernel Mailing List, Al Viro


[-- Attachment #1: Type: text/plain, Size: 828 bytes --]

Hi all,

On Mon, 30 Mar 2020 13:47:46 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> Hi all,
> 
> After merging the tip tree, today's linux-next build (arm
> multi_v7_defconfig) produced this warning:
> 
> 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")
> 
> but I don't see how it makes this difference :-(

I now get this warning from building Linus' tree :-(

-- 
Cheers,
Stephen Rothwell

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: linux-next: build warning after merge of the tip tree
  2020-03-31 21:57 ` Stephen Rothwell
@ 2020-04-01 10:25   ` Thomas Gleixner
  2020-04-01 22:00     ` Stephen Rothwell
  0 siblings, 1 reply; 9+ messages in thread
From: Thomas Gleixner @ 2020-04-01 10:25 UTC (permalink / raw)
  To: Stephen Rothwell, Ingo Molnar, H. Peter Anvin, Peter Zijlstra
  Cc: Linux Next Mailing List, Linux Kernel Mailing List, Al Viro

Stephen Rothwell <sfr@canb.auug.org.au> writes:
> Hi all,
>
> On Mon, 30 Mar 2020 13:47:46 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>>
>> Hi all,
>> 
>> After merging the tip tree, today's linux-next build (arm
>> multi_v7_defconfig) produced this warning:
>> 
>> 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")

Huch?
 
>> but I don't see how it makes this difference :-(

Me neither. Which compiler version?

I'm using arm-linux-gnueabi-gcc (Debian 8.3.0-2) 8.3.0 which does not
show that oddity.

Thanks,

        tglx


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: linux-next: build warning after merge of the tip tree
  2020-04-01 10:25   ` Thomas Gleixner
@ 2020-04-01 22:00     ` Stephen Rothwell
  2020-04-01 22:39       ` Thomas Gleixner
  0 siblings, 1 reply; 9+ messages in thread
From: Stephen Rothwell @ 2020-04-01 22:00 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Ingo Molnar, H. Peter Anvin, Peter Zijlstra,
	Linux Next Mailing List, Linux Kernel Mailing List, Al Viro


[-- Attachment #1: Type: text/plain, Size: 1472 bytes --]

Hi Thomas,

On Wed, 01 Apr 2020 12:25:25 +0200 Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Stephen Rothwell <sfr@canb.auug.org.au> writes:
> >
> > On Mon, 30 Mar 2020 13:47:46 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:  
> >>
> >> After merging the tip tree, today's linux-next build (arm
> >> multi_v7_defconfig) produced this warning:
> >> 
> >> 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")  
> 
> Huch?
>  
> >> but I don't arm-linux-gnueabi-gcc (Debian 9.2.1-21) 9.2.1 20191130see how it makes this difference :-(  
> 
> Me neither. Which compiler version?

arm-linux-gnueabi-gcc (Debian 9.2.1-21) 9.2.1 20191130

> I'm using arm-linux-gnueabi-gcc (Debian 8.3.0-2) 8.3.0 which does not
> show that oddity.

I assume it is because of the change to arch_futex_atomic_op_inuser()
for arm and the compiler is not clever enough to work out that the early
return from arch_futex_atomic_op_inuser() means that oldval is not
referenced in its caller.

-- 
Cheers,
Stephen Rothwell

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: linux-next: build warning after merge of the tip tree
  2020-04-01 22:00     ` Stephen Rothwell
@ 2020-04-01 22:39       ` Thomas Gleixner
  2020-04-13  0:01         ` Stephen Rothwell
  0 siblings, 1 reply; 9+ messages in thread
From: Thomas Gleixner @ 2020-04-01 22:39 UTC (permalink / raw)
  To: Stephen Rothwell
  Cc: Ingo Molnar, H. Peter Anvin, Peter Zijlstra,
	Linux Next Mailing List, Linux Kernel Mailing List, Al Viro

Stephen,

Stephen Rothwell <sfr@canb.auug.org.au> writes:
> On Wed, 01 Apr 2020 12:25:25 +0200 Thomas Gleixner <tglx@linutronix.de> wrote:
>> Me neither. Which compiler version?
>
> arm-linux-gnueabi-gcc (Debian 9.2.1-21) 9.2.1 20191130
>
>> I'm using arm-linux-gnueabi-gcc (Debian 8.3.0-2) 8.3.0 which does not
>> show that oddity.
>
> I assume it is because of the change to arch_futex_atomic_op_inuser()
> for arm and the compiler is not clever enough to work out that the early
> return from arch_futex_atomic_op_inuser() means that oldval is not
> referenced in its caller.

Actually no. It's the ASM part which causes this. With the following
hack applied it compiles:

diff --git a/arch/arm/include/asm/futex.h b/arch/arm/include/asm/futex.h
index e133da303a98..2c6b40f71009 100644
--- a/arch/arm/include/asm/futex.h
+++ b/arch/arm/include/asm/futex.h
@@ -132,7 +132,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
 static inline int
 arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
 {
-	int oldval = 0, ret, tmp;
+	int oldval = 0, ret;
 
 	if (!access_ok(uaddr, sizeof(u32)))
 		return -EFAULT;
@@ -142,6 +142,7 @@ arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
 #endif
 
 	switch (op) {
+#if 0
 	case FUTEX_OP_SET:
 		__futex_atomic_op("mov	%0, %4", ret, oldval, tmp, uaddr, oparg);
 		break;
@@ -157,6 +158,7 @@ arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
 	case FUTEX_OP_XOR:
 		__futex_atomic_op("eor	%0, %1, %4", ret, oldval, tmp, uaddr, oparg);
 		break;
+#endif
 	default:
 		ret = -ENOSYS;
 	}

but with this is emits the warning:

diff --git a/arch/arm/include/asm/futex.h b/arch/arm/include/asm/futex.h
index e133da303a98..5191d7b61b83 100644
--- a/arch/arm/include/asm/futex.h
+++ b/arch/arm/include/asm/futex.h
@@ -145,6 +145,7 @@ arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
 	case FUTEX_OP_SET:
 		__futex_atomic_op("mov	%0, %4", ret, oldval, tmp, uaddr, oparg);
 		break;
+#if 0
 	case FUTEX_OP_ADD:
 		__futex_atomic_op("add	%0, %1, %4", ret, oldval, tmp, uaddr, oparg);
 		break;
@@ -157,6 +158,7 @@ arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
 	case FUTEX_OP_XOR:
 		__futex_atomic_op("eor	%0, %1, %4", ret, oldval, tmp, uaddr, oparg);
 		break;
+#endif
 	default:
 		ret = -ENOSYS;
 	}

and the below proves it:

diff --git a/arch/arm/include/asm/futex.h b/arch/arm/include/asm/futex.h
index e133da303a98..a9151884bc85 100644
--- 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 oparg, int *oval, u32 __user *uaddr)
 	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;
 }

I think that's the right thing to do anyway. The conditional is pointless.

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: linux-next: build warning after merge of the tip tree
  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
  0 siblings, 2 replies; 9+ messages in thread
From: Stephen Rothwell @ 2020-04-13  0:01 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Ingo Molnar, H. Peter Anvin, Peter Zijlstra,
	Linux Next Mailing List, Linux Kernel Mailing List, Al Viro


[-- Attachment #1: Type: text/plain, Size: 3663 bytes --]

Hi Thomas,

On Thu, 02 Apr 2020 00:39:55 +0200 Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Stephen Rothwell <sfr@canb.auug.org.au> writes:
> > On Wed, 01 Apr 2020 12:25:25 +0200 Thomas Gleixner <tglx@linutronix.de> wrote:  
> >> Me neither. Which compiler version?  
> >
> > arm-linux-gnueabi-gcc (Debian 9.2.1-21) 9.2.1 20191130
> >  
> >> I'm using arm-linux-gnueabi-gcc (Debian 8.3.0-2) 8.3.0 which does not
> >> show that oddity.  
> >
> > I assume it is because of the change to arch_futex_atomic_op_inuser()
> > for arm and the compiler is not clever enough to work out that the early
> > return from arch_futex_atomic_op_inuser() means that oldval is not
> > referenced in its caller.  
> 
> Actually no. It's the ASM part which causes this. With the following
> hack applied it compiles:
> 
> diff --git a/arch/arm/include/asm/futex.h b/arch/arm/include/asm/futex.h
> index e133da303a98..2c6b40f71009 100644
> --- a/arch/arm/include/asm/futex.h
> +++ b/arch/arm/include/asm/futex.h
> @@ -132,7 +132,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
>  static inline int
>  arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
>  {
> -	int oldval = 0, ret, tmp;
> +	int oldval = 0, ret;
>  
>  	if (!access_ok(uaddr, sizeof(u32)))
>  		return -EFAULT;
> @@ -142,6 +142,7 @@ arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
>  #endif
>  
>  	switch (op) {
> +#if 0
>  	case FUTEX_OP_SET:
>  		__futex_atomic_op("mov	%0, %4", ret, oldval, tmp, uaddr, oparg);
>  		break;
> @@ -157,6 +158,7 @@ arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
>  	case FUTEX_OP_XOR:
>  		__futex_atomic_op("eor	%0, %1, %4", ret, oldval, tmp, uaddr, oparg);
>  		break;
> +#endif
>  	default:
>  		ret = -ENOSYS;
>  	}
> 
> but with this is emits the warning:
> 
> diff --git a/arch/arm/include/asm/futex.h b/arch/arm/include/asm/futex.h
> index e133da303a98..5191d7b61b83 100644
> --- a/arch/arm/include/asm/futex.h
> +++ b/arch/arm/include/asm/futex.h
> @@ -145,6 +145,7 @@ arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
>  	case FUTEX_OP_SET:
>  		__futex_atomic_op("mov	%0, %4", ret, oldval, tmp, uaddr, oparg);
>  		break;
> +#if 0
>  	case FUTEX_OP_ADD:
>  		__futex_atomic_op("add	%0, %1, %4", ret, oldval, tmp, uaddr, oparg);
>  		break;
> @@ -157,6 +158,7 @@ arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
>  	case FUTEX_OP_XOR:
>  		__futex_atomic_op("eor	%0, %1, %4", ret, oldval, tmp, uaddr, oparg);
>  		break;
> +#endif
>  	default:
>  		ret = -ENOSYS;
>  	}
> 
> and the below proves it:
> 
> diff --git a/arch/arm/include/asm/futex.h b/arch/arm/include/asm/futex.h
> index e133da303a98..a9151884bc85 100644
> --- 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 oparg, int *oval, u32 __user *uaddr)
>  	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;
>  }
> 
> I think that's the right thing to do anyway. The conditional is pointless.

Thanks for the analysis.

I am still getting this warning, now from Linus' tree builds.

-- 
Cheers,
Stephen Rothwell

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: linux-next: build warning after merge of the tip tree
  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
  1 sibling, 0 replies; 9+ messages in thread
From: Thomas Gleixner @ 2020-04-14  8:42 UTC (permalink / raw)
  To: Stephen Rothwell
  Cc: Ingo Molnar, H. Peter Anvin, Peter Zijlstra,
	Linux Next Mailing List, Linux Kernel Mailing List, Al Viro

Stephen,

Stephen Rothwell <sfr@canb.auug.org.au> writes:
> On Thu, 02 Apr 2020 00:39:55 +0200 Thomas Gleixner <tglx@linutronix.de> wrote:
>> and the below proves it:
>> 
>> diff --git a/arch/arm/include/asm/futex.h b/arch/arm/include/asm/futex.h
>> index e133da303a98..a9151884bc85 100644
>> --- 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 oparg, int *oval, u32 __user *uaddr)
>>  	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;
>>  }
>> 
>> I think that's the right thing to do anyway. The conditional is pointless.
>
> Thanks for the analysis.
>
> I am still getting this warning, now from Linus' tree builds.

Yeah. Just noticed that both of us failed to CC the arm folks. :(

Let me send out a proper patch.

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* ARM: futex: Address build warning
  2020-04-13  0:01         ` Stephen Rothwell
  2020-04-14  8:42           ` Thomas Gleixner
@ 2020-04-14  9:07           ` Thomas Gleixner
  2020-05-06 22:19             ` Stephen Rothwell
  1 sibling, 1 reply; 9+ messages in thread
From: Thomas Gleixner @ 2020-04-14  9:07 UTC (permalink / raw)
  To: Stephen Rothwell
  Cc: Ingo Molnar, H. Peter Anvin, Peter Zijlstra,
	Linux Next Mailing List, Linux Kernel Mailing List, Al Viro,
	Russell King, Arnd Bergmann, linux-arm-kernel

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;
 }

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: ARM: futex: Address build warning
  2020-04-14  9:07           ` ARM: futex: Address build warning Thomas Gleixner
@ 2020-05-06 22:19             ` Stephen Rothwell
  0 siblings, 0 replies; 9+ messages in thread
From: Stephen Rothwell @ 2020-05-06 22:19 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Ingo Molnar, H. Peter Anvin, Peter Zijlstra,
	Linux Next Mailing List, Linux Kernel Mailing List, Al Viro,
	Russell King, Arnd Bergmann, linux-arm-kernel


[-- 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 --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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

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