Skip to content

Commit

Permalink
Revert "[clang analysis][thread-safety] Handle return-by-reference... (…
Browse files Browse the repository at this point in the history 
…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.
  • Loading branch information
@legrosbuffle
legrosbuffle committed Sep 29, 2023
1 parent 6dd96d6 commit 462bdd5
Show file tree
Hide file tree
Showing 5 changed files with 28 additions and 161 deletions.
8 changes: 1 addition & 7 deletions clang/include/clang/Analysis/Analyses/ThreadSafety.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 1 addition & 9 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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">,
Expand All @@ -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<
Expand Down
80 changes: 26 additions & 54 deletions clang/lib/Analysis/ThreadSafety.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand All @@ -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
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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.
///
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down
12 changes: 0 additions & 12 deletions clang/lib/Sema/AnalysisBasedWarnings.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
79 changes: 0 additions & 79 deletions clang/test/SemaCXX/warn-thread-safety-analysis.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down

0 comments on commit 462bdd5

Please sign in to comment.