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