https://github.com/melver created https://github.com/llvm/llvm-project/pull/190154
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. Replace the broad heuristic (including a special-case carve-outs for std::bind/bind_front) with a narrower rule based on type mismatch: an alias is now only invalidated if its address is passed to a parameter of a different underlying type (e.g., Foo** to void**). Aliases passed by reference are never invalidated. This type-erasure boundary signals that the pointer is being handled opaquely (such as in generic reallocation or explicit marker functions), providing a semantically stronger signal for invalidation that avoids false positives for common type-preserving calls: the latter are aware of the semantics of the underlying object and typically handle lock transfers correctly (if the aliased object is changed). v1: https://github.com/llvm/llvm-project/pull/187691 - Compared to v1, this also retains the testPointerAliasEscapeMultiple() test case unmodified, as that was explicitly added with a fix to avoid false negative detection! Discussion: https://github.com/llvm/llvm-project/pull/183640 >From a799819065e2253f0fa16fa2c2efd740c39ff48e Mon Sep 17 00:00:00 2001 From: Marco Elver <[email protected]> Date: Wed, 1 Apr 2026 21:18:42 +0200 Subject: [PATCH] Thread Safety Analysis: Drop most 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. Replace the broad heuristic (including a special-case carve-outs for std::bind/bind_front) with a narrower rule based on type mismatch: an alias is now only invalidated if its address is passed to a parameter of a different underlying type (e.g., Foo** to void**). Aliases passed by reference are never invalidated. This type-erasure boundary signals that the pointer is being handled opaquely (such as in generic reallocation or explicit marker functions), providing a semantically stronger signal for invalidation that avoids false positives for common type-preserving calls: the latter are aware of the semantics of the underlying object and typically handle lock transfers correctly (if the aliased object is changed). v1: https://github.com/llvm/llvm-project/pull/187691 - Compared to v1, this also retains the testPointerAliasEscapeMultiple() test case unmodified, as that was explicitly added with a fix to avoid false negative detection! Discussion: https://github.com/llvm/llvm-project/pull/183640 --- clang/docs/ReleaseNotes.rst | 5 +++ clang/lib/Analysis/ThreadSafety.cpp | 35 +++++++---------- .../SemaCXX/warn-thread-safety-analysis.cpp | 39 +++++++++++++++---- 3 files changed, 52 insertions(+), 27 deletions(-) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 061ccd2492c49..d5e918594a4b0 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -353,6 +353,11 @@ Improvements to Clang's diagnostics - Improved ``-Wgnu-zero-variadic-macro-arguments`` to suggest using ``__VA_OPT__`` if the current language version supports it(#GH188624) +- The :doc:`ThreadSafetyAnalysis` dropped most call-based alias + invalidation (alias analysis only with ``-Wthread-safety-beta``), eliminating + false positives when passing local variable aliases by non-const pointer or + reference. + Improvements to Clang's time-trace ---------------------------------- diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 099e8590b50e3..08ee4add36458 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -683,23 +683,15 @@ void VarMapBuilder::VisitBinaryOperator(const BinaryOperator *BO) { } } -// Invalidates local variable definitions if variable escaped. +// Invalidates local variable aliases when passed through a type-mismatching +// pointer cast (e.g., Foo** to void**). This signals that the callee is +// treating the pointer opaquely and likely changes its value, such as in a +// generic reallocation function (e.g. custom_realloc(&ptr, ...)). 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; @@ -708,20 +700,23 @@ void VarMapBuilder::VisitCallExpr(const CallExpr *CE) { 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() && + if (ParamType->isPointerType() && !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 (const auto *DRE = dyn_cast<DeclRefExpr>(SubE)) { + const ValueDecl *VD = DRE->getDecl(); + QualType OriginalTy = + VD->getType().getCanonicalType().getUnqualifiedType(); + QualType PointeeTy = ParamType->getPointeeType() + .getCanonicalType() + .getUnqualifiedType(); + if (OriginalTy != PointeeTy) + VDec = VD; + } } } } diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index 079e068ca7811..35d860a50e94c 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -7494,13 +7494,17 @@ 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: 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. void testPointerAliasEscape1(Foo *f) { Foo *ptr = f; escapeAlias(0, ptr); 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; ptr->mu.Unlock(); } @@ -7509,8 +7513,7 @@ void testPointerAliasEscape2(Foo *f) { escapeAlias(0, &ptr); 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; ptr->mu.Unlock(); } @@ -7521,8 +7524,7 @@ void testPointerAliasEscape3(Foo *f) { escapeAlias(0, &ptr); 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; ptr->mu.Unlock(); } @@ -7538,6 +7540,29 @@ 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. +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); + F->data = 42; + ptr->data = 42; + unlockFooPtr(&ptr); +} + +void customRealloc(void **p, long); +void testPointerAliasTypeErasedEscape(Foo *f) { + Foo *ptr = f; + customRealloc((void**)&ptr, sizeof(*ptr)); + ptr->mu.Lock(); + f->data = 42; // expected-warning{{writing variable 'data' requires holding mutex 'f->mu' exclusively}} \ + // expected-note{{found near match 'ptr->mu'}} + ptr->mu.Unlock(); +} + // A function that may do anything to the objects referred to by the inputs. void escapeAliasMultiple(void *, void *, void *); void testPointerAliasEscapeMultiple(Foo *F) { @@ -7553,7 +7578,7 @@ 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. unlockFooWithEscapablePointer(&L); } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
