[clang] 859f2d0 - Revert "Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (#68394)"
Caroline Tice via cfe-commits
cfe-commits at lists.llvm.org
Fri Oct 6 22:32:24 PDT 2023
Author: Caroline Tice
Date: 2023-10-06T22:23:02-07:00
New Revision: 859f2d032386632562521a99db20923217d98988
URL: https://github.com/llvm/llvm-project/commit/859f2d032386632562521a99db20923217d98988
DIFF: https://github.com/llvm/llvm-project/commit/859f2d032386632562521a99db20923217d98988.diff
LOG: Revert "Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (#68394)"
This reverts commit cd184c866e0aad1f957910b8c7a94f98a2b21ceb.
Added:
Modified:
clang/include/clang/Analysis/Analyses/ThreadSafety.h
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Analysis/ThreadSafety.cpp
clang/lib/Sema/AnalysisBasedWarnings.cpp
clang/test/SemaCXX/warn-thread-safety-analysis.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 0866b09bab2995e..1808d1d71e05d2c 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -47,13 +47,7 @@ enum ProtectedOperationKind {
POK_PassByRef,
/// Passing a pt-guarded variable by reference.
- POK_PtPassByRef,
-
- /// Returning a guarded variable by reference.
- POK_ReturnByRef,
-
- /// Returning a pt-guarded variable by reference.
- POK_PtReturnByRef,
+ POK_PtPassByRef
};
/// This enum distinguishes between
diff erent kinds of lock actions. For
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index e3cd49bcc9ecc6a..28f1db29e62fa91 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/return by reference
+// Thread safety warnings on pass by reference
def warn_guarded_pass_by_reference : Warning<
"passing variable %1 by reference requires holding %0 "
"%select{'%2'|'%2' exclusively}3">,
@@ -3873,14 +3873,6 @@ 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<ThreadSafetyReference>, 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<ThreadSafetyReference>, 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 54d0e95c6bd79a2..58dd7113665b132 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 FunctionDecl *CurrentFunction;
+ const CXXMethodDecl *CurrentMethod = nullptr;
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 (!isa_and_nonnull<CXXMethodDecl>(CurrentFunction))
+ if (!CurrentMethod)
return false;
const ValueDecl *VD = P->clangDecl();
- return VD->getDeclContext() == CurrentFunction->getDeclContext();
+ return VD->getDeclContext() == CurrentMethod->getDeclContext();
}
return false;
@@ -1541,8 +1541,6 @@ 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;
@@ -1568,11 +1566,9 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
bool SkipFirstParam = false);
public:
- BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo &Info,
- const FactSet &FunctionExitFSet)
+ BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo &Info)
: ConstStmtVisitor<BuildLockset>(), Analyzer(Anlzr), FSet(Info.EntrySet),
- FunctionExitFSet(FunctionExitFSet), LVarCtx(Info.EntryContext),
- CtxIndex(Info.EntryIndex) {}
+ LVarCtx(Info.EntryContext), CtxIndex(Info.EntryIndex) {}
void VisitUnaryOperator(const UnaryOperator *UO);
void VisitBinaryOperator(const BinaryOperator *BO);
@@ -1581,7 +1577,6 @@ 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
@@ -1763,8 +1758,6 @@ void ThreadSafetyAnalyzer::checkPtAccess(const FactSet &FSet, const Expr *Exp,
// Pass by reference warnings are under a
diff erent 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())
@@ -2149,25 +2142,6 @@ 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.
///
@@ -2277,7 +2251,8 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
CFG *CFGraph = walker.getGraph();
const NamedDecl *D = walker.getDecl();
- CurrentFunction = dyn_cast<FunctionDecl>(D);
+ const auto *CurrentFunction = dyn_cast<FunctionDecl>(D);
+ CurrentMethod = dyn_cast<CXXMethodDecl>(D);
if (D->hasAttr<NoThreadSafetyAnalysisAttr>())
return;
@@ -2303,7 +2278,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;
@@ -2373,25 +2348,6 @@ 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];
@@ -2451,7 +2407,7 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
if (!CurrBlockInfo->Reachable)
continue;
- BuildLockset LocksetBuilder(this, *CurrBlockInfo, ExpectedFunctionExitSet);
+ BuildLockset LocksetBuilder(this, *CurrBlockInfo);
// Visit all the statements in the basic block.
for (const auto &BI : *CurrBlock) {
@@ -2527,8 +2483,24 @@ 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(ExpectedFunctionExitSet, Final.ExitSet, Final.ExitLoc,
+ intersectAndWarn(ExpectedExitSet, 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 0947e8b0f526a3b..77bb560eb6288f7 100644
--- a/clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -1983,12 +1983,6 @@ 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
@@ -2019,12 +2013,6 @@ 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 205cfa284f6c9c9..8e312e589d81160 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -5580,85 +5580,6 @@ 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
More information about the cfe-commits
mailing list