* 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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; 11+ 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 related [flat|nested] 11+ 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; 11+ 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] 11+ 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; 11+ 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] 11+ 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 2020-05-06 22:45 ` [tip: locking/urgent] " tip-bot2 for Thomas Gleixner 1 sibling, 2 replies; 11+ 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] 11+ 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 2020-05-06 22:45 ` [tip: locking/urgent] " tip-bot2 for Thomas Gleixner 1 sibling, 0 replies; 11+ 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] 11+ messages in thread
* Re: ARM: futex: Address build warning @ 2020-05-06 22:19 ` Stephen Rothwell 0 siblings, 0 replies; 11+ messages in thread From: Stephen Rothwell @ 2020-05-06 22:19 UTC (permalink / raw) To: Thomas Gleixner Cc: Arnd Bergmann, Peter Zijlstra, Linux Kernel Mailing List, Linux Next Mailing List, Al Viro, H. Peter Anvin, Russell King, Ingo Molnar, linux-arm-kernel [-- Attachment #1.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 #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 11+ messages in thread
* [tip: locking/urgent] ARM: futex: Address build warning 2020-04-14 9:07 ` ARM: futex: Address build warning Thomas Gleixner 2020-05-06 22:19 ` Stephen Rothwell @ 2020-05-06 22:45 ` tip-bot2 for Thomas Gleixner 1 sibling, 0 replies; 11+ messages in thread From: tip-bot2 for Thomas Gleixner @ 2020-05-06 22:45 UTC (permalink / raw) To: linux-tip-commits; +Cc: Stephen Rothwell, Thomas Gleixner, x86, LKML The following commit has been merged into the locking/urgent branch of tip: Commit-ID: 8101b5a1531f3390b3a69fa7934c70a8fd6566ad Gitweb: https://git.kernel.org/tip/8101b5a1531f3390b3a69fa7934c70a8fd6566ad Author: Thomas Gleixner <tglx@linutronix.de> AuthorDate: Tue, 14 Apr 2020 11:07:22 +02:00 Committer: Thomas Gleixner <tglx@linutronix.de> CommitterDate: Thu, 07 May 2020 00:41:47 +02:00 ARM: futex: Address build warning 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://lkml.kernel.org/r/87pncao2ph.fsf@nanos.tec.linutronix.de --- arch/arm/include/asm/futex.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/arch/arm/include/asm/futex.h b/arch/arm/include/asm/futex.h index e133da3..a915188 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; } ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-05-06 22:45 UTC | newest] Thread overview: 11+ 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 2020-05-06 22:19 ` Stephen Rothwell 2020-05-06 22:45 ` [tip: locking/urgent] " tip-bot2 for Thomas Gleixner
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.