[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68394)
https://github.com/legrosbuffle closed https://github.com/llvm/llvm-project/pull/68394 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68394)
llvmbot wrote:
@llvm/pr-subscribers-clang-analysis
Changes
…. (#67776)"
Now that `scudo` issues have been fixed (#68273).
This reverts commit 462bdd5bf0861a27f451f7917802a954e2046bc7.
---
Full diff: https://github.com/llvm/llvm-project/pull/68394.diff
5 Files Affected:
- (modified) clang/include/clang/Analysis/Analyses/ThreadSafety.h (+7-1)
- (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+9-1)
- (modified) clang/lib/Analysis/ThreadSafety.cpp (+54-26)
- (modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+12)
- (modified) clang/test/SemaCXX/warn-thread-safety-analysis.cpp (+79)
``diff
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 1808d1d71e05d2c..0866b09bab2995e 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -47,7 +47,13 @@ enum ProtectedOperationKind {
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
index 28f1db29e62fa91..e3cd49bcc9ecc6a 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3864,7 +3864,7 @@ def warn_fun_requires_negative_cap : Warning<
"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">,
@@ -3873,6 +3873,14 @@ 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 58dd7113665b132..54d0e95c6bd79a2 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1008,7 +1008,7 @@ class ThreadSafetyAnalyzer {
threadSafety::SExprBuilder SxBuilder;
ThreadSafetyHandler
- const CXXMethodDecl *CurrentMethod = nullptr;
+ const FunctionDecl *CurrentFunction;
LocalVariableMap LocalVarMap;
FactManager FactMan;
std::vector BlockInfo;
@@ -1243,10 +1243,10 @@ bool ThreadSafetyAnalyzer::inCurrentScope(const
CapabilityExpr ) {
// 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;
@@ -1541,6 +1541,8 @@ class BuildLockset : public
ConstStmtVisitor {
ThreadSafetyAnalyzer *Analyzer;
FactSet FSet;
+ // The fact set for the function on exit.
+ const FactSet
/// Maps constructed objects to `this` placeholder prior to initialization.
llvm::SmallDenseMap ConstructedObjects;
LocalVariableMap::Context LVarCtx;
@@ -1566,9 +1568,11 @@ class BuildLockset : public
ConstStmtVisitor {
bool SkipFirstParam = false);
public:
- BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo )
+ BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo ,
+ const FactSet )
: ConstStmtVisitor(), Analyzer(Anlzr), FSet(Info.EntrySet),
-LVarCtx(Info.EntryContext), CtxIndex(Info.EntryIndex) {}
+FunctionExitFSet(FunctionExitFSet), LVarCtx(Info.EntryContext),
+CtxIndex(Info.EntryIndex) {}
void VisitUnaryOperator(const UnaryOperator *UO);
void VisitBinaryOperator(const BinaryOperator *BO);
@@ -1577,6 +1581,7 @@ 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
@@ -1758,6 +1763,8 @@ void
[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68394)
https://github.com/legrosbuffle created
https://github.com/llvm/llvm-project/pull/68394
…. (#67776)"
Now that `scudo` issues have been fixed (#68273).
This reverts commit 462bdd5bf0861a27f451f7917802a954e2046bc7.
>From 6bf1af81adef93f20f25db4023810fe7be750410 Mon Sep 17 00:00:00 2001
From: Clement Courbet
Date: Fri, 29 Sep 2023 14:14:29 +0200
Subject: [PATCH] Reapply "[clang analysis][thread-safety] Handle
return-by-reference... (#67776)"
Now that `scudo` issues have been fixed (#68273).
This reverts commit 462bdd5bf0861a27f451f7917802a954e2046bc7.
---
.../clang/Analysis/Analyses/ThreadSafety.h| 8 +-
.../clang/Basic/DiagnosticSemaKinds.td| 10 ++-
clang/lib/Analysis/ThreadSafety.cpp | 80 +--
clang/lib/Sema/AnalysisBasedWarnings.cpp | 12 +++
.../SemaCXX/warn-thread-safety-analysis.cpp | 79 ++
5 files changed, 161 insertions(+), 28 deletions(-)
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 1808d1d71e05d2c..0866b09bab2995e 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -47,7 +47,13 @@ enum ProtectedOperationKind {
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
index 28f1db29e62fa91..e3cd49bcc9ecc6a 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3864,7 +3864,7 @@ def warn_fun_requires_negative_cap : Warning<
"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">,
@@ -3873,6 +3873,14 @@ 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 58dd7113665b132..54d0e95c6bd79a2 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1008,7 +1008,7 @@ class ThreadSafetyAnalyzer {
threadSafety::SExprBuilder SxBuilder;
ThreadSafetyHandler
- const CXXMethodDecl *CurrentMethod = nullptr;
+ const FunctionDecl *CurrentFunction;
LocalVariableMap LocalVarMap;
FactManager FactMan;
std::vector BlockInfo;
@@ -1243,10 +1243,10 @@ bool ThreadSafetyAnalyzer::inCurrentScope(const
CapabilityExpr ) {
// 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;
@@ -1541,6 +1541,8 @@ class BuildLockset : public
ConstStmtVisitor {
ThreadSafetyAnalyzer *Analyzer;
FactSet FSet;
+ // The fact set for the function on exit.
+ const FactSet
/// Maps constructed objects to `this` placeholder prior to initialization.
llvm::SmallDenseMap ConstructedObjects;
LocalVariableMap::Context LVarCtx;
@@ -1566,9 +1568,11 @@ class BuildLockset : public
ConstStmtVisitor {
bool SkipFirstParam = false);
public:
- BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo )
+ BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo ,
+ const FactSet )
: ConstStmtVisitor(), Analyzer(Anlzr), FSet(Info.EntrySet),
-LVarCtx(Info.EntryContext), CtxIndex(Info.EntryIndex) {}
+FunctionExitFSet(FunctionExitFSet), LVarCtx(Info.EntryContext),
+CtxIndex(Info.EntryIndex) {}
void VisitUnaryOperator(const UnaryOperator *UO);
void VisitBinaryOperator(const BinaryOperator *BO);
@@ -1577,6 +1581,7 @@ class BuildLockset : public
