Re: Thread Safety Analysis and the Linux kernel

[Peter Zijlstra <peterz@infradead.org>]

On Fri, Mar 07, 2025 at 08:59:50AM +0100, Peter Zijlstra wrote:
> On Thu, Mar 06, 2025 at 10:18:32PM +0000, Puchert, Aaron wrote:
> 
> > > 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);
> > >         }
> > > }
> > 
> > Can the return value be used as an initializer by moving the
> > declaration into the scoped_guard block? Or do you have a style guide
> > that wants all declarations at the beginning of a block?
> 
> Yeah, we have a style guide that strongly suggests variables are
> declared at the start. In fact, we used to have
> -Wdeclaration-after-statement and only (finally) got rid of it in order
> to allow for these scope guards.
> 
> > We track capabilities as symbolic expressions, so something like
> > "rq->__lock" in this case. If there is an assignment to "rq", that
> > changes the meaning of the symbolic expression. The object referred to
> > by the expression is then no longer reachable. Currently we don't look
> > at assignments at all when it comes to tracking capabilities. We don't
> > even warn, it's simply documented as not being supported. We only look
> > at initializers, as in alias analysis. Having a separate variable
> > being initialized with the return value gives us a unique name for the
> > return value, which is good when we're working with symbolic
> > expressions.
> > 
> > If there are cases where assignment is really needed, we can also
> > check to which extent we can rewrite expressions or warn when the
> > objects they point to become unreachable. (In the example that would
> > be the case if someone assigned to "rq" again after the call to
> > "__task_rq_lock ".)
> 
> Right. I suspect we might need this if we want to minimize code churn.

There is also a case where a guard wraps the pointer in a struct;
because we need to carry extra state, like IRQ flags and the like, or
because the lock doesn't have a native type (RCU).

In this case the constructor things will return this structure and we'll
get a local copy on stack, a pointer to which will then be handed to the
destructor thing.

It would be very nice if it could understand that pattern too.