courbet created this revision.
courbet added a reviewer: delesley.
Herald added a reviewer: NoQ.
Herald added a project: All.
courbet requested review of this revision.
Herald added a project: clang.
...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 implemented as `-Wthread-safety-return` and not part of
`-Wthread-safety` for now.
Repository:
rG LLVM Github Monorepo
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,41 @@
}
};
+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_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"
@@ -1016,6 +1017,11 @@
BeforeSet *GlobalBeforeSet;
+ // The set of return values to check on exit.
+ llvm::SmallPtrSet<const Expr *, 16> ReturnValues;
+
+ void checkReturnValues(const FactSet &ExitSet);
+
void warnIfMutexNotHeld(const FactSet &FSet, const NamedDecl *D,
const Expr *Exp, AccessKind AK, Expr *MutexExp,
ProtectedOperationKind POK, til::LiteralPtr *Self,
@@ -1579,6 +1585,7 @@
void VisitCXXConstructExpr(const CXXConstructExpr *Exp);
void VisitDeclStmt(const DeclStmt *S);
void VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *Exp);
+ void VisitReturnStmt(const ReturnStmt *S);
};
} // namespace
@@ -2143,6 +2150,21 @@
}
}
+void BuildLockset::VisitReturnStmt(const ReturnStmt *S) {
+ const Expr *RetVal = S->getRetValue();
+ if (!RetVal) {
+ return;
+ }
+
+ // If returning by reference, add the return value to the set to check on
+ // function exit.
+ if (RetVal->isLValue()) {
+ Analyzer->ReturnValues.insert(RetVal);
+ }
+
+ VisitorBase::VisitReturnStmt(S);
+}
+
/// Given two facts merging on a join point, possibly warn and decide whether to
/// keep or replace.
///
@@ -2495,9 +2517,18 @@
intersectAndWarn(ExpectedExitSet, Final->ExitSet, Final->ExitLoc,
LEK_LockedAtEndOfFunction, LEK_NotLockedAtEndOfFunction);
+ // Check return values.
+ checkReturnValues(ExpectedExitSet);
+
Handler.leaveFunction(CurrentFunction);
}
+void ThreadSafetyAnalyzer::checkReturnValues(const FactSet &ExitSet) {
+ for (const Expr *RetVal : ReturnValues) {
+ checkAccess(ExitSet, RetVal, AK_Read, POK_ReturnByRef);
+ }
+}
+
/// Check a function's CFG for thread-safety violations.
///
/// We traverse the blocks in the CFG, compute the set of mutexes that are held
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
