diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h index 0866b09bab299..1808d1d71e05d 100644 --- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h +++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h @@ -47,13 +47,7 @@ enum ProtectedOperationKind { POK_PassByRef, /// Passing a pt-guarded variable by reference. - POK_PtPassByRef, - - /// Returning a guarded variable by reference. - POK_ReturnByRef, - - /// Returning a pt-guarded variable by reference. - POK_PtReturnByRef, + POK_PtPassByRef }; /// 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 index 39e6bc5a73e12..29362df683653 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -3858,7 +3858,7 @@ def warn_fun_requires_negative_cap : Warning< "calling function %0 requires negative capability '%1'">, InGroup, DefaultIgnore; -// Thread safety warnings on pass/return by reference +// Thread safety warnings on pass by reference def warn_guarded_pass_by_reference : Warning< "passing variable %1 by reference requires holding %0 " "%select{'%2'|'%2' exclusively}3">, @@ -3867,14 +3867,6 @@ def warn_pt_guarded_pass_by_reference : Warning< "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 index 54d0e95c6bd79..58dd7113665b1 100644 --- a/clang/lib/Analysis/ThreadSafety.cpp +++ b/clang/lib/Analysis/ThreadSafety.cpp @@ -1008,7 +1008,7 @@ class ThreadSafetyAnalyzer { threadSafety::SExprBuilder SxBuilder; ThreadSafetyHandler &Handler; - const FunctionDecl *CurrentFunction; + const CXXMethodDecl *CurrentMethod = nullptr; LocalVariableMap LocalVarMap; FactManager FactMan; std::vector BlockInfo; @@ -1243,10 +1243,10 @@ bool ThreadSafetyAnalyzer::inCurrentScope(const CapabilityExpr &CapE) { // Members are in scope from methods of the same class. if (const auto *P = dyn_cast(SExp)) { - if (!isa_and_nonnull(CurrentFunction)) + if (!CurrentMethod) return false; const ValueDecl *VD = P->clangDecl(); - return VD->getDeclContext() == CurrentFunction->getDeclContext(); + return VD->getDeclContext() == CurrentMethod->getDeclContext(); } return false; @@ -1541,8 +1541,6 @@ class BuildLockset : public ConstStmtVisitor { 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; @@ -1568,11 +1566,9 @@ class BuildLockset : public ConstStmtVisitor { bool SkipFirstParam = false); public: - BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo &Info, - const FactSet &FunctionExitFSet) + BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo &Info) : ConstStmtVisitor(), Analyzer(Anlzr), FSet(Info.EntrySet), - FunctionExitFSet(FunctionExitFSet), LVarCtx(Info.EntryContext), - CtxIndex(Info.EntryIndex) {} + LVarCtx(Info.EntryContext), CtxIndex(Info.EntryIndex) {} void VisitUnaryOperator(const UnaryOperator *UO); void VisitBinaryOperator(const BinaryOperator *BO); @@ -1581,7 +1577,6 @@ class BuildLockset : public ConstStmtVisitor { void VisitCXXConstructExpr(const CXXConstructExpr *Exp); void VisitDeclStmt(const DeclStmt *S); void VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *Exp); - void VisitReturnStmt(const ReturnStmt *S); }; } // namespace @@ -1763,8 +1758,6 @@ void ThreadSafetyAnalyzer::checkPtAccess(const FactSet &FSet, const Expr *Exp, // 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()) @@ -2149,25 +2142,6 @@ void BuildLockset::VisitMaterializeTemporaryExpr( } } -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. /// @@ -2277,7 +2251,8 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { CFG *CFGraph = walker.getGraph(); const NamedDecl *D = walker.getDecl(); - CurrentFunction = dyn_cast(D); + const auto *CurrentFunction = dyn_cast(D); + CurrentMethod = dyn_cast(D); if (D->hasAttr()) return; @@ -2303,7 +2278,7 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { 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; @@ -2373,25 +2348,6 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { } } - // 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]; @@ -2451,7 +2407,7 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { if (!CurrBlockInfo->Reachable) continue; - BuildLockset LocksetBuilder(this, *CurrBlockInfo, ExpectedFunctionExitSet); + BuildLockset LocksetBuilder(this, *CurrBlockInfo); // Visit all the statements in the basic block. for (const auto &BI : *CurrBlock) { @@ -2527,8 +2483,24 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) { 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(ExpectedFunctionExitSet, Final.ExitSet, Final.ExitLoc, + intersectAndWarn(ExpectedExitSet, 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 index 0947e8b0f526a..77bb560eb6288 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -1983,12 +1983,6 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { 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 @@ -2019,12 +2013,6 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler { 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 index 205cfa284f6c9..8e312e589d811 100644 --- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -5580,85 +5580,6 @@ class Bar { } }; -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