Skip to content

Commit 462bdd5

Browse files
legrosbufflelegrosbuffle
committed
Revert "[clang analysis][thread-safety] Handle return-by-reference... (#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.
1 parent 6dd96d6 commit 462bdd5

File tree

5 files changed

+28
-161
lines changed

5 files changed

+28
-161
lines changed

‎clang/include/clang/Analysis/Analyses/ThreadSafety.h

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -47,13 +47,7 @@ enum ProtectedOperationKind {
4747
POK_PassByRef,
4848

4949
/// Passing a pt-guarded variable by reference.
50-
POK_PtPassByRef,
51-
52-
/// Returning a guarded variable by reference.
53-
POK_ReturnByRef,
54-
55-
/// Returning a pt-guarded variable by reference.
56-
POK_PtReturnByRef,
50+
POK_PtPassByRef
5751
};
5852

5953
/// This enum distinguishes between different kinds of lock actions. For

‎clang/include/clang/Basic/DiagnosticSemaKinds.td

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3858,7 +3858,7 @@ def warn_fun_requires_negative_cap : Warning<
38583858
"calling function %0 requires negative capability '%1'">,
38593859
InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
38603860

3861-
// Thread safety warnings on pass/return by reference
3861+
// Thread safety warnings on pass by reference
38623862
def warn_guarded_pass_by_reference : Warning<
38633863
"passing variable %1 by reference requires holding %0 "
38643864
"%select{'%2'|'%2' exclusively}3">,
@@ -3867,14 +3867,6 @@ def warn_pt_guarded_pass_by_reference : Warning<
38673867
"passing the value that %1 points to by reference requires holding %0 "
38683868
"%select{'%2'|'%2' exclusively}3">,
38693869
InGroup<ThreadSafetyReference>, DefaultIgnore;
3870-
def warn_guarded_return_by_reference : Warning<
3871-
"returning variable %1 by reference requires holding %0 "
3872-
"%select{'%2'|'%2' exclusively}3">,
3873-
InGroup<ThreadSafetyReference>, DefaultIgnore;
3874-
def warn_pt_guarded_return_by_reference : Warning<
3875-
"returning the value that %1 points to by reference requires holding %0 "
3876-
"%select{'%2'|'%2' exclusively}3">,
3877-
InGroup<ThreadSafetyReference>, DefaultIgnore;
38783870

38793871
// Imprecise thread safety warnings
38803872
def warn_variable_requires_lock : Warning<

‎clang/lib/Analysis/ThreadSafety.cpp

Lines changed: 26 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -1008,7 +1008,7 @@ class ThreadSafetyAnalyzer {
10081008
threadSafety::SExprBuilder SxBuilder;
10091009

10101010
ThreadSafetyHandler &Handler;
1011-
const FunctionDecl *CurrentFunction;
1011+
const CXXMethodDecl *CurrentMethod = nullptr;
10121012
LocalVariableMap LocalVarMap;
10131013
FactManager FactMan;
10141014
std::vector<CFGBlockInfo> BlockInfo;
@@ -1243,10 +1243,10 @@ bool ThreadSafetyAnalyzer::inCurrentScope(const CapabilityExpr &CapE) {
12431243

12441244
// Members are in scope from methods of the same class.
12451245
if (const auto *P = dyn_cast<til::Project>(SExp)) {
1246-
if (!isa_and_nonnull<CXXMethodDecl>(CurrentFunction))
1246+
if (!CurrentMethod)
12471247
return false;
12481248
const ValueDecl *VD = P->clangDecl();
1249-
return VD->getDeclContext() == CurrentFunction->getDeclContext();
1249+
return VD->getDeclContext() == CurrentMethod->getDeclContext();
12501250
}
12511251

12521252
return false;
@@ -1541,8 +1541,6 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
15411541

15421542
ThreadSafetyAnalyzer *Analyzer;
15431543
FactSet FSet;
1544-
// The fact set for the function on exit.
1545-
const FactSet &FunctionExitFSet;
15461544
/// Maps constructed objects to `this` placeholder prior to initialization.
15471545
llvm::SmallDenseMap<const Expr *, til::LiteralPtr *> ConstructedObjects;
15481546
LocalVariableMap::Context LVarCtx;
@@ -1568,11 +1566,9 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
15681566
bool SkipFirstParam = false);
15691567

15701568
public:
1571-
BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo &Info,
1572-
const FactSet &FunctionExitFSet)
1569+
BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo &Info)
15731570
: ConstStmtVisitor<BuildLockset>(), Analyzer(Anlzr), FSet(Info.EntrySet),
1574-
FunctionExitFSet(FunctionExitFSet), LVarCtx(Info.EntryContext),
1575-
CtxIndex(Info.EntryIndex) {}
1571+
LVarCtx(Info.EntryContext), CtxIndex(Info.EntryIndex) {}
15761572

