[clang] [ThreadSafety] Add heuristic for not invalidating out parameters of annotated functions (PR #183640)

[Marco Elver via cfe-commits]

melver wrote:

> > The problem is that, if lockFooWithEscapablePointer() doesn't take the pointer-to-alias as const, we conservatively invalidate the alias, because after that function call we don't know if F is still pointing to whatever it pointed to before the call. That's technically WAI.
> 
> Hmm, this reminds me of what @delesley wrote on https://reviews.llvm.org/D52395:
> 
> > 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.
> 
> Also later:
> 
> > Adding to the difficulty is the fact that the correct use of "const" in real-world C++ code is spotty at best. There is LOTS of code that arguably should be using const, but doesn't for various reasons.
> 
> Which probably applies even more to C, given that it still doesn't allow conversions like `T**` → `const T* const*` that are allowed in C++.
> 
> The other aspect is that we're not the static analyzer, and we're not trying to track values and their changes over the course of a function. There are too many moving pieces. I think we should use alias analysis only as a fallback, expanding variables if there is no equality.

Right, we're not tracking general values through a function - but pointer aliases are genuinely special: the key capability objects we reason about may not be immediately resolvable.

The alias analysis made it possible to actually start applying -Wthread-safety to more complex code, like the Linux kernel (it's [upstream](https://www.phoronix.com/news/Linux-7.0-Locking) now!). False positives due to alias analysis are rare, and if there are they point out genuinely difficult to reason about code (like the "pass pointer-to-alias-to-lock through function call which then changes the alias to point elsewhere" case).

> What I mean by this is the following: expressions should not be eagerly expanded but stay as written, with the context attached. In the commit message example, we're locking `*C` in the context { `C` = `0` } (a bit strange, as you pointed out, but let's just go with it), then accessing `x` with `*C` locked in the context { `C` = ? }, then unlocking `*C` in the same context.

Syntactic-first matching with no expansion is equivalent to ignoring aliases entirely: F->mu matches F->mu regardless of what F holds, so a genuine rebinding of F between lock and use is silently accepted. That'd be a regression - although I also tend to favor false negatives over false positives I'm not sure we want to go that far.

> Both the access and the unlocking should proceed with the usual AST walk, expanding variables with the context only if they're not equal. In this case, we have `*C` = `*C` for both comparisons, so we stop right there. Only when comparison fails should we attempt to expand one or both sides until we have a match.

Lazy expansion on match failure looks expensive: FactEntry stores only a til::SExpr* - the LVarCtx used to build it is not retained. Re-expanding at comparison time would require storing a full context snapshot alongside every lock in the fact set, which would be equivalent to eager expansion with more state.

> So I agree with @ziqingluo-90 that this behavior is undesirable and should be fixed. We can also discuss the invalidation rules. But the example shouldn't depend on invalidation rules and always be accepted.

I'd probably refine the invalidation rules. What this PR did wasn't inherently wrong, but we need to be aware of the alternatives. I think if you want to address this -- with the known caveats -- this PR could be revived.

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