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/[email protected]/)
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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