15771573
void VisitUnaryOperator(const UnaryOperator *UO);
15781574
void VisitBinaryOperator(const BinaryOperator *BO);
@@ -1581,7 +1577,6 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
15811577
void VisitCXXConstructExpr(const CXXConstructExpr *Exp);
15821578
void VisitDeclStmt(const DeclStmt *S);
15831579
void VisitMaterializeTemporaryExpr(const MaterializeTemporaryExpr *Exp);
1584-
void VisitReturnStmt(const ReturnStmt *S);
15851580
};
15861581

15871582
} // namespace
@@ -1763,8 +1758,6 @@ void ThreadSafetyAnalyzer::checkPtAccess(const FactSet &FSet, const Expr *Exp,
17631758
// Pass by reference warnings are under a different flag.
17641759
ProtectedOperationKind PtPOK = POK_VarDereference;
17651760
if (POK == POK_PassByRef) PtPOK = POK_PtPassByRef;
1766-
if (POK == POK_ReturnByRef)
1767-
PtPOK = POK_PtReturnByRef;
17681761

17691762
const ValueDecl *D = getValueDecl(Exp);
17701763
if (!D || !D->hasAttrs())
@@ -2149,25 +2142,6 @@ void BuildLockset::VisitMaterializeTemporaryExpr(
21492142
}
21502143
}
21512144

2152-
void BuildLockset::VisitReturnStmt(const ReturnStmt *S) {
2153-
if (Analyzer->CurrentFunction == nullptr)
2154-
return;
2155-
const Expr *RetVal = S->getRetValue();
2156-
if (!RetVal)
2157-
return;
2158-
2159-
// If returning by reference, check that the function requires the appropriate
2160-
// capabilities.
2161-
const QualType ReturnType =
2162-
Analyzer->CurrentFunction->getReturnType().getCanonicalType();
2163-
if (ReturnType->isLValueReferenceType()) {
2164-
Analyzer->checkAccess(
2165-
FunctionExitFSet, RetVal,
2166-
ReturnType->getPointeeType().isConstQualified() ? AK_Read : AK_Written,
2167-
POK_ReturnByRef);
2168-
}
2169-
}
2170-
21712145
/// Given two facts merging on a join point, possibly warn and decide whether to
21722146
/// keep or replace.
21732147
///
@@ -2277,7 +2251,8 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
22772251

22782252
CFG *CFGraph = walker.getGraph();
22792253
const NamedDecl *D = walker.getDecl();
2280-
CurrentFunction = dyn_cast<FunctionDecl>(D);
2254+
const auto *CurrentFunction = dyn_cast<FunctionDecl>(D);
2255+
CurrentMethod = dyn_cast<CXXMethodDecl>(D);
22812256

22822257
if (D->hasAttr<NoThreadSafetyAnalysisAttr>())
22832258
return;
@@ -2303,7 +2278,7 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
23032278
PostOrderCFGView::CFGBlockSet VisitedBlocks(CFGraph);
23042279

23052280
CFGBlockInfo &Initial = BlockInfo[CFGraph->getEntry().getBlockID()];
2306-
CFGBlockInfo &Final = BlockInfo[CFGraph->getExit().getBlockID()];
2281+
CFGBlockInfo &Final = BlockInfo[CFGraph->getExit().getBlockID()];
23072282

23082283
// Mark entry block as reachable
23092284
Initial.Reachable = true;
@@ -2373,25 +2348,6 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
23732348
}
23742349
}
23752350

