Revert "[clang analysis][thread-safety] Handle return-by-reference...… #67795
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
…llvm#67776)" This detects issues in `scudo`. Reverting until these are fixed. ``` /b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd.h:74:12: error: returning variable 'QuarantineCache' by reference requires holding mutex 'Mutex' exclusively [-Werror,-Wthread-safety-reference] 74 | return QuarantineCache; | ^ /b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/combined.h:248:28: note: in instantiation of member function 'scudo::TSD<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::getQuarantineCache' requested here 248 | Quarantine.drain(&TSD->getQuarantineCache(), | ^ /b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd.h:57:15: note: in instantiation of member function 'scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>::commitBack' requested here 57 | Instance->commitBack(this); | ^ /b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:172:27: note: in instantiation of member function 'scudo::TSD<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::commitBack' requested here 172 | TSDRegistryT::ThreadTSD.commitBack(Instance); | ^ /b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:33:46: note: in instantiation of function template specialization 'scudo::teardownThread<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>' requested here 33 | CHECK_EQ(pthread_key_create(&PThreadKey, teardownThread<Allocator>), 0); | ^ /b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:42:5: note: in instantiation of member function 'scudo::TSDRegistryExT<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::init' requested here 42 | init(Instance); // Sets Initialized. | ^ /b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:130:5: note: in instantiation of member function 'scudo::TSDRegistryExT<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::initOnceMaybe' requested here 130 | initOnceMaybe(Instance); | ^ /b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:74:5: note: in instantiation of member function 'scudo::TSDRegistryExT<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::initThread' requested here 74 | initThread(Instance, MinimalInit); | ^ /b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/combined.h:221:17: note: in instantiation of member function 'scudo::TSDRegistryExT<scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>>::initThreadMaybe' requested here 221 | TSDRegistry.initThreadMaybe(this, MinimalInit); | ^ /b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/combined.h:790:5: note: in instantiation of member function 'scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>::initThreadMaybe' requested here 790 | initThreadMaybe(); | ^ /b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/wrappers_c.inc:36:25: note: in instantiation of member function 'scudo::Allocator<scudo::DefaultConfig, &malloc_postinit>::canReturnNull' requested here 36 | if (SCUDO_ALLOCATOR.canReturnNull()) { ``` This reverts commit 6dd96d6.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Changes… (#67776)" This detects issues in This reverts commit 6dd96d6. Full diff: https://github.com/llvm/llvm-project/pull/67795.diff 5 Files Affected:
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 different kinds of lock actions. For
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 39e6bc5a73e128c..29362df68365350 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<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">,
@@ -3867,14 +3867,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 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())
@@ -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
|
You can test this locally with the following command:git-clang-format --diff b7663760a54f31b46f4c9b5608c2c0c20c98efb2 462bdd5bf0861a27f451f7917802a954e2046bc7 -- clang/include/clang/Analysis/Analyses/ThreadSafety.h clang/lib/Analysis/ThreadSafety.cpp clang/lib/Sema/AnalysisBasedWarnings.cpp clang/test/SemaCXX/warn-thread-safety-analysis.cppView the diff from clang-format here.diff --git a/clang/lib/Analysis/ThreadSafety.cpp b/clang/lib/Analysis/ThreadSafety.cpp
index 58dd71136..59d4a94a8 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1757,7 +1757,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_PassByRef)
+ PtPOK = POK_PtPassByRef;
const ValueDecl *D = getValueDecl(Exp);
if (!D || !D->hasAttrs())
@@ -2278,7 +2279,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;
|
legrosbuffle
added a commit
to legrosbuffle/llvm-project
that referenced
this pull request
Oct 3, 2023
This avoids new warnings from `-Wthread-safety` after llvm#67776, see llvm#67795. Note that this are not really useful since thread safety analysis is disabled anyway here: llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:172
ChiaHungDuan
added a commit
that referenced
this pull request
Oct 5, 2023
In getCache()/getQuarantineCache(), they return a reference to variable guarded by a mutex. After #67776, thread-safey analysis checks if a variable return by reference has the lock held. The ASSERT_CAPABILITY only claims after calling that function, the lock will be held. But not asserting that the lock is held *before* calling that function. In the patch, we switch to use REQUIRES() and assertLocked() to mark the code paths. Also remove the misused ASSERT_CAPABILITY. Fixes #67795, #67796
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.
… (#67776)"
This detects issues in
scudo. Reverting until these are fixed.This reverts commit 6dd96d6.