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

[Marco Elver via cfe-commits]

melver wrote:

Addressed comments so far.

> > The equivalent of passing a `pt_guarded_by` variable by value doesn't seem to warn.
> 
> This inconsistency is probably my largest concern. If you have
> 
> ```c
> int x GUARDED_BY(mu);
> int *p PT_GUARDED_BY(mu);
> ```
> 
> then `&x` should basically behave like `p`, and `x` like `*p`. But with the current implementation, that cannot be the case, because `p` is just a no-op.

Right, PT_GUARDED_BY() coverage is lacking in this case.

> That's why I think we should rather check function arguments, and warn on passing (1) the address of a guarded variable, (2) a pointee-guarded variable.

I agree this might be more conservative.

> There is already functionality that makes sure (1) and (2) work together: `checkAccess` and `checkPtAccess` call each other when necessary. This doesn't seem to handle unary `&` at the moment, but I suppose this would be easy to add. The check for passing arguments itself is in `examineArguments`. We currently only check reference type arguments:
> 
> ```c++
>     if (Qt->isReferenceType())
>       checkAccess(*Arg, AK_Read, POK_PassByRef);
> ```
> 
> Here we might simply add a branch for pointer types with the appropriate `POK` that you already introduced (though we might need to rename it slightly).
> 
> This still doesn't cover local pointer variables, but here I guess we need a better understanding of the patterns and how much alias analysis we'd need to unravel them. Local pointer variables as obfuscation device are hopefully not common, so there is likely some more complex logic that would be intractable for us anyway. So I'd be curious to see some interesting examples.

The Linux kernel has odd idioms like this one:
```
#define __READ_ONCE(x)  (*(const volatile __unqual_scalar_typeof(x) *)&(x))
```
When passed a guarded variable (e.g. `READ_ONCE(my_guarded_var)`), this will no longer warn if we do not check addressof.

My intent with Wthread-safety-addressof was to blindly warn on "addressof", so we get this coverage.

But I suppose if we switch it to just cover cases when passing to functions, I'd propose:
- introduce Wthread-safety-pointer as a counterpart to Wthread-safety-reference
- enable it by default similar to Wthread-safety-reference (but initially under Wthread-safety-beta)

I still wouldn't mind if Wthread-safety-addressof existed (for the plethora of weird macros the Linux kernel has), but I understand that maintaining more features might not be what we want.

Preferences?

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