2376-
// Compute the expected exit set.
2377-
// By default, we expect all locks held on entry to be held on exit.
2378-
FactSet ExpectedFunctionExitSet = Initial.EntrySet;
2379-
2380-
// Adjust the expected exit set by adding or removing locks, as declared
2381-
// by *-LOCK_FUNCTION and UNLOCK_FUNCTION. The intersect below will then
2382-
// issue the appropriate warning.
2383-
// FIXME: the location here is not quite right.
2384-
for (const auto &Lock : ExclusiveLocksAcquired)
2385-
ExpectedFunctionExitSet.addLock(
2386-
FactMan, std::make_unique<LockableFactEntry>(Lock, LK_Exclusive,
2387-
D->getLocation()));
2388-
for (const auto &Lock : SharedLocksAcquired)
2389-
ExpectedFunctionExitSet.addLock(
2390-
FactMan,
2391-
std::make_unique<LockableFactEntry>(Lock, LK_Shared, D->getLocation()));
2392-
for (const auto &Lock : LocksReleased)
2393-
ExpectedFunctionExitSet.removeLock(FactMan, Lock);
2394-
23952351
for (const auto *CurrBlock : *SortedGraph) {
23962352
unsigned CurrBlockID = CurrBlock->getBlockID();
23972353
CFGBlockInfo *CurrBlockInfo = &BlockInfo[CurrBlockID];
@@ -2451,7 +2407,7 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
24512407
if (!CurrBlockInfo->Reachable)
24522408
continue;
24532409

2454-
BuildLockset LocksetBuilder(this, *CurrBlockInfo, ExpectedFunctionExitSet);
2410+
BuildLockset LocksetBuilder(this, *CurrBlockInfo);
24552411

24562412
// Visit all the statements in the basic block.
24572413
for (const auto &BI : *CurrBlock) {
@@ -2527,8 +2483,24 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
25272483
if (!Final.Reachable)
25282484
return;
25292485

2486+
// By default, we expect all locks held on entry to be held on exit.
2487+
FactSet ExpectedExitSet = Initial.EntrySet;
2488+
2489+
// Adjust the expected exit set by adding or removing locks, as declared
2490+
// by *-LOCK_FUNCTION and UNLOCK_FUNCTION. The intersect below will then
2491+
// issue the appropriate warning.
2492+
// FIXME: the location here is not quite right.
2493+
for (const auto &Lock : ExclusiveLocksAcquired)
2494+
ExpectedExitSet.addLock(FactMan, std::make_unique<LockableFactEntry>(
2495+
Lock, LK_Exclusive, D->getLocation()));
2496+
for (const auto &Lock : SharedLocksAcquired)
2497+
ExpectedExitSet.addLock(FactMan, std::make_unique<LockableFactEntry>(
2498+
Lock, LK_Shared, D->getLocation()));
2499+
for (const auto &Lock : LocksReleased)
2500+
ExpectedExitSet.removeLock(FactMan, Lock);
2501+
25302502
// FIXME: Should we call this function for all blocks which exit the function?
2531-
intersectAndWarn(ExpectedFunctionExitSet, Final.ExitSet, Final.ExitLoc,
2503+
intersectAndWarn(ExpectedExitSet, Final.ExitSet, Final.ExitLoc,
25322504
LEK_LockedAtEndOfFunction, LEK_NotLockedAtEndOfFunction);
25332505

25342506
Handler.leaveFunction(CurrentFunction);

‎clang/lib/Sema/AnalysisBasedWarnings.cpp

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1983,12 +1983,6 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
19831983
case POK_PtPassByRef:
19841984
DiagID = diag::warn_pt_guarded_pass_by_reference;
19851985
break;
1986-
case POK_ReturnByRef:
1987-
DiagID = diag::warn_guarded_return_by_reference;
1988-
break;
1989-
case POK_PtReturnByRef:
1990-
DiagID = diag::warn_pt_guarded_return_by_reference;
1991-
break;
19921986
}
19931987
PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) << Kind
19941988
<< D
@@ -2019,12 +2013,6 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
20192013
case POK_PtPassByRef:
20202014
DiagID = diag::warn_pt_guarded_pass_by_reference;
20212015
break;
2022-
case POK_ReturnByRef:
2023-
DiagID = diag::warn_guarded_return_by_reference;
2024-
break;
2025-
case POK_PtReturnByRef:
2026-
DiagID = diag::warn_pt_guarded_return_by_reference;
2027-
break;
20282016
}
20292017
PartialDiagnosticAt Warning(Loc, S.PDiag(DiagID) << Kind
20302018
<< D

‎clang/test/SemaCXX/warn-thread-safety-analysis.cpp

Lines changed: 0 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -5580,85 +5580,6 @@ class Bar {
55805580
}
55815581
};
55825582

