RE: Thread Safety Analysis and the Linux kernel

> From: Peter Zijlstra <[email protected]>
> So one of the most important use cases from my perspective is the
> interaction with __attribute__((cleanup())), which we use to build
> scope based guards in C.
> 
> And most of that might be fine with using 'T * const', but the support
> for conditional locks explicitly needs to set the pointer NULL when the
> acquire fails.
> 
> We have a macro used like:
> 
> DEFINE_GUARD(mutex, struct mutex *, mutex_lock(_T), mutex_unlock(_T))
> 
> Which expands to:
> 
> static inline void class_mutex_destructor(struct mutex **p)
> {
> 	struct mutex *_T = *p;
> 	if (_T) { mutex_unlock(_T); }
> }
> 
> static inline struct mutex * class_mutex_constructor(struct mutex *_T)
> {
> 	struct mutex * t = ({ mutex_lock(_T); _T; });
> 	return t;
> }
> 
> This is then typically used like:
> 
> 	scoped_guard (mutex, &my_lock) {
> 		...
> 	}
> 
> Which in turn expands to:
> 
> 	for (struct mutex *scope
> __attribute__((cleanup(class_mutex_destructor))) =
> 	     class_mutex_constructor(&my_lock); scope; ({ goto _label; }))
> 		if (0) {
> _label:
> 			break;
> 		} else {
> 
> 		...
> 	}
> 
> And here I can probably make the guard() thing add const like:
> 
> 		struct mutex * const __UNIQUE_ID_guard_123
> 			__attribute__((cleanup(class_mutex_destructor) =
> 				class_mutex_constructor(&my_lock);

Yes, and the scope variable is hidden to the macro user, so it cannot be accessed anyway. So in that case we should also be fine.

> But note that _constructor() is still laundering this through a
> non-const version,

That's not an issue. Remember that we analyze function bodies in isolation. In the worst case we just put a "no_thread_safety_analysis" attribute on the constructor.

One problem though is that the constructor symbolically locks my_lock, while the destructor symbolically unlocks __UNIQUE_ID_guard_123. We need to be able to figure out that they're the same thing. I see three possibilities, without having tried any of them:

1. Annotate the constructor with __attribute__((return_capability(_T))).
2. Create an alias before we call the constructor, so that constructor and destructor get the same argument. But this wouldn't work for try_acquire: the alias couldn't be const.
3. We model the cleanup-annotated variable like a C++ scoped lock.

We model C++ scoped locks as objects that "manage" a fixed set of capabilities. In the example above, __UNIQUE_ID_guard_123 would be a scoped lock that manages my_lock. We have an attribute to indicate scope types. We could use that here by wrapping the pointer in an annotated struct:

struct __attribute__((scoped_capability)) mutex_scope {
	struct mutex * /*const? */ t;
};

Or we say that every variable is a scoped lock, if it has type "pointer to a capability-annotated type" and a cleanup attribute. But it could be an issue if being a scoped lock is no longer a type property. Or that we'll scoop up too much this way. We'll have to see.

> and while the above could have:
>
> 	struct mutex * t const = ...
> 
> That no longer works for the case of:
> 
> DEFINE_GUARD_COND(mutex, _try, mutex_trylock(_T))
> 
> which expands to have a constructor like:
> 
> static inline struct mutex * class_mutex_try_constructor(struct mutex *_T)
> {
> 	struct mutex * t = ({ void *_t = _T; if (_T && !(mutex_trylock(_T))) _t =
> NULL; _t; });
> 	return t;
> }
> 
> Notably that NULL assignment on lock failure is not happy with having
> that const on.

Same as above: what's important is the "const" where it's being used. If we have to put "no_thread_safety_analysis" on this one function, we don't lose much. You'll likely not even need that, because we don't attempt to analyze functions annotated with try-acquire attributes (try_acquire_capability and try_acquire_shared_capability). (Basically, we bail out on mutex implementation internals, which we recognize based on certain attributes.)

However, with try-acquire the wrapping might be counterproductive. We want the return value to be ("contextually" in C++) convertible to bool. Pointers are convertible to bool, but structs are not. So it seems like we need to "guess" scoped locks. Or we put an annotation on the constructor.

> Note that this combination of trylock and scoped_guard() results in not
> taking the for-loop on failure. Eg:
> 
> 	scoped_guard (mutex_trylock, &my_lock) {
> 		print("ponies\n");
> 	}
> 
> Will in fact only print 'ponies' when it got the lock.

This should in principle be supported. Once you branch on the return value of a try-acquire-annotated function, we introduce the capability in the correct branch. Because the destructor is unconditionally executed though, we likely need to model this as scoped capability, otherwise we get complaints that we release a lock that we don't hold. Although we could also add special treatment for cleanup function calls.

> static inline void class_mutex_destructor(struct mutex * const *p)
> 	__releases(*p) __no_capability_analysis
> {
> 	struct mutex *_T = *p;
> 	if (_T) { mutex_unlock(_T); }
> }
> 
> static inline struct mutex * class_mutex_constructor(struct mutex *_T)
> 	__acquires(_T) __no_capability_analysis
> {
> 	struct mutex * t = ({ mutex_lock(_T); _T; });
> 	return t;
> }
> 
> static inline struct mutex * class_mutex_try_constructor(struct mutex *_T)
> 	__const_acquires(nonnull, _T) __no_capability_analysis
> {
> 	struct mutex * t = ({ void *_t = _T; if (_T && !(mutex_trylock(_T))) _t =
> NULL; _t; });
> 	return t;
> }

That looks reasonable, assuming that __const_acquires is your macro for try_acquire_capability.

> the: struct mutex * const in the for() construct might just be enough.
> 
> 
> Would this be sufficient consty for it to untangle this web?

I think so. The cleanup function is going to be called on &__UNIQUE_ID_guard_123, and the annotation says we release *&__UNIQUE_ID_guard_123 = __UNIQUE_ID_guard_123. Then we check the type: is it a reference or const pointer? It is, so we look up the initializer in a map. We substitute that and get class_mutex_constructor(&my_lock). There are two ways of looking through the call: __attribute__((return_capability(...))) or the scoped locks idea. We don't look into the constructor, so it can do whatever it wants.

Aaron