[clang] [clang analysis][thread-safety] Handle return-by-reference... (PR #67428)
llvmbot wrote:
@llvm/pr-subscribers-clang
Changes
...of guarded variables, when the function is not marked as requiring locks:
```
class Return {
Mutex mu;
Foo foo GUARDED_BY(mu);
Foo returns_ref_locked() {
MutexLock lock(mu);
return foo; // BAD
}
Foo returns_ref_locks_required() SHARED_LOCKS_REQUIRED(mu) {
return foo; // OK
}
};
```
This is carried over from phabricator: https://reviews.llvm.org/D153131
---
Full diff: https://github.com/llvm/llvm-project/pull/67428.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 f4eb02fd9570c2f..8e423ea7691de88 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 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">,
@@ -3867,6 +3867,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 f160cf4d013c78d..77b12f750e18a45 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
[clang] [clang analysis][thread-safety] Handle return-by-reference... (PR #67428)
https://github.com/legrosbuffle created
https://github.com/llvm/llvm-project/pull/67428
...of guarded variables, when the function is not marked as requiring locks:
```
class Return {
Mutex mu;
Foo foo GUARDED_BY(mu);
Foo _ref_locked() {
MutexLock lock();
return foo; // BAD
}
Foo _ref_locks_required() SHARED_LOCKS_REQUIRED(mu) {
return foo; // OK
}
};
```
This is carried over from phabricator: https://reviews.llvm.org/D153131
>From c14b45843e0a72d96c4898fc7d7ee72bcd619988 Mon Sep 17 00:00:00 2001
From: Clement Courbet
Date: Tue, 26 Sep 2023 15:22:09 +0200
Subject: [PATCH] [clang analysis][thread-safety] Handle return-by-reference...
...of guarded variables, when the function is not marked as requiring locks:
```
class Return {
Mutex mu;
Foo foo GUARDED_BY(mu);
Foo _ref_locked() {
MutexLock lock();
return foo; // BAD
}
Foo _ref_locks_required() SHARED_LOCKS_REQUIRED(mu) {
return foo; // OK
}
};
```
This is carried over from phabricator: https://reviews.llvm.org/D153131
---
.../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 f4eb02fd9570c2f..8e423ea7691de88 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 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">,
@@ -3867,6 +3867,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 f160cf4d013c78d..77b12f750e18a45 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 )
+
