https://github.com/melver updated https://github.com/llvm/llvm-project/pull/187691
>From e52576a1bc7bacf48e2c9f55776effd5788f9c04 Mon Sep 17 00:00:00 2001 From: Marco Elver <[email protected]> Date: Mon, 16 Mar 2026 15:47:15 +0100 Subject: [PATCH] Thread Safety Analysis: Drop call-based alias invalidation Local variables passed by non-const pointer or reference to a function were previously invalidated in the LocalVariableMap (VarMapBuilder), on the assumption that the callee might change what they point to. This caused false positives when the function also carries ACQUIRE/RELEASE annotations: handleCall translates those annotations with the pre-call context, while subsequent guard checks use the post-invalidation context, producing an expansion mismatch and a spurious warning. The invalidation rules were a heuristic with significant complexity (including a special-case carve-out for std::bind/bind_front) and unclear benefit. Instead of adding more heuristics, drop the alias-invalidation rules entirely. --- clang/lib/Analysis/ThreadSafety.cpp | 52 ------------------- .../SemaCXX/warn-thread-safety-analysis.cpp | 50 +++++++++++++----- 2 files changed, 36 insertions(+), 66 deletions(-) diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 2c5976af9f61d..8687dbf46d90c 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -637,7 +637,6 @@ class VarMapBuilder : public ConstStmtVisitor<VarMapBuilder> { void VisitDeclStmt(const DeclStmt *S); void VisitBinaryOperator(const BinaryOperator *BO); - void VisitCallExpr(const CallExpr *CE); }; } // namespace @@ -683,57 +682,6 @@ void VarMapBuilder::VisitBinaryOperator(const BinaryOperator *BO) { } } -// Invalidates local variable definitions if variable escaped. -void VarMapBuilder::VisitCallExpr(const CallExpr *CE) { - const FunctionDecl *FD = CE->getDirectCallee(); - if (!FD) - return; - - // Heuristic for likely-benign functions that pass by mutable reference. This - // is needed to avoid a slew of false positives due to mutable reference - // passing where the captured reference is usually passed on by-value. - if (const IdentifierInfo *II = FD->getIdentifier()) { - // Any kind of std::bind-like functions. - if (II->isStr("bind") || II->isStr("bind_front")) - return; - } - - // Invalidate local variable definitions that are passed by non-const - // reference or non-const pointer. - for (unsigned Idx = 0; Idx < CE->getNumArgs(); ++Idx) { - if (Idx >= FD->getNumParams()) - break; - - const Expr *Arg = CE->getArg(Idx)->IgnoreParenImpCasts(); - const ParmVarDecl *PVD = FD->getParamDecl(Idx); - QualType ParamType = PVD->getType(); - - // Potential reassignment if passed by non-const reference / pointer. - const ValueDecl *VDec = nullptr; - if (ParamType->isReferenceType() && - !ParamType->getPointeeType().isConstQualified()) { - if (const auto *DRE = dyn_cast<DeclRefExpr>(Arg)) - VDec = DRE->getDecl(); - } else if (ParamType->isPointerType() && - !ParamType->getPointeeType().isConstQualified()) { - Arg = Arg->IgnoreParenCasts(); - if (const auto *UO = dyn_cast<UnaryOperator>(Arg)) { - if (UO->getOpcode() == UO_AddrOf) { - const Expr *SubE = UO->getSubExpr()->IgnoreParenCasts(); - if (const auto *DRE = dyn_cast<DeclRefExpr>(SubE)) - VDec = DRE->getDecl(); - } - } - } - - if (VDec) - Ctx = VMap->clearDefinition(VDec, Ctx); - } - // Save the context after the call where escaped variables' definitions (if - // they exist) are cleared. - VMap->saveContext(CE, Ctx); -} - // Computes the intersection of two contexts. The intersection is the // set of variables which have the same definition in both contexts; // variables with different definitions are discarded. diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index a6c9a2236fc45..7a277c47b6109 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -7440,23 +7440,29 @@ void testPointerAliasNoEscape3() { ptr->mu.Unlock(); } +// Passing an alias by non-const reference or pointer-to-pointer to a function +// is no longer treated as an escape: the alias is retained and the lock +// acquired through it continues to cover accesses via the original pointer. +// This is a deliberate trade-off: dropping call-based alias invalidation +// eliminates false positives (e.g. annotated acquire/release functions that +// incidentally take the alias by mutable pointer) at the cost of not warning +// when the called function actually changes what the alias points to. Direct +// reassignment (ptr = ...) is still tracked and will produce a warning. void testPointerAliasEscape1(Foo *f) { Foo *ptr = f; - escapeAlias(0, ptr); + escapeAlias(0, ptr); // pass by non-const ref — alias retained ptr->mu.Lock(); - f->data = 42; // expected-warning{{writing variable 'data' requires holding mutex 'f->mu' exclusively}} \ - // expected-note{{found near match 'ptr->mu'}} + f->data = 42; // ok: ptr->mu covers f->mu (alias still live) ptr->mu.Unlock(); } void testPointerAliasEscape2(Foo *f) { Foo *ptr = f; - escapeAlias(0, &ptr); + escapeAlias(0, &ptr); // pass by non-const pointer — alias retained ptr->mu.Lock(); - f->data = 42; // expected-warning{{writing variable 'data' requires holding mutex 'f->mu' exclusively}} \ - // expected-note{{found near match 'ptr->mu'}} + f->data = 42; // ok: alias retained ptr->mu.Unlock(); } @@ -7464,11 +7470,10 @@ void testPointerAliasEscape3(Foo *f) { Foo *ptr; ptr = f; - escapeAlias(0, &ptr); + escapeAlias(0, &ptr); // alias retained ptr->mu.Lock(); - f->data = 42; // expected-warning{{writing variable 'data' requires holding mutex 'f->mu' exclusively}} \ - // expected-note{{found near match 'ptr->mu'}} + f->data = 42; // ok: alias retained ptr->mu.Unlock(); } @@ -7484,22 +7489,39 @@ void testPointerAliasEscapeAndReset(Foo *f) { ptr->mu.Unlock(); } +// Annotated acquire/release functions that incidentally take the alias by +// non-const pointer must not produce false positives. Previously, passing +// `&ptr` to such a function would clear ptr's definition, causing the lock +// fact (recorded with the pre-call expansion) to mismatch the guard check +// (recorded with the post-call expansion). +void lockFooPtr(Foo **Fp) EXCLUSIVE_LOCK_FUNCTION((*Fp)->mu); +void unlockFooPtr(Foo **Fp) EXCLUSIVE_UNLOCK_FUNCTION((*Fp)->mu); + +void testAnnotatedAcquireReleaseNoFalsePositive(Foo *F) { + Foo *ptr = F; + lockFooPtr(&ptr); // acquires ptr->mu (= F->mu) + F->data = 42; // ok: no spurious warning + ptr->data = 42; // ok + unlockFooPtr(&ptr); +} + // A function that may do anything to the objects referred to by the inputs. void escapeAliasMultiple(void *, void *, void *); void testPointerAliasEscapeMultiple(Foo *F) { Foo *L; - F->mu.Lock(); // expected-note{{mutex acquired here}} + F->mu.Lock(); Foo *Fp = F; - escapeAliasMultiple(&L, &L, &Fp); - Fp->mu.Unlock(); // expected-warning{{releasing mutex 'Fp->mu' that was not held}} -} // expected-warning{{mutex 'F->mu' is still held at the end of function}} + escapeAliasMultiple(&L, &L, &Fp); // Fp alias retained + Fp->mu.Unlock(); // ok: Fp still aliases F +} void unlockFooWithEscapablePointer(Foo **Fp) EXCLUSIVE_UNLOCK_FUNCTION((*Fp)->mu); void testEscapeInvalidationHappensRightAfterTheCall(Foo* F) { Foo* L; L = F; L->mu.Lock(); - // Release the lock held by 'L' before clearing its definition. + // L's alias is retained across the call; the UNLOCK annotation resolves + // (*&L)->mu = L->mu = F->mu, correctly releasing the held lock. unlockFooWithEscapablePointer(&L); } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
