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

[Aaron Puchert via cfe-commits]

aaronpuchert wrote:

> 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!).

I'm not suggesting to remove it but only use it as a fallback.

> 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).

You're right, but when you want people to rewrite difficult code you're always fighting an uphill battle. We want the analysis to be something that you can easily turn on without needing to rewrite a lot of code.

> > 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.

Why is it equivalent to ignoring aliases entirely? What we wanted was that different expressions can match because one aliases another. The [motivating example](https://lore.kernel.org/all/569186c5-8663-43df-a01c-d543f57ce5ca@kernel.org/) was this:

```patch
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -52,10 +52,8 @@
   */
  void tty_buffer_lock_exclusive(struct tty_port *port)
  {
-	struct tty_bufhead *buf = &port->buf;
-
-	atomic_inc(&buf->priority);
-	mutex_lock(&buf->lock);
+	atomic_inc(&port->buf.priority);
+	mutex_lock(&port->buf.lock);
```

The problem that you faced was that `buf->lock` didn't match `port->buf.lock`. So the problem was not that the analysis considered things equal that weren't, but the opposite. This is precisely what lazy expansion fixes without introducing a host of new problems.

Otherwise I agree with you, this would be accepted. It should be accepted. Unless we know that the expression's meaning changed for sure (and this should be pretty rare, as I mentioned above), we shouldn't assume that something changed because it could have changed. This is not an alias analysis warning, but a thread safety warning. This is a case of "staying in our lane."

> 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.

Alias analysis as currently implemented is a regression on previous behavior, because it emits (false positive) warnings in code that it previously didn't emit.

The other problem is that conceptually, the warning was always about comparing expressions as written. That's part of the simplicity, and makes it easy to understand for users why a warning is emitted (or not). We shouldn't take that away just because we now have a (very bare bones) alias analysis.

In principle I'd be fine with warning if we know that two expanded expressions are different, but this will only be the case if we reference a local or global variable with no indirections, and I assume this is rare enough that it's not worth the effort. (Complex expressions and aliases tend to occur not around global variables, but around member mutexes.)

> > 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.

The context snapshots are retained anyway to my knowledge, at least until we're done with the function. However, you're right that eager expansion with keeping the original variable would also work. Basically a variable that also contains the expression it would expand to.

> > 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.

We're not going to improve the warning by coming up with more complicated heuristics. Users don't see the context that we're keeping track of and don't see where an alias has been invalidated and why. It's one thing if the analysis says: you locked A but now you unlock B and it's not clear from the code why A and B are the same thing. It's another thing if the analysis says: you locked (an expanded) A but now you unlock A. This is just confusing. And even if we add a note to say where the alias has been invalidated, we're still going to get bug reports that the code is actually fine and there is no easy way to rewrite it.

Another way to put it: our alias analysis is very limited (and will stay very limited), so we should only trust it to that extent. If we end up saying to people: rewrite your code so that our simple alias analysis isn't confused by it, we're going to get comments like the one from Jiri Slaby:

> Fix the analyzer instead.

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