5583-
class Return {
5584-
Mutex mu;
5585-
Foo foo GUARDED_BY(mu);
5586-
Foo* foo_ptr PT_GUARDED_BY(mu);
5587-
5588-
Foo returns_value_locked() {
5589-
MutexLock lock(&mu);
5590-
return foo;
5591-
}
5592-
5593-
Foo returns_value_locks_required() EXCLUSIVE_LOCKS_REQUIRED(mu) {
5594-
return foo;
5595-
}
5596-
5597-
Foo returns_value_releases_lock_after_return() UNLOCK_FUNCTION(mu) {
5598-
MutexLock lock(&mu, true);
5599-
return foo;
5600-
}
5601-
5602-
Foo returns_value_aquires_lock() EXCLUSIVE_LOCK_FUNCTION(mu) {
5603-
mu.Lock();
5604-
return foo;
5605-
}
5606-
5607-
Foo returns_value_not_locked() {
5608-
return foo; // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}}
5609-
}
5610-
5611-
Foo returns_value_releases_lock_before_return() UNLOCK_FUNCTION(mu) {
5612-
mu.Unlock();
5613-
return foo; // expected-warning {{reading variable 'foo' requires holding mutex 'mu'}}
5614-
}
5615-
5616-
Foo &returns_ref_not_locked() {
5617-
return foo; // expected-warning {{returning variable 'foo' by reference requires holding mutex 'mu'}}
5618-
}
5619-
5620-
Foo &returns_ref_locked() {
5621-
MutexLock lock(&mu);
5622-
return foo; // expected-warning {{returning variable 'foo' by reference requires holding mutex 'mu'}}
5623-
}
5624-
5625-
Foo &returns_ref_shared_locks_required() SHARED_LOCKS_REQUIRED(mu) {
5626-
return foo; // expected-warning {{returning variable 'foo' by reference requires holding mutex 'mu' exclusively}}
5627-
}
5628-
5629-
Foo &returns_ref_exclusive_locks_required() EXCLUSIVE_LOCKS_REQUIRED(mu) {
5630-
return foo;
5631-
}
5632-
5633-
Foo &returns_ref_releases_lock_after_return() UNLOCK_FUNCTION(mu) {
5634-
MutexLock lock(&mu, true);
5635-
return foo; // expected-warning {{returning variable 'foo' by reference requires holding mutex 'mu' exclusively}}
5636-
}
5637-
5638-
Foo& returns_ref_releases_lock_before_return() UNLOCK_FUNCTION(mu) {
5639-
mu.Unlock();
5640-
return foo; // // expected-warning {{returning variable 'foo' by reference requires holding mutex 'mu' exclusively}}
5641-
}
5642-
5643-
Foo &returns_ref_aquires_lock() EXCLUSIVE_LOCK_FUNCTION(mu) {
5644-
mu.Lock();
5645-
return foo;
5646-
}
5647-
5648-
const Foo &returns_constref_shared_locks_required() SHARED_LOCKS_REQUIRED(mu) {
5649-
return foo;
5650-
}
5651-
5652-
Foo *returns_ptr() {
5653-
return &foo; // FIXME -- Do we want to warn on this ?
5654-
}
5655-
5656-
Foo &returns_ref2() {
5657-
return *foo_ptr; // expected-warning {{returning the value that 'foo_ptr' points to by reference requires holding mutex 'mu' exclusively}}
5658-
}
5659-
5660-
};
5661-
56625583

56635584
} // end namespace PassByRefTest
56645585

0 commit comments

Comments
 (0)