courbet updated this revision to Diff 532558.
courbet marked an inline comment as done.
courbet added a comment.
Address review comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153131/new/
https://reviews.llvm.org/D153131
Files:
clang/include/clang/Analysis/Analyses/ThreadSafety.h
clang/include/clang/Basic/DiagnosticGroups.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Analysis/ThreadSafety.cpp
clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/test/SemaCXX/warn-thread-safety-analysis.cpp
Index: clang/test/SemaCXX/warn-thread-safety-analysis.cpp
===================================================================
--- clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ 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
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===================================================================
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ 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
Index: clang/lib/Analysis/ThreadSafety.cpp
===================================================================
--- clang/lib/Analysis/ThreadSafety.cpp
+++ 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<CFGBlockInfo> BlockInfo;
@@ -1244,10 +1245,10 @@
// Members are in scope from methods of the same class.
if (const auto *P = dyn_cast<til::Project>(SExp)) {
- if (!CurrentMethod)
+ if (CurrentFunction == nullptr || !isa<CXXMethodDecl>(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<BuildLockset> {
- using VisitorBase = ConstStmtVisitor<BuildLockset>;
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<const Expr *, til::LiteralPtr *> 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<BuildLockset>(), 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<FunctionDecl>(D);
- CurrentMethod = dyn_cast<CXXMethodDecl>(D);
+ CurrentFunction = dyn_cast<FunctionDecl>(D);
if (D->hasAttr<NoThreadSafetyAnalysisAttr>())
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;
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3802,6 +3802,12 @@
"%select{'%2'|'%2' exclusively}3">,
InGroup<ThreadSafetyReference>, 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<ThreadSafetyReturn>, DefaultIgnore;
+
// Imprecise thread safety warnings
def warn_variable_requires_lock : Warning<
"%select{reading|writing}3 variable %1 requires holding %0 "
Index: clang/include/clang/Basic/DiagnosticGroups.td
===================================================================
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ 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,
Index: clang/include/clang/Analysis/Analyses/ThreadSafety.h
===================================================================
--- clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ 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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
