Reapply "[clang analysis][thread-safety] Handle return-by-reference..… #68572
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Changes…… (#68394)" The new warnings are now under a separate flag
This reverts commit 859f2d0. Full diff: https://github.com/llvm/llvm-project/pull/68572.diff 6 Files Affected:
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/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 0b09c002191848a..0462919af660285 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1061,18 +1061,20 @@ def Most : DiagGroup<"most", [
]>;
// Thread Safety warnings
-def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">;
-def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">;
-def ThreadSafetyPrecise : DiagGroup<"thread-safety-precise">;
-def ThreadSafetyReference : DiagGroup<"thread-safety-reference">;
-def ThreadSafetyNegative : DiagGroup<"thread-safety-negative">;
+def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">;
+def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">;
+def ThreadSafetyPrecise : DiagGroup<"thread-safety-precise">;
+def ThreadSafetyReference : DiagGroup<"thread-safety-reference">;
+def ThreadSafetyReferenceReturn : DiagGroup<"thread-safety-reference-return">;
+def ThreadSafetyNegative : DiagGroup<"thread-safety-negative">;
def ThreadSafety : DiagGroup<"thread-safety",
[ThreadSafetyAttributes,
ThreadSafetyAnalysis,
ThreadSafetyPrecise,
ThreadSafetyReference]>;
def ThreadSafetyVerbose : DiagGroup<"thread-safety-verbose">;
-def ThreadSafetyBeta : DiagGroup<"thread-safety-beta">;
+def ThreadSafetyBeta : DiagGroup<"thread-safety-beta",
+ [ThreadSafetyReferenceReturn]>;
// Uniqueness Analysis warnings
def Consumed : DiagGroup<"consumed">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index b211680a0e9b6e9..c777d73dd4a769b 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<ThreadSafetyAnalysis>, 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<ThreadSafetyReference>, DefaultIgnore;
+def warn_guarded_return_by_reference : Warning<
+ "returning variable %1 by reference requires holding %0 "
+ "%select{'%2'|'%2' exclusively}3">,
+ InGroup<ThreadSafetyReferenceReturn>, 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<ThreadSafetyReferenceReturn>, 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 &Handler;
- const CXXMethodDecl *CurrentMethod = nullptr;
+ const FunctionDecl *CurrentFunction;
LocalVariableMap LocalVarMap;
FactManager FactMan;
std::vector<CFGBlockInfo> 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<til::Project>(SExp)) {
- if (!CurrentMethod)
+ if (!isa_and_nonnull<CXXMethodDecl>(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<BuildLockset> {
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<const Expr *, til::LiteralPtr *> ConstructedObjects;
LocalVariableMap::Context LVarCtx;
@@ -1566,9 +1568,11 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
bool SkipFirstParam = false);
public:
- BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo &Info)
+ BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo &Info,
+ const FactSet &FunctionExitFSet)
: ConstStmtVisitor<BuildLockset>(), 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<BuildLockset> {
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 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())
@@ -2142,6 +2149,25 @@ 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.
///
@@ -2251,8 +2277,7 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
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,7 +2303,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;
@@ -2348,6 +2373,25 @@ 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<LockableFactEntry>(Lock, LK_Exclusive,
+ D->getLocation()));
+ for (const auto &Lock : SharedLocksAcquired)
+ ExpectedFunctionExitSet.addLock(
+ FactMan,
+ std::make_unique<LockableFactEntry>(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];
@@ -2407,7 +2451,7 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
if (!CurrBlockInfo->Reachable)
continue;
- BuildLockset LocksetBuilder(this, *CurrBlockInfo);
+ BuildLockset LocksetBuilder(this, *CurrBlockInfo, ExpectedFunctionExitSet);
// Visit all the statements in the basic block.
for (const auto &BI : *CurrBlock) {
@@ -2483,24 +2527,8 @@ 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<LockableFactEntry>(
- Lock, LK_Exclusive, D->getLocation()));
- for (const auto &Lock : SharedLocksAcquired)
- ExpectedExitSet.addLock(FactMan, std::make_unique<LockableFactEntry>(
- 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(ExpectedExitSet, Final.ExitSet, Final.ExitLoc,
+ intersectAndWarn(ExpectedFunctionExitSet, 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 77bb560eb6288f7..0947e8b0f526a3b 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -1983,6 +1983,12 @@ 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
@@ -2013,6 +2019,12 @@ 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 8e312e589d81160..205cfa284f6c9c9 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -5580,6 +5580,85 @@ 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
|
|
@aeubanks FYI |
aeubanks
reviewed
Oct 13, 2023
| def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">; | ||
| def ThreadSafetyPrecise : DiagGroup<"thread-safety-precise">; | ||
| def ThreadSafetyReference : DiagGroup<"thread-safety-reference">; | ||
| def ThreadSafetyReferenceReturn : DiagGroup<"thread-safety-reference-return">; |
There was a problem hiding this comment.
it's fine to put this in ThreadSafety and have it on by default (which it should be because it's improving thread safety analysis) since now people can turn it off with -Wno-thread-safety-reference while cleaning up their codebase
legrosbuffle
force-pushed
the
ts-return
branch
3 times, most recently
from
2d90dc0 to
db86804
Compare
aeubanks
approved these changes
Oct 17, 2023
aaronpuchert
approved these changes
Oct 17, 2023
Comment on lines
1064
to
1066
| def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">; | ||
| def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">; | ||
| def ThreadSafetyPrecise : DiagGroup<"thread-safety-precise">; |
There was a problem hiding this comment.
I would suggest to undo the indentation changes here, as the flag is intended to be temporary.
clang/lib/Analysis/ThreadSafety.cpp
Outdated
| @@ -2278,7 +2303,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()]; | |||
There was a problem hiding this comment.
This change seems unnecessary, but I don't mind either way.
llvm#68394)" The new warnings are now under a separate flag `-Wthread-safety-reference-return`, which is on by default under `-Wthread-safety-reference`. - People can opt out via `-Wthread-safety-reference -Wnothread-safety-reference-return`. This reverts commit 859f2d0.
legrosbuffle
force-pushed
the
ts-return
branch
from
db86804 to
f3b0c97
Compare
aarongable
pushed a commit
to chromium/chromium
that referenced
this pull request
Oct 23, 2023
…return llvm/llvm-project#68572 refined the new functionality under a flag. Bug: 1490607 Change-Id: Ib2b47dc5a825e189dc4f20a242e64f2501dfe03b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4966766 Commit-Queue: Arthur Eubanks <[email protected]> Auto-Submit: Arthur Eubanks <[email protected]> Reviewed-by: Hans Wennborg <[email protected]> Cr-Commit-Position: refs/heads/main@{#1213677}
UI-RayanWang
pushed a commit
to ubiquiti/ubnt_libjingle_component_src_build
that referenced
this pull request
Dec 6, 2023
…return llvm/llvm-project#68572 refined the new functionality under a flag. Bug: 1490607 Change-Id: Ib2b47dc5a825e189dc4f20a242e64f2501dfe03b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4966766 Commit-Queue: Arthur Eubanks <[email protected]> Auto-Submit: Arthur Eubanks <[email protected]> Reviewed-by: Hans Wennborg <[email protected]> Cr-Commit-Position: refs/heads/main@{#1213677} NOKEYCHECK=True GitOrigin-RevId: 9f40a21c4abea603d74a445e37044c07c157b316
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…… (#68394)"
The new warnings are now under a separate flag
-Wthread-safety-reference-return, which is on by default under-Wthread-safety-reference.-Wthread-safety-reference -Wnothread-safety-reference-return.This reverts commit 859f2d0.