[clang] Thread Safety Analysis: Support warning on obtaining address of guarded variables (PR #123063)

[Marco Elver via cfe-commits]

melver wrote:

> We had a discussion on https://reviews.llvm.org/D52395 that might be relevant here. To quote @delesley:
> 
> > When you pass an object to a function, either by pointer or by reference, no actual load from memory has yet occurred. Thus, there is a real risk of false positives; what you are saying is that the function _might_ read or write from protected memory, not that it actually will.

Right. Though in the absence of better pointer tracking / alias analysis, I believe there's no way to avoid resulting false positives with pointers. It's something that a user of Wthread-safety-addressof would opt-in explicitly - documentation needs elaboration on this perhaps.

> Another aspect is that we don't check if the lock is still held when we dereference the pointer, so there are also false negatives:
> 
> ```c++
> Mutex mu;
> int x GUARDED_BY(mu);
> 
> void f() {
>   mu.Lock();
>   int *p = &x;
>   mu.Unlock();
>   *p = 0;
> }
> ```

Good point - though I'd prefer few false negatives + false positives over no checks at all. It's a tradeoff, and therefore we definitely shouldn't enable Wthread-safety-addressof by default.

> You use the analogy of C++ references. Interestingly, we don't warn on "taking a reference", partly because no such thing exists. (You can only initialize something of reference type with an expression.) We warn on passing a variable by reference _to a function_, or in other words, initializing a parameter of reference type with a guarded variable. (Since https://reviews.llvm.org/D153131, we also warn on returning a reference when the lock is no longer held after return. Note that the initialization of the reference might still happen under lock!)
> 
> However, we also track references in a way that pointers are not tracked. The reason is probably that references are immutable (that is, `T&` is semantically the same thing as `T* const`).
> 
> ```c++
> void g() {
>   mu.Lock();
>   int &ref = x;
>   mu.Unlock();
>   ref = 0;  // warning: writing variable 'x' requires holding mutex 'mu' exclusively
> }
> ```
> 
> If you "take the reference" outside of the lock and do the assignment inside, there is no warning. Because the assignment is what needs the lock, not taking the address.

Good points, and I missed that reference handling is much better in this regard.

> However, with functions it's somewhat reasonable to assume that the function accesses through the pointer, and that the pointer doesn't escape. (This is of course not always true, but often enough.) So I wonder whether we should restrict this to pointers passed as function arguments.

That's a reasonable option to make it more conservative, although I'm not sure it's necessary (yet).

> But if the number of false positives is manageable, we can also leave it like you implemented it. Did you try it out on some code base that uses thread safety annotations?

I'm working on bringing Wthread-safety to the Linux kernel. The strategy chosen is to convert one subsystem (or single TU) at a time, based on an opt-in basis. Since Linux already has a rather special dialect of C, those that would choose to opt in their TUs would opt into another variant of Linux's C dialect (one with Wthread-safety enabled).

My experience in converting a subsystem I maintain is that applying Wthread-safety already requires small refactors to avoid false positives, and I'm not opposed to add additional constraints via Wthread-safety-addressof. Without additional checking of "obtaining address of guarded variables", I find that Wthread-safety barely provides benefits, as passing pointers to datastructures is so common.

Resolving this will improve the usefulness, and ultimately the chances of us succeeding to upstream this to the Linux kernel.

I don't think we will ever convert 100% of the kernel to use Wthread-safety, but the plan is that willing subsystem maintainers opt in and deal with warnings produced by Wthread-safety. Having Wthread-safety-addressof will add additional coverage. For cases where it produces too many false positives, I intend to add a simple option that can enable/disable addressof checking for individual TUs. Or if almost all places where obtaining the address of a particular lock-guarded variable is _not_ actually under a lock (but still correct), leaving off `guarded_by` might simply be the right choice.

https://github.com/llvm/llvm-project/pull/123063