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,10 @@ POK_PassByRef, /// Passing a pt-guarded variable by reference. - POK_PtPassByRef + POK_PtPassByRef, + + /// Returning a guarded variable by reference. + POK_ReturnByRef, }; /// This enum distinguishes between different kinds of lock actions. For diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -1044,6 +1044,7 @@ def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">; def ThreadSafetyPrecise : DiagGroup<"thread-safety-precise">; def ThreadSafetyReference : DiagGroup<"thread-safety-reference">; +def ThreadSafetyReturn : DiagGroup<"thread-safety-return">; def ThreadSafetyNegative : DiagGroup<"thread-safety-negative">; def ThreadSafety : DiagGroup<"thread-safety", [ThreadSafetyAttributes, 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 @@ -3802,6 +3802,12 @@ "%select{'%2'|'%2' exclusively}3">, InGroup, DefaultIgnore; +// Thread safety warnings on return +def warn_guarded_return_by_reference : Warning< + "returning variable %1 by reference requires holding %0 " + "%select{'%2'|'%2' exclusively}3">, + InGroup, DefaultIgnore; + // Imprecise thread safety warnings def warn_variable_requires_lock : Warning< "%select{reading|writing}3 variable %1 requires holding %0 " 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 @@ -41,6 +41,7 @@ #include "llvm/ADT/DenseMap.h" #include "llvm/ADT/ImmutableMap.h" #include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/SmallPtrSet.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Allocator.h" @@ -1009,7 +1010,7 @@ threadSafety::SExprBuilder SxBuilder; ThreadSafetyHandler &Handler; - const CXXMethodDecl *CurrentMethod; + const FunctionDecl *CurrentFunction; LocalVariableMap LocalVarMap; FactManager FactMan; std::vector BlockInfo; @@ -1244,10 +1245,10 @@ // Members are in scope from methods of the same class. if (const auto *P = dyn_cast(SExp)) { - if (!CurrentMethod) + if (CurrentFunction == nullptr || !isa(CurrentFunction)) return false; const ValueDecl *VD = P->clangDecl(); - return VD->getDeclContext() == CurrentMethod->getDeclContext(); + return VD->getDeclContext() == CurrentFunction->getDeclContext(); } return false; @@ -1538,11 +1539,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 (i.e., its entry block). + const FactSet &FunctionFSet; /// Maps constructed objects to `this` placeholder prior to initialization. llvm::SmallDenseMap ConstructedObjects; LocalVariableMap::Context LVarCtx; @@ -1568,9 +1570,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 &FunctionFSet) + : ConstStmtVisitor(), Analyzer(Anlzr), FSet(Info.EntrySet), + FunctionFSet(FunctionFSet), LVarCtx(Info.EntryContext), + CtxIndex(Info.EntryIndex) {} void VisitUnaryOperator(const UnaryOperator *UO); void VisitBinaryOperator(const BinaryOperator *BO); @@ -1579,6 +1583,7 @@ void VisitCXXConstructExpr(const CXXConstructExpr *Exp); void VisitDeclStmt(const DeclStmt *S); void VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *Exp); + void VisitReturnStmt(const ReturnStmt *S); }; } // namespace @@ -2143,6 +2148,19 @@ } } +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(FunctionFSet, 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. /// @@ -2252,8 +2270,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,6 +2295,9 @@ const PostOrderCFGView *SortedGraph = walker.getSortedGraph(); PostOrderCFGView::CFGBlockSet VisitedBlocks(CFGraph); + CFGBlockInfo *Initial = &BlockInfo[CFGraph->getEntry().getBlockID()]; + CFGBlockInfo *Final = &BlockInfo[CFGraph->getExit().getBlockID()]; + // Mark entry block as reachable BlockInfo[CFGraph->getEntry().getBlockID()].Reachable = true; @@ -2295,8 +2315,7 @@ // to initial lockset. Also turn off checking for lock and unlock functions. // FIXME: is there a more intelligent way to check lock/unlock functions? if (!SortedGraph->empty() && D->hasAttrs()) { - const CFGBlock *FirstBlock = *SortedGraph->begin(); - FactSet &InitialLockset = BlockInfo[FirstBlock->getBlockID()].EntrySet; + FactSet &InitialLockset = Initial->EntrySet; CapExprSet ExclusiveLocksToAdd; CapExprSet SharedLocksToAdd; @@ -2405,7 +2424,7 @@ if (!CurrBlockInfo->Reachable) continue; - BuildLockset LocksetBuilder(this, *CurrBlockInfo); + BuildLockset LocksetBuilder(this, *CurrBlockInfo, Initial->EntrySet); // Visit all the statements in the basic block. for (const auto &BI : *CurrBlock) { @@ -2468,9 +2487,6 @@ } } - CFGBlockInfo *Initial = &BlockInfo[CFGraph->getEntry().getBlockID()]; - CFGBlockInfo *Final = &BlockInfo[CFGraph->getExit().getBlockID()]; - // Skip the final check if the exit block is unreachable. if (!Final->Reachable) return; 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 @@ -1971,6 +1971,9 @@ case POK_PtPassByRef: DiagID = diag::warn_pt_guarded_pass_by_reference; break; + case POK_ReturnByRef: + DiagID = diag::warn_guarded_return_by_reference; + break; } PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) << Kind << D @@ -2001,6 +2004,9 @@ case POK_PtPassByRef: DiagID = diag::warn_pt_guarded_pass_by_reference; break; + case POK_ReturnByRef: + DiagID = diag::warn_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 @@ -1,7 +1,7 @@ -// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=0 %s -// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=1 %s -// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 -Wthread-safety -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=0 %s -// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 -Wthread-safety -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=1 %s +// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-return -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=0 %s +// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -Wthread-safety -Wthread-safety-return -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=1 %s +// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 -Wthread-safety -Wthread-safety-return -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=0 %s +// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 -Wthread-safety -Wthread-safety-return -Wthread-safety-beta -Wno-thread-safety-negative -fcxx-exceptions -DUSE_CAPABILITY=1 %s // FIXME: should also run %clang_cc1 -fsyntax-only -verify -Wthread-safety -std=c++11 -Wc++98-compat %s // FIXME: should also run %clang_cc1 -fsyntax-only -verify -Wthread-safety %s @@ -5580,6 +5580,49 @@ } }; +class Return { + Mutex mu; + Foo foo 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_not_locked() { + return foo; // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}} + } + + Foo &returns_ref() { + 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; + } + + 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 ? + } +}; + } // end namespace PassByRefTest