Re: Thread Safety Analysis and the Linux kernel

On Wed, Mar 05, 2025 at 11:54:14PM +0000, Puchert, Aaron wrote:

> > Peter Zijlstra's feedback:
> > https://lore.kernel.org/all/[email protected]/
> > -- much of which led to the below requests. Peter also managed to
> > crash Clang, but that's probably unrelated to -Wthread-safety
> > directly: https://github.com/llvm/llvm-project/issues/129873
> 
> Technically a parser issue and not in the analysis itself. But to my
> knowledge expressions in attributes were originally introduced for the
> analysis, so in some sense it is related.

Right, so that construct came about trying to work around not being able
to reference the return value.

> > 3. Ability to refer to locks in returned reference/pointer. For example:
> >     struct foo *ret_lock_struct(void) ACQUIRE(return->somelock);
> >     struct foo *try_ret_lock_struct(void) TRY_ACQUIRE(1,
> > return->somelock); // locked if non-NULL
> > I expect this also requires basic alias analysis to work so that
> > assigning the returned pointer to a function-local variable and then
> > later use in an unlock function works as expected.
> 
> We had a discussion about this, maybe I can find it later. The idea
> was to introduce a builtin for the return value. A rough outline:
> * Introduce the builtin to the parser, say __builtin_return_value().
> This shouldn't be hard.
> * Add handling to Sema: we can only accept the builtin in attributes
> on functions. The return type is the return type of the function.
> * In the caller we might not need alias analysis: there are no
> variables that alias. We already produce S-expressions for some return
> values from functions, and then handle a DeclStmt with initializer. (I
> introduced that for some C++ patterns in
> https://reviews.llvm.org/D129755.)
> * In the function itself we need to substitute the return expression
> for the builtin before checking the exit set. (I assume this restricts
> the possible attributes to acquire-type attributes.)
> 
> We can also discuss this in more detail if you want to pick it up. It
> might be quite a bit of work, but it should fit nicely into the
> existing framework.

Yes, as far as I can follow, that should work nicely.

The thing I tried to work around yesterday, making clang ICE, was
something like this function:

struct rq *__task_rq_lock(struct task_struct *p)
	__must_hold(p->pi_lock)
	__acquires(_Return->__lock)
{
	struct rq *rq;

	lockdep_assert_held(&p->pi_lock);

	for (;;) {
		rq = task_rq(p);
		raw_spin_rq_lock(rq);
		if (likely(rq == task_rq(p) && !task_on_rq_migrating(p)))
			return rq;

		raw_spin_rq_unlock(rq);

		while (unlikely(task_on_rq_migrating(p)))
			cpu_relax();
	}
}

Except for not having that _Return / __builtin_return_value() thing, I
tried __acquires(task_rq(p)->__lock), and task_rq() expands into a pile
of gunk that made it go *boom*.

Users would typically look like:

try_to_wake_up(p, state)
{
	struct rq *rq;

	scoped_guard (raw_spinlock_irqsave, &p->pi_lock) {
		if (!ttwu_state_match(p, state))
			break;

		rq = __task_rq_lock(p);
		// go enqueue task
		raw_spin_rq_unlock(rq);
	}
}

Anyway, the whole 'find-and-lock' pattern is widely used.