Stephen,
Stephen Rothwell <sfr@xxxxxxxxxxxxxxxx> writes:
> On Wed, 01 Apr 2020 12:25:25 +0200 Thomas Gleixner <tglx@xxxxxxxxxxxxx> 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
