diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h index 6c97905a2d7f9..cd2c278c2278f 100644 --- a/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h +++ b/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h @@ -395,7 +395,7 @@ class SExprBuilder { CapabilityExpr translateAttrExpr(const Expr *AttrExp, CallingContext *Ctx); // Translate a variable reference. - til::LiteralPtr *createVariable(const VarDecl *VD); + til::SExpr *createVariable(const VarDecl *VD, CallingContext *Ctx = nullptr); // Translate a clang statement or expression to a TIL expression. // Also performs substitution of variables; Ctx provides the context. @@ -501,6 +501,9 @@ class SExprBuilder { void mergeEntryMapBackEdge(); void mergePhiNodesBackEdge(const CFGBlock *Blk); + // Returns true if a variable is assumed to be reassigned. + bool isVariableReassigned(const VarDecl *VD); + private: // Set to true when parsing capability expressions, which get translated // inaccurately in order to hack around smart pointers etc. @@ -531,6 +534,9 @@ class SExprBuilder { std::vector IncompleteArgs; til::BasicBlock *CurrentBB = nullptr; BlockInfo *CurrentBlockInfo = nullptr; + + // Map caching if a local variable is reassigned. + llvm::DenseMap LocalVariableReassigned; }; #ifndef NDEBUG diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp index 80e7c8eff671a..a0ab241125d1c 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -1141,11 +1141,10 @@ class ThreadSafetyAnalyzer { void warnIfMutexNotHeld(const FactSet &FSet, const NamedDecl *D, const Expr *Exp, AccessKind AK, Expr *MutexExp, - ProtectedOperationKind POK, til::LiteralPtr *Self, + ProtectedOperationKind POK, til::SExpr *Self, SourceLocation Loc); void warnIfMutexHeld(const FactSet &FSet, const NamedDecl *D, const Expr *Exp, - Expr *MutexExp, til::LiteralPtr *Self, - SourceLocation Loc); + Expr *MutexExp, til::SExpr *Self, SourceLocation Loc); void checkAccess(const FactSet &FSet, const Expr *Exp, AccessKind AK, ProtectedOperationKind POK); @@ -1599,7 +1598,7 @@ class BuildLockset : public ConstStmtVisitor { } void handleCall(const Expr *Exp, const NamedDecl *D, - til::LiteralPtr *Self = nullptr, + til::SExpr *Self = nullptr, SourceLocation Loc = SourceLocation()); void examineArguments(const FunctionDecl *FD, CallExpr::const_arg_iterator ArgBegin, @@ -1629,7 +1628,7 @@ class BuildLockset : public ConstStmtVisitor { /// of at least the passed in AccessKind. void ThreadSafetyAnalyzer::warnIfMutexNotHeld( const FactSet &FSet, const NamedDecl *D, const Expr *Exp, AccessKind AK, - Expr *MutexExp, ProtectedOperationKind POK, til::LiteralPtr *Self, + Expr *MutexExp, ProtectedOperationKind POK, til::SExpr *Self, SourceLocation Loc) { LockKind LK = getLockKindFromAccessKind(AK); CapabilityExpr Cp = SxBuilder.translateAttrExpr(MutexExp, D, Exp, Self); @@ -1688,8 +1687,7 @@ void ThreadSafetyAnalyzer::warnIfMutexNotHeld( /// Warn if the LSet contains the given lock. void ThreadSafetyAnalyzer::warnIfMutexHeld(const FactSet &FSet, const NamedDecl *D, const Expr *Exp, - Expr *MutexExp, - til::LiteralPtr *Self, + Expr *MutexExp, til::SExpr *Self, SourceLocation Loc) { CapabilityExpr Cp = SxBuilder.translateAttrExpr(MutexExp, D, Exp, Self); if (Cp.isInvalid()) { @@ -1857,7 +1855,7 @@ void ThreadSafetyAnalyzer::checkPtAccess(const FactSet &FSet, const Expr *Exp, /// of an implicitly called cleanup function. /// \param Loc If \p Exp = nullptr, the location. void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D, - til::LiteralPtr *Self, SourceLocation Loc) { + til::SExpr *Self, SourceLocation Loc) { CapExprSet ExclusiveLocksToAdd, SharedLocksToAdd; CapExprSet ExclusiveLocksToRemove, SharedLocksToRemove, GenericLocksToRemove; CapExprSet ScopedReqsAndExcludes; @@ -1869,7 +1867,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D, const auto *TagT = Exp->getType()->getAs(); if (D->hasAttrs() && TagT && Exp->isPRValue()) { til::LiteralPtr *Placeholder = - Analyzer->SxBuilder.createVariable(nullptr); + cast(Analyzer->SxBuilder.createVariable(nullptr)); [[maybe_unused]] auto inserted = Analyzer->ConstructedObjects.insert({Exp, Placeholder}); assert(inserted.second && "Are we visiting the same expression again?"); diff --git a/clang/lib/Analysis/ThreadSafetyCommon.cpp b/clang/lib/Analysis/ThreadSafetyCommon.cpp index ddbd0a9ca904b..d95dfa087fdf0 100644 --- a/clang/lib/Analysis/ThreadSafetyCommon.cpp +++ b/clang/lib/Analysis/ThreadSafetyCommon.cpp @@ -19,6 +19,7 @@ #include "clang/AST/Expr.h" #include "clang/AST/ExprCXX.h" #include "clang/AST/OperationKinds.h" +#include "clang/AST/RecursiveASTVisitor.h" #include "clang/AST/Stmt.h" #include "clang/AST/Type.h" #include "clang/Analysis/Analyses/ThreadSafetyTIL.h" @@ -241,7 +242,22 @@ CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp, return CapabilityExpr(E, AttrExp->getType(), Neg); } -til::LiteralPtr *SExprBuilder::createVariable(const VarDecl *VD) { +til::SExpr *SExprBuilder::createVariable(const VarDecl *VD, + CallingContext *Ctx) { + if (VD) { + // Substitute local pointer variables with their initializers if they are + // explicitly const or never reassigned. + QualType Ty = VD->getType(); + if (Ty->isPointerType() && VD->hasInit() && !isVariableReassigned(VD)) { + const Expr *Init = VD->getInit()->IgnoreParenImpCasts(); + // Check for self-initialization to prevent infinite recursion. + if (const auto *InitDRE = dyn_cast(Init)) { + if (InitDRE->getDecl()->getCanonicalDecl() == VD->getCanonicalDecl()) + return new (Arena) til::LiteralPtr(VD); + } + return translate(Init, Ctx); + } + } return new (Arena) til::LiteralPtr(VD); } @@ -353,6 +369,9 @@ til::SExpr *SExprBuilder::translateDeclRefExpr(const DeclRefExpr *DRE, : cast(D)->getCanonicalDecl()->getParamDecl(I); } + if (const auto *VarD = dyn_cast(VD)) + return createVariable(VarD, Ctx); + // For non-local variables, treat it as a reference to a named object. return new (Arena) til::LiteralPtr(VD); } @@ -1012,6 +1031,63 @@ void SExprBuilder::exitCFG(const CFGBlock *Last) { IncompleteArgs.clear(); } +bool SExprBuilder::isVariableReassigned(const VarDecl *VD) { + // Note: The search is performed lazily per-variable and result is cached. An + // alternative would have been to eagerly create a set of all reassigned + // variables, but that would consume significantly more memory. The number of + // variables needing the reassignment check in a single function is expected + // to be small. Also see createVariable(). + struct ReassignmentFinder : RecursiveASTVisitor { + explicit ReassignmentFinder(const VarDecl *VD) : QueryVD(VD) { + assert(QueryVD->getCanonicalDecl() == QueryVD); + } + + bool VisitBinaryOperator(BinaryOperator *BO) { + if (!BO->isAssignmentOp()) + return true; + auto *DRE = dyn_cast(BO->getLHS()->IgnoreParenImpCasts()); + if (!DRE) + return true; + auto *AssignedVD = dyn_cast(DRE->getDecl()); + if (!AssignedVD) + return true; + // Don't count the initialization itself as a reassignment. + if (AssignedVD->hasInit() && + AssignedVD->getInit()->getBeginLoc() == BO->getBeginLoc()) + return true; + // If query variable appears as LHS of assignment. + if (QueryVD == AssignedVD->getCanonicalDecl()) { + FoundReassignment = true; + return false; // stop + } + return true; + } + + const VarDecl *QueryVD; + bool FoundReassignment = false; + }; + + if (VD->getType().isConstQualified()) + return false; // Assume UB-freedom. + if (!VD->isLocalVarDecl()) + return true; // Not a local variable (assume reassigned). + auto *FD = dyn_cast(VD->getDeclContext()); + if (!FD) + return true; // Assume reassigned. + + // Try to look up in cache; use the canonical declaration to ensure consistent + // lookup in the cache. + VD = VD->getCanonicalDecl(); + auto It = LocalVariableReassigned.find(VD); + if (It != LocalVariableReassigned.end()) + return It->second; + + ReassignmentFinder Visitor(VD); + // const_cast ok: FunctionDecl not modified. + Visitor.TraverseDecl(const_cast(FD)); + return LocalVariableReassigned[VD] = Visitor.FoundReassignment; +} + #ifndef NDEBUG namespace { diff --git a/clang/test/Sema/warn-thread-safety-analysis.c b/clang/test/Sema/warn-thread-safety-analysis.c index b43f97af9162e..549cb1231baa6 100644 --- a/clang/test/Sema/warn-thread-safety-analysis.c +++ b/clang/test/Sema/warn-thread-safety-analysis.c @@ -184,9 +184,13 @@ int main(void) { /// Cleanup functions { struct Mutex* const __attribute__((cleanup(unlock_scope))) scope = &mu1; - mutex_exclusive_lock(scope); // Note that we have to lock through scope, because no alias analysis! + mutex_exclusive_lock(scope); // Lock through scope works. // Cleanup happens automatically -> no warning. } + { + struct Mutex* const __attribute__((unused, cleanup(unlock_scope))) scope = &mu1; + mutex_exclusive_lock(&mu1); // With basic alias analysis lock through mu1 also works. + } foo_.a_value = 0; // expected-warning {{writing variable 'a_value' requires holding mutex 'mu_' exclusively}} *foo_.a_ptr = 1; // expected-warning {{writing the value pointed to by 'a_ptr' requires holding mutex 'bar.other_mu' exclusively}} diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp index d64ed1e5f260a..da5fd350daf74 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -1556,9 +1556,9 @@ void main() { Child *c; Base *b = c; - b->func1(); // expected-warning {{calling function 'func1' requires holding mutex 'b->mu_' exclusively}} + b->func1(); // expected-warning {{calling function 'func1' requires holding mutex 'c->mu_' exclusively}} b->mu_.Lock(); - b->func2(); // expected-warning {{cannot call function 'func2' while mutex 'b->mu_' is held}} + b->func2(); // expected-warning {{cannot call function 'func2' while mutex 'c->mu_' is held}} b->mu_.Unlock(); c->func1(); // expected-warning {{calling function 'func1' requires holding mutex 'c->mu_' exclusively}} @@ -7224,3 +7224,101 @@ class TestNegativeWithReentrantMutex { }; } // namespace Reentrancy + +// Tests for tracking aliases of capabilities. +namespace CapabilityAliases { +struct Foo { + Mutex mu; + int data GUARDED_BY(mu); +}; + +void testBasicPointerAlias(Foo *f) { + Foo *ptr = f; + ptr->mu.Lock(); // lock through alias + f->data = 42; // access through original + ptr->mu.Unlock(); // unlock through alias +} + +// FIXME: No alias tracking for non-const reassigned pointers. +void testReassignment() { + Foo f1, f2; + Foo *ptr = &f1; + ptr->mu.Lock(); + f1.data = 42; // expected-warning{{writing variable 'data' requires holding mutex 'f1.mu' exclusively}} \ + // expected-note{{found near match 'ptr->mu'}} + ptr->mu.Unlock(); + + ptr = &f2; // reassign + ptr->mu.Lock(); + f2.data = 42; // expected-warning{{writing variable 'data' requires holding mutex 'f2.mu' exclusively}} \ + // expected-note{{found near match 'ptr->mu'}} + f1.data = 42; // expected-warning{{writing variable 'data' requires holding mutex 'f1.mu'}} \ + // expected-note{{found near match 'ptr->mu'}} + ptr->mu.Unlock(); +} + +// Nested field access through pointer +struct Container { + Foo foo; +}; + +void testNestedAccess(Container *c) { + Foo *ptr = &c->foo; // pointer to nested field + ptr->mu.Lock(); + c->foo.data = 42; + ptr->mu.Unlock(); +} + +void testNestedAcquire(Container *c) EXCLUSIVE_LOCK_FUNCTION(&c->foo.mu) { + Foo *buf = &c->foo; + buf->mu.Lock(); +} + +struct ContainerOfPtr { + Foo *foo_ptr; +}; + +void testIndirectAccess(ContainerOfPtr *fc) { + Foo *ptr = fc->foo_ptr; // get pointer + ptr->mu.Lock(); + fc->foo_ptr->data = 42; // access via original + ptr->mu.Unlock(); +} + +// FIXME: No alias tracking through complex control flow. +void testSimpleControlFlow(Foo *f1, Foo *f2, bool cond) { + Foo *ptr; + if (cond) { + ptr = f1; + } else { + ptr = f2; + } + ptr->mu.Lock(); + if (cond) { + f1->data = 42; // expected-warning{{writing variable 'data' requires holding mutex 'f1->mu' exclusively}} \ + // expected-note{{found near match 'ptr->mu'}} + } else { + f2->data = 42; // expected-warning{{writing variable 'data' requires holding mutex 'f2->mu' exclusively}} \ + // expected-note{{found near match 'ptr->mu'}} + } + ptr->mu.Unlock(); +} + +void testLockFunction(Foo *f) EXCLUSIVE_LOCK_FUNCTION(&f->mu) { + Mutex *mu = &f->mu; + mu->Lock(); +} + +void testUnlockFunction(Foo *f) UNLOCK_FUNCTION(&f->mu) { + Mutex *mu = &f->mu; + mu->Unlock(); +} + +// Semantically UB, but let's not crash the compiler with this (should be +// handled by -Wuninitialized). +void testSelfInit() { + Mutex *mu = mu; // don't do this at home + mu->Lock(); + mu->Unlock(); +} +} // namespace CapabilityAliases