Re: [PATCH v3 3/3] arm64, compiler-context-analysis: Permit alias analysis through __READ_ONCE() with CONFIG_LTO=y

[Will Deacon <will@kernel.org>]

On Mon, Feb 02, 2026 at 07:29:23PM +0000, David Laight wrote:
> On Mon, 2 Feb 2026 15:39:36 +0000
> Will Deacon <will@kernel.org> wrote:
> 
> > On Fri, Jan 30, 2026 at 02:28:26PM +0100, Marco Elver wrote:
> > > When enabling Clang's Context Analysis (aka. Thread Safety Analysis) on
> > > kernel/futex/core.o (see Peter's changes at [1]), in arm64 LTO builds we
> > > could see:
> > > 
> > > | kernel/futex/core.c:982:1: warning: spinlock 'atomic ? __u.__val : q->lock_ptr' is still held at the end of function [-Wthread-safety-analysis]
> > > |      982 | }
> > > |          | ^
> > > |    kernel/futex/core.c:976:2: note: spinlock acquired here
> > > |      976 |         spin_lock(lock_ptr);
> > > |          |         ^
> > > | kernel/futex/core.c:982:1: warning: expecting spinlock 'q->lock_ptr' to be held at the end of function [-Wthread-safety-analysis]
> > > |      982 | }
> > > |          | ^
> > > |    kernel/futex/core.c:966:6: note: spinlock acquired here
> > > |      966 | void futex_q_lockptr_lock(struct futex_q *q)
> > > |          |      ^
> > > |    2 warnings generated.
> > > 
> > > Where we have:
> > > 
> > > 	extern void futex_q_lockptr_lock(struct futex_q *q) __acquires(q->lock_ptr);
> > > 	..
> > > 	void futex_q_lockptr_lock(struct futex_q *q)
> > > 	{
> > > 		spinlock_t *lock_ptr;
> > > 
> > > 		/*
> > > 		 * See futex_unqueue() why lock_ptr can change.
> > > 		 */
> > > 		guard(rcu)();
> > > 	retry:  
> > > >>		lock_ptr = READ_ONCE(q->lock_ptr);  
> > > 		spin_lock(lock_ptr);
> > > 	...
> > > 	}
> > > 
> > > At the time of the above report (prior to removal of the 'atomic' flag),
> > > Clang Thread Safety Analysis's alias analysis resolved 'lock_ptr' to
> > > 'atomic ?  __u.__val : q->lock_ptr' (now just '__u.__val'), and used
> > > this as the identity of the context lock given it cannot "see through"
> > > the inline assembly; however, we want 'q->lock_ptr' as the canonical
> > > context lock.
> > > 
> > > While for code generation the compiler simplified to '__u.__val' for
> > > pointers (8 byte case -> 'atomic' was set), TSA's analysis (a) happens
> > > much earlier on the AST, and (b) would be the wrong deduction.
> > > 
> > > Now that we've gotten rid of the 'atomic' ternary comparison, we can
> > > return '__u.__val' through a pointer that we initialize with '&x', but
> > > then update via a pointer-to-pointer. When READ_ONCE()'ing a context
> > > lock pointer, TSA's alias analysis does not invalidate the initial alias
> > > when updated through the pointer-to-pointer, and we make it effectively
> > > "see through" the __READ_ONCE().
> > > 
> > > Code generation is unchanged.
> > > 
> > > Link: https://lkml.kernel.org/r/20260121110704.221498346@infradead.org [1]
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Closes: https://lore.kernel.org/oe-kbuild-all/202601221040.TeM0ihff-lkp@intel.com/
> > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > Tested-by: Boqun Feng <boqun@kernel.org>
> > > Signed-off-by: Marco Elver <elver@google.com>
> > > ---
> > > v3:
> > > * Use 'typeof(*__ret)'.
> > > * Commit message.
> > > 
> > > v2:
> > > * Rebase.
> > > ---
> > >  arch/arm64/include/asm/rwonce.h | 10 +++++++---
> > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/arch/arm64/include/asm/rwonce.h b/arch/arm64/include/asm/rwonce.h
> > > index 42c9e8429274..b7de74d4bf07 100644
> > > --- a/arch/arm64/include/asm/rwonce.h
> > > +++ b/arch/arm64/include/asm/rwonce.h
> > > @@ -45,8 +45,12 @@
> > >   */
> > >  #define __READ_ONCE(x)							\
> > >  ({									\
> > > -	typeof(&(x)) __x = &(x);					\
> > > -	union { __rwonce_typeof_unqual(*__x) __val; char __c[1]; } __u;	\
> > > +	auto __x = &(x);						\
> > > +	auto __ret = (__rwonce_typeof_unqual(*__x) *)__x;		\
> > > +	/* Hides alias reassignment from Clang's -Wthread-safety. */	\
> > > +	auto __retp = &__ret;						\
> > > +	union { typeof(*__ret) __val; char __c[1]; } __u;		\
> > > +	*__retp = &__u.__val;						\
> > >  	switch (sizeof(x)) {						\
> > >  	case 1:								\
> > >  		asm volatile(__LOAD_RCPC(b, %w0, %1)			\
> > > @@ -71,7 +75,7 @@
> > >  	default:							\
> > >  		__u.__val = *(volatile typeof(*__x) *)__x;		\
> > >  	}								\
> > > -	__u.__val;							\
> > > +	*__ret;								\
> > >  })  
> > 
> > What does GCC do with this? :/
> 
> GCC currently doesn't see it, LTO is clang only.

LTO is just one way that a compiler could end up breaking dependency
chains, so I really want to maintain the option to enable this path for
GCC in case we run into problems caused by other optimisations in future.

Will