diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h --- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h +++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h @@ -47,7 +47,13 @@ POK_PassByRef, /// Passing a pt-guarded variable by reference. - POK_PtPassByRef + POK_PtPassByRef, + + /// Returning a guarded variable by reference. + POK_ReturnByRef, + + /// Returning a pt-guarded variable by reference. + POK_PtReturnByRef, }; /// This enum distinguishes between different kinds of lock actions. For diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -3844,7 +3844,7 @@ "calling function %0 requires negative capability '%1'">, InGroup, DefaultIgnore; -// Thread safety warnings on pass by reference +// Thread safety warnings on pass/return by reference def warn_guarded_pass_by_reference : Warning< "passing variable %1 by reference requires holding %0 " "%select{'%2'|'%2' exclusively}3">, @@ -3853,6 +3853,14 @@ "passing the value that %1 points to by reference requires holding %0 " "%select{'%2'|'%2' exclusively}3">, InGroup, DefaultIgnore; +def warn_guarded_return_by_reference : Warning< + "returning variable %1 by reference requires holding %0 " + "%select{'%2'|'%2' exclusively}3">, + InGroup, DefaultIgnore; +def warn_pt_guarded_return_by_reference : Warning< + "returning the value that %1 points to by reference requires holding %0 " + "%select{'%2'|'%2' exclusively}3">, + InGroup, DefaultIgnore; // Imprecise thread safety warnings def warn_variable_requires_lock : Warning< diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -1008,7 +1008,7 @@ threadSafety::SExprBuilder SxBuilder; ThreadSafetyHandler &Handler; - const CXXMethodDecl *CurrentMethod = nullptr; + const FunctionDecl *CurrentFunction; LocalVariableMap LocalVarMap; FactManager FactMan; std::vector BlockInfo; @@ -1243,10 +1243,10 @@ // Members are in scope from methods of the same class. if (const auto *P = dyn_cast(SExp)) { - if (!CurrentMethod) + if (!isa_and_nonnull(CurrentFunction)) return false; const ValueDecl *VD = P->clangDecl(); - return VD->getDeclContext() == CurrentMethod->getDeclContext(); + return VD->getDeclContext() == CurrentFunction->getDeclContext(); } return false; @@ -1537,11 +1537,12 @@ /// output error messages related to missing locks. /// FIXME: In future, we may be able to not inherit from a visitor. class BuildLockset : public ConstStmtVisitor { - using VisitorBase = ConstStmtVisitor; friend class ThreadSafetyAnalyzer; ThreadSafetyAnalyzer *Analyzer; FactSet FSet; + // The fact set for the function on exit. + const FactSet &FunctionExitFSet; /// Maps constructed objects to `this` placeholder prior to initialization. llvm::SmallDenseMap ConstructedObjects; LocalVariableMap::Context LVarCtx; @@ -1567,9 +1568,11 @@ bool SkipFirstParam = false); public: - BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo &Info) - : VisitorBase(), Analyzer(Anlzr), FSet(Info.EntrySet), - LVarCtx(Info.EntryContext), CtxIndex(Info.EntryIndex) {} + BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo &Info, + const FactSet &FunctionExitFSet) + : ConstStmtVisitor(), Analyzer(Anlzr), FSet(Info.EntrySet), + FunctionExitFSet(FunctionExitFSet), LVarCtx(Info.EntryContext), + CtxIndex(Info.EntryIndex) {} void VisitUnaryOperator(const UnaryOperator *UO); void VisitBinaryOperator(const BinaryOperator *BO); @@ -1578,6 +1581,7 @@ void VisitCXXConstructExpr(const CXXConstructExpr *Exp); void VisitDeclStmt(const DeclStmt *S); void VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *Exp); + void VisitReturnStmt(const ReturnStmt *S); }; } // namespace @@ -1759,6 +1763,8 @@ // Pass by reference warnings are under a different flag. ProtectedOperationKind PtPOK = POK_VarDereference; if (POK == POK_PassByRef) PtPOK = POK_PtPassByRef; + if (POK == POK_ReturnByRef) + PtPOK = POK_PtReturnByRef; const ValueDecl *D = getValueDecl(Exp); if (!D || !D->hasAttrs()) @@ -2142,6 +2148,25 @@ } } +void BuildLockset::VisitReturnStmt(const ReturnStmt *S) { + if (Analyzer->CurrentFunction == nullptr) + return; + const Expr *RetVal = S->getRetValue(); + if (!RetVal) + return; + + // If returning by reference, check that the function requires the appropriate + // capabilities. + const QualType ReturnType = + Analyzer->CurrentFunction->getReturnType().getCanonicalType(); + if (ReturnType->isLValueReferenceType()) { + Analyzer->checkAccess( + FunctionExitFSet, RetVal, + ReturnType->getPointeeType().isConstQualified() ? AK_Read : AK_Written, + POK_ReturnByRef); + } +} + /// Given two facts merging on a join point, possibly warn and decide whether to /// keep or replace. /// @@ -2251,8 +2276,7 @@ CFG *CFGraph = walker.getGraph(); const NamedDecl *D = walker.getDecl(); - const auto *CurrentFunction = dyn_cast(D); - CurrentMethod = dyn_cast(D); + CurrentFunction = dyn_cast(D); if (D->hasAttr()) return; @@ -2278,7 +2302,7 @@ PostOrderCFGView::CFGBlockSet VisitedBlocks(CFGraph); CFGBlockInfo &Initial = BlockInfo[CFGraph->getEntry().getBlockID()]; - CFGBlockInfo &Final = BlockInfo[CFGraph->getExit().getBlockID()]; + CFGBlockInfo &Final = BlockInfo[CFGraph->getExit().getBlockID()]; // Mark entry block as reachable Initial.Reachable = true; @@ -2348,6 +2372,25 @@ } } + // Compute the expected exit set. + // By default, we expect all locks held on entry to be held on exit. + FactSet ExpectedFunctionExitSet = Initial.EntrySet; + + // Adjust the expected exit set by adding or removing locks, as declared + // by *-LOCK_FUNCTION and UNLOCK_FUNCTION. The intersect below will then + // issue the appropriate warning. + // FIXME: the location here is not quite right. + for (const auto &Lock : ExclusiveLocksAcquired) + ExpectedFunctionExitSet.addLock( + FactMan, std::make_unique(Lock, LK_Exclusive, + D->getLocation())); + for (const auto &Lock : SharedLocksAcquired) + ExpectedFunctionExitSet.addLock( + FactMan, + std::make_unique(Lock, LK_Shared, D->getLocation())); + for (const auto &Lock : LocksReleased) + ExpectedFunctionExitSet.removeLock(FactMan, Lock); + for (const auto *CurrBlock : *SortedGraph) { unsigned CurrBlockID = CurrBlock->getBlockID(); CFGBlockInfo *CurrBlockInfo = &BlockInfo[CurrBlockID]; @@ -2407,7 +2450,7 @@ if (!CurrBlockInfo->Reachable) continue; - BuildLockset LocksetBuilder(this, *CurrBlockInfo); + BuildLockset LocksetBuilder(this, *CurrBlockInfo, ExpectedFunctionExitSet); // Visit all the statements in the basic block. for (const auto &BI : *CurrBlock) { @@ -2474,24 +2517,8 @@ if (!Final.Reachable) return; - // By default, we expect all locks held on entry to be held on exit. - FactSet ExpectedExitSet = Initial.EntrySet; - - // Adjust the expected exit set by adding or removing locks, as declared - // by *-LOCK_FUNCTION and UNLOCK_FUNCTION. The intersect below will then - // issue the appropriate warning. - // FIXME: the location here is not quite right. - for (const auto &Lock : ExclusiveLocksAcquired) - ExpectedExitSet.addLock(FactMan, std::make_unique( - Lock, LK_Exclusive, D->getLocation())); - for (const auto &Lock : SharedLocksAcquired) - ExpectedExitSet.addLock(FactMan, std::make_unique( - Lock, LK_Shared, D->getLocation())); - for (const auto &Lock : LocksReleased) - ExpectedExitSet.removeLock(FactMan, Lock); - // FIXME: Should we call this function for all blocks which exit the function? - intersectAndWarn(ExpectedExitSet, Final.ExitSet, Final.ExitLoc, + intersectAndWarn(ExpectedFunctionExitSet, Final.ExitSet, Final.ExitLoc, LEK_LockedAtEndOfFunction, LEK_NotLockedAtEndOfFunction); Handler.leaveFunction(CurrentFunction); diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -1983,6 +1983,12 @@ case POK_PtPassByRef: DiagID = diag::warn_pt_guarded_pass_by_reference; break; + case POK_ReturnByRef: + DiagID = diag::warn_guarded_return_by_reference; + break; + case POK_PtReturnByRef: + DiagID = diag::warn_pt_guarded_return_by_reference; + break; } PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) << Kind << D @@ -2013,6 +2019,12 @@ case POK_PtPassByRef: DiagID = diag::warn_pt_guarded_pass_by_reference; break; + case POK_ReturnByRef: + DiagID = diag::warn_guarded_return_by_reference; + break; + case POK_PtReturnByRef: + DiagID = diag::warn_pt_guarded_return_by_reference; + break; } PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) << Kind << D diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -5580,6 +5580,85 @@ } }; +class Return { + Mutex mu; + Foo foo GUARDED_BY(mu); + Foo* foo_ptr PT_GUARDED_BY(mu); + + Foo returns_value_locked() { + MutexLock lock(&mu); + return foo; + } + + Foo returns_value_locks_required() EXCLUSIVE_LOCKS_REQUIRED(mu) { + return foo; + } + + Foo returns_value_releases_lock_after_return() UNLOCK_FUNCTION(mu) { + MutexLock lock(&mu, true); + return foo; + } + + Foo returns_value_aquires_lock() EXCLUSIVE_LOCK_FUNCTION(mu) { + mu.Lock(); + return foo; + } + + Foo returns_value_not_locked() { + return foo; // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}} + } + + Foo returns_value_releases_lock_before_return() UNLOCK_FUNCTION(mu) { + mu.Unlock(); + return foo; // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}} + } + + Foo &returns_ref_not_locked() { + return foo; // expected-warning {{returning variable 'foo' by reference requires holding mutex 'mu'}} + } + + Foo &returns_ref_locked() { + MutexLock lock(&mu); + return foo; // expected-warning {{returning variable 'foo' by reference requires holding mutex 'mu'}} + } + + Foo &returns_ref_shared_locks_required() SHARED_LOCKS_REQUIRED(mu) { + return foo; // expected-warning {{returning variable 'foo' by reference requires holding mutex 'mu' exclusively}} + } + + Foo &returns_ref_exclusive_locks_required() EXCLUSIVE_LOCKS_REQUIRED(mu) { + return foo; + } + + Foo &returns_ref_releases_lock_after_return() UNLOCK_FUNCTION(mu) { + MutexLock lock(&mu, true); + return foo; // expected-warning {{returning variable 'foo' by reference requires holding mutex 'mu' exclusively}} + } + + Foo& returns_ref_releases_lock_before_return() UNLOCK_FUNCTION(mu) { + mu.Unlock(); + return foo; // // expected-warning {{returning variable 'foo' by reference requires holding mutex 'mu' exclusively}} + } + + Foo &returns_ref_aquires_lock() EXCLUSIVE_LOCK_FUNCTION(mu) { + mu.Lock(); + return foo; + } + + const Foo &returns_constref_shared_locks_required() SHARED_LOCKS_REQUIRED(mu) { + return foo; + } + + Foo *returns_ptr() { + return &foo; // FIXME -- Do we want to warn on this ? + } + + Foo &returns_ref2() { + return *foo_ptr; // expected-warning {{returning the value that 'foo_ptr' points to by reference requires holding mutex 'mu' exclusively}} + } + +}; + } // end namespace PassByRefTest