Skip to content

Commit bf34707

Browse files
malek203malek203
committed
Thread Safety Analysis: Check managed capabilities of returned scoped capability
Verify that the return value of type scoped lockable manages the mutexes expected by the function annotations.
1 parent cea00f0 commit bf34707

File tree

5 files changed

+160
-40
lines changed

5 files changed

+160
-40
lines changed

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

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -243,32 +243,37 @@ class ThreadSafetyHandler {
243243
/// \param Kind -- The kind of the expected mutex.
244244
/// \param Expected -- The name of the expected mutex.
245245
/// \param Actual -- The name of the actual mutex.
246+
/// \param ForParam -- Indicates whether the note applies to a function parameter.
246247
virtual void handleUnmatchedUnderlyingMutexes(SourceLocation Loc,
247248
SourceLocation DLoc,
248249
Name ScopeName, StringRef Kind,
249-
Name Expected, Name Actual) {}
250+
Name Expected, Name Actual,
251+
bool ForParam) {}
250252

251253
/// Warn when we get fewer underlying mutexes than expected.
252254
/// \param Loc -- The location of the call expression.
253255
/// \param DLoc -- The location of the function declaration.
254256
/// \param ScopeName -- The name of the scope passed to the function.
255257
/// \param Kind -- The kind of the expected mutex.
256258
/// \param Expected -- The name of the expected mutex.
259+
/// \param ForParam -- Indicates whether the note applies to a function parameter.
257260
virtual void handleExpectMoreUnderlyingMutexes(SourceLocation Loc,
258261
SourceLocation DLoc,
259262
Name ScopeName, StringRef Kind,
260-
Name Expected) {}
263+
Name Expected, bool ForParam) {}
261264

262265
/// Warn when we get more underlying mutexes than expected.
263266
/// \param Loc -- The location of the call expression.
264267
/// \param DLoc -- The location of the function declaration.
265268
/// \param ScopeName -- The name of the scope passed to the function.
266269
/// \param Kind -- The kind of the actual mutex.
267270
/// \param Actual -- The name of the actual mutex.
271+
/// \param ForParam -- Indicates whether the note applies to a function parameter.
268272
virtual void handleExpectFewerUnderlyingMutexes(SourceLocation Loc,
269273
SourceLocation DLoc,
270274
Name ScopeName,
271-
StringRef Kind, Name Actual) {}
275+
StringRef Kind, Name Actual,
276+
bool ForParam) {}
272277

273278
/// Warn that L1 cannot be acquired before L2.
274279
virtual void handleLockAcquiredBefore(StringRef Kind, Name L1Name,

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4117,8 +4117,8 @@ def warn_expect_more_underlying_mutexes : Warning<
41174117
def warn_expect_fewer_underlying_mutexes : Warning<
41184118
"did not expect %0 '%2' to be managed by '%1'">,
41194119
InGroup<ThreadSafetyAnalysis>, DefaultIgnore;
4120-
def note_managed_mismatch_here_for_param : Note<
4121-
"see attribute on parameter here">;
4120+
def note_managed_mismatch_here : Note<
4121+
"see attribute on %select{function|parameter}0 here">;
41224122

41234123

41244124
// Thread safety warnings negative capabilities

‎clang/lib/Analysis/ThreadSafety.cpp

Lines changed: 85 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1040,6 +1040,7 @@ class ThreadSafetyAnalyzer {
10401040
std::vector<CFGBlockInfo> BlockInfo;
10411041

10421042
BeforeSet *GlobalBeforeSet;
1043+
CapExprSet ExpectedReturnedCapabilities;
10431044

10441045
public:
10451046
ThreadSafetyAnalyzer(ThreadSafetyHandler &H, BeforeSet* Bset)
@@ -2041,15 +2042,16 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
20412042
if (!a.has_value()) {
20422043
Analyzer->Handler.handleExpectFewerUnderlyingMutexes(
20432044
Exp->getExprLoc(), D->getLocation(), Scope->toString(),
2044-
b.value().getKind(), b.value().toString());
2045+
b.value().getKind(), b.value().toString(), true);
20452046
} else if (!b.has_value()) {
20462047
Analyzer->Handler.handleExpectMoreUnderlyingMutexes(
20472048
Exp->getExprLoc(), D->getLocation(), Scope->toString(),
2048-
a.value().getKind(), a.value().toString());
2049+
a.value().getKind(), a.value().toString(), true);
20492050
} else if (!a.value().matches(b.value())) {
20502051
Analyzer->Handler.handleUnmatchedUnderlyingMutexes(
20512052
Exp->getExprLoc(), D->getLocation(), Scope->toString(),
2052-
a.value().getKind(), a.value().toString(), b.value().toString());
2053+
a.value().getKind(), a.value().toString(), b.value().toString(),
2054+
true);
20532055
break;
20542056
}
20552057
}
@@ -2294,6 +2296,25 @@ void BuildLockset::VisitMaterializeTemporaryExpr(
22942296
}
22952297
}
22962298

2299+
static bool checkRecordTypeForScopedCapability(QualType Ty) {
2300+
const RecordType *RT = Ty->getAs<RecordType>();
2301+
2302+
if (!RT)
2303+
return false;
2304+
2305+
if (RT->getDecl()->hasAttr<ScopedLockableAttr>())
2306+
return true;
2307+
2308+
// Else check if any base classes have the attribute.
2309+
if (const auto *CRD = dyn_cast<CXXRecordDecl>(RT->getDecl())) {
2310+
if (!CRD->forallBases([](const CXXRecordDecl *Base) {
2311+
return !Base->hasAttr<ScopedLockableAttr>();
2312+
}))
2313+
return true;
2314+
}
2315+
return false;
2316+
}
2317+
22972318
void BuildLockset::VisitReturnStmt(const ReturnStmt *S) {
22982319
if (Analyzer->CurrentFunction == nullptr)
22992320
return;
@@ -2316,6 +2337,49 @@ void BuildLockset::VisitReturnStmt(const ReturnStmt *S) {
23162337
ReturnType->getPointeeType().isConstQualified() ? AK_Read : AK_Written,
23172338
POK_ReturnPointer);
23182339
}
2340+
2341+
if (!checkRecordTypeForScopedCapability(ReturnType))
2342+
return;
2343+
2344+
if (const auto *CBTE = dyn_cast<ExprWithCleanups>(RetVal))
2345+
RetVal = CBTE->getSubExpr();
2346+
RetVal = RetVal->IgnoreCasts();
2347+
if (const auto *CBTE = dyn_cast<CXXBindTemporaryExpr>(RetVal))
2348+
RetVal = CBTE->getSubExpr();
2349+
CapabilityExpr Cp;
2350+
if (auto Object = Analyzer->ConstructedObjects.find(RetVal);
2351+
Object != Analyzer->ConstructedObjects.end()) {
2352+
Cp = CapabilityExpr(Object->second, StringRef(), false);
2353+
Analyzer->ConstructedObjects.erase(Object);
2354+
}
2355+
if (!Cp.shouldIgnore()) {
2356+
const FactEntry *Fact = FSet.findLock(Analyzer->FactMan, Cp);
2357+
if (const ScopedLockableFactEntry *Scope =
2358+
cast_or_null<ScopedLockableFactEntry>(Fact)) {
2359+
CapExprSet LocksInReturnVal = Scope->getUnderlyingMutexes();
2360+
for (const auto &[a, b] : zip_longest(
2361+
Analyzer->ExpectedReturnedCapabilities, LocksInReturnVal)) {
2362+
if (!a.has_value()) {
2363+
Analyzer->Handler.handleExpectFewerUnderlyingMutexes(
2364+
RetVal->getExprLoc(), Analyzer->CurrentFunction->getLocation(),
2365+
Scope->toString(), b.value().getKind(), b.value().toString(),
2366+
false);
2367+
} else if (!b.has_value()) {
2368+
Analyzer->Handler.handleExpectMoreUnderlyingMutexes(
2369+
RetVal->getExprLoc(), Analyzer->CurrentFunction->getLocation(),
2370+
Scope->toString(), a.value().getKind(), a.value().toString(),
2371+
false);
2372+
break;
2373+
} else if (!a.value().matches(b.value())) {
2374+
Analyzer->Handler.handleUnmatchedUnderlyingMutexes(
2375+
RetVal->getExprLoc(), Analyzer->CurrentFunction->getLocation(),
2376+
Scope->toString(), a.value().getKind(), a.value().toString(),
2377+
b.value().toString(), false);
2378+
break;
2379+
}
2380+
}
2381+
}
2382+
}
23192383
}
23202384

23212385
/// Given two facts merging on a join point, possibly warn and decide whether to
@@ -2480,11 +2544,22 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
24802544
CapExprSet SharedLocksToAdd;
24812545

24822546
SourceLocation Loc = D->getLocation();
2547+
bool ReturnsScopedCapability;
2548+
if (CurrentFunction)
2549+
ReturnsScopedCapability = checkRecordTypeForScopedCapability(
2550+
CurrentFunction->getReturnType().getCanonicalType());
2551+
else if (auto CurrentMethod = dyn_cast<ObjCMethodDecl>(D))
2552+
ReturnsScopedCapability = checkRecordTypeForScopedCapability(
2553+
CurrentMethod->getReturnType().getCanonicalType());
2554+
else
2555+
llvm_unreachable("Unknown function kind");
24832556
for (const auto *Attr : D->attrs()) {
24842557
Loc = Attr->getLocation();
24852558
if (const auto *A = dyn_cast<RequiresCapabilityAttr>(Attr)) {
24862559
getMutexIDs(A->isShared() ? SharedLocksToAdd : ExclusiveLocksToAdd, A,
24872560
nullptr, D);
2561+
if (ReturnsScopedCapability)
2562+
getMutexIDs(ExpectedReturnedCapabilities, A, nullptr, D);
24882563
} else if (const auto *A = dyn_cast<ReleaseCapabilityAttr>(Attr)) {
24892564
// UNLOCK_FUNCTION() is used to hide the underlying lock implementation.
24902565
// We must ignore such methods.
@@ -2493,12 +2568,19 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
24932568
getMutexIDs(A->isShared() ? SharedLocksToAdd : ExclusiveLocksToAdd, A,
24942569
nullptr, D);
24952570
getMutexIDs(LocksReleased, A, nullptr, D);
2571+
if (ReturnsScopedCapability)
2572+
getMutexIDs(ExpectedReturnedCapabilities, A, nullptr, D);
24962573
} else if (const auto *A = dyn_cast<AcquireCapabilityAttr>(Attr)) {
24972574
if (A->args_size() == 0)
24982575
return;
24992576
getMutexIDs(A->isShared() ? SharedLocksAcquired
25002577
: ExclusiveLocksAcquired,
25012578
A, nullptr, D);
2579+
if (ReturnsScopedCapability)
2580+
getMutexIDs(ExpectedReturnedCapabilities, A, nullptr, D);
2581+
} else if (const auto *A = dyn_cast<LocksExcludedAttr>(Attr)) {
2582+
if (ReturnsScopedCapability)
2583+
getMutexIDs(ExpectedReturnedCapabilities, A, nullptr, D);
25022584
} else if (isa<ExclusiveTrylockFunctionAttr>(Attr)) {
25032585
// Don't try to check trylock functions for now.
25042586
return;

‎clang/lib/Sema/AnalysisBasedWarnings.cpp

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1799,10 +1799,11 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
17991799
: getNotes();
18001800
}
18011801

1802-
OptionalNotes makeManagedMismatchNoteForParam(SourceLocation DeclLoc) {
1802+
OptionalNotes makeManagedMismatchNote(SourceLocation DeclLoc, bool forParam) {
18031803
return DeclLoc.isValid()
18041804
? getNotes(PartialDiagnosticAt(
1805-
DeclLoc, S.PDiag(diag::note_managed_mismatch_here_for_param)))
1805+
DeclLoc, S.PDiag(diag::note_managed_mismatch_here)
1806+
<< forParam))
18061807
: getNotes();
18071808
}
18081809

@@ -1828,34 +1829,35 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
18281829

18291830
void handleUnmatchedUnderlyingMutexes(SourceLocation Loc, SourceLocation DLoc,
18301831
Name scopeName, StringRef Kind,
1831-
Name expected, Name actual) override {
1832+
Name expected, Name actual,
1833+
bool forParam) override {
18321834
PartialDiagnosticAt Warning(Loc,
18331835
S.PDiag(diag::warn_unmatched_underlying_mutexes)
18341836
<< Kind << scopeName << expected << actual);
18351837
Warnings.emplace_back(std::move(Warning),
1836-
makeManagedMismatchNoteForParam(DLoc));
1838+
makeManagedMismatchNote(DLoc, forParam));
18371839
}
18381840

18391841
void handleExpectMoreUnderlyingMutexes(SourceLocation Loc,
18401842
SourceLocation DLoc, Name scopeName,
1841-
StringRef Kind,
1842-
Name expected) override {
1843+
StringRef Kind, Name expected,
1844+
bool forParam) override {
18431845
PartialDiagnosticAt Warning(
18441846
Loc, S.PDiag(diag::warn_expect_more_underlying_mutexes)
18451847
<< Kind << scopeName << expected);
18461848
Warnings.emplace_back(std::move(Warning),
1847-
makeManagedMismatchNoteForParam(DLoc));
1849+
makeManagedMismatchNote(DLoc, forParam));
18481850
}
18491851

18501852
void handleExpectFewerUnderlyingMutexes(SourceLocation Loc,
18511853
SourceLocation DLoc, Name scopeName,
1852-
StringRef Kind,
1853-
Name actual) override {
1854+
StringRef Kind, Name actual,
1855+
bool forParam) override {
18541856
PartialDiagnosticAt Warning(
18551857
Loc, S.PDiag(diag::warn_expect_fewer_underlying_mutexes)
18561858
<< Kind << scopeName << actual);
18571859
Warnings.emplace_back(std::move(Warning),
1858-
makeManagedMismatchNoteForParam(DLoc));
1860+
makeManagedMismatchNote(DLoc, forParam));
18591861
}
18601862

18611863
void handleInvalidLockExp(SourceLocation Loc) override {

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

Lines changed: 53 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,27 @@ class SCOPED_LOCKABLE DoubleMutexLock {
5757
~DoubleMutexLock() UNLOCK_FUNCTION();
5858
};
5959

60+
class DeferTraits {};
61+
struct SharedTraits {};
62+
struct ExclusiveTraits {};
63+
64+
class SCOPED_LOCKABLE RelockableMutexLock {
65+
public:
66+
RelockableMutexLock(Mutex *mu, DeferTraits) LOCKS_EXCLUDED(mu);
67+
RelockableMutexLock(Mutex *mu, SharedTraits) SHARED_LOCK_FUNCTION(mu);
68+
RelockableMutexLock(Mutex *mu, ExclusiveTraits) EXCLUSIVE_LOCK_FUNCTION(mu);
69+
~RelockableMutexLock() UNLOCK_FUNCTION();
70+
71+
void Lock() EXCLUSIVE_LOCK_FUNCTION();
72+
void Unlock() UNLOCK_FUNCTION();
73+
74+
void ReaderLock() SHARED_LOCK_FUNCTION();
75+
void ReaderUnlock() UNLOCK_FUNCTION();
76+
77+
void PromoteShared() UNLOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION();
78+
void DemoteExclusive() UNLOCK_FUNCTION() SHARED_LOCK_FUNCTION();
79+
};
80+
6081
// The universal lock, written "*", allows checking to be selectively turned
6182
// off for a particular piece of code.
6283
void beginNoWarnOnReads() SHARED_LOCK_FUNCTION("*");
@@ -2753,8 +2774,6 @@ void Foo::test6() {
27532774

27542775
namespace RelockableScopedLock {
27552776

2756-
class DeferTraits {};
2757-
27582777
class SCOPED_LOCKABLE RelockableExclusiveMutexLock {
27592778
public:
27602779
RelockableExclusiveMutexLock(Mutex *mu) EXCLUSIVE_LOCK_FUNCTION(mu);
@@ -2765,26 +2784,6 @@ class SCOPED_LOCKABLE RelockableExclusiveMutexLock {
27652784
void Unlock() UNLOCK_FUNCTION();
27662785
};
27672786

2768-
struct SharedTraits {};
2769-
struct ExclusiveTraits {};
2770-
2771-
class SCOPED_LOCKABLE RelockableMutexLock {
2772-
public:
2773-
RelockableMutexLock(Mutex *mu, DeferTraits) LOCKS_EXCLUDED(mu);
2774-
RelockableMutexLock(Mutex *mu, SharedTraits) SHARED_LOCK_FUNCTION(mu);
2775-
RelockableMutexLock(Mutex *mu, ExclusiveTraits) EXCLUSIVE_LOCK_FUNCTION(mu);
2776-
~RelockableMutexLock() UNLOCK_FUNCTION();
2777-
2778-
void Lock() EXCLUSIVE_LOCK_FUNCTION();
2779-
void Unlock() UNLOCK_FUNCTION();
2780-
2781-
void ReaderLock() SHARED_LOCK_FUNCTION();
2782-
void ReaderUnlock() UNLOCK_FUNCTION();
2783-
2784-
void PromoteShared() UNLOCK_FUNCTION() EXCLUSIVE_LOCK_FUNCTION();
2785-
void DemoteExclusive() UNLOCK_FUNCTION() SHARED_LOCK_FUNCTION();
2786-
};
2787-
27882787
Mutex mu;
27892788
int x GUARDED_BY(mu);
27902789
bool b;
@@ -3560,6 +3559,38 @@ struct foo
35603559
}
35613560
};
35623561

3562+
#ifdef __cpp_guaranteed_copy_elision
3563+
// expected-note@+2{{mutex acquired here}}
3564+
// expected-note@+1{{see attribute on function here}}
3565+
RelockableScope returnUnmatchTest() EXCLUSIVE_LOCK_FUNCTION(mu){
3566+
// expected-note@+1{{mutex acquired here}}
3567+
return RelockableScope(&mu2); // expected-warning{{mutex managed by '<temporary>' is 'mu2' instead of 'mu'}}
3568+
} // expected-warning{{mutex 'mu2' is still held at the end of function}}
3569+
// expected-warning@-1{{expecting mutex 'mu' to be held at the end of function}}
3570+
3571+
// expected-note@+2{{mutex acquired here}}
3572+
// expected-note@+1{{see attribute on function here}}
3573+
RelockableScope returnMoreTest() EXCLUSIVE_LOCK_FUNCTION(mu, mu2){
3574+
return RelockableScope(&mu); // expected-warning{{mutex 'mu2' not managed by '<temporary>'}}
3575+
} // expected-warning{{expecting mutex 'mu2' to be held at the end of function}}
3576+
3577+
// expected-note@+1{{see attribute on function here}}
3578+
DoubleMutexLock returnFewerTest() EXCLUSIVE_LOCK_FUNCTION(mu){
3579+
// expected-note@+1{{mutex acquired here}}
3580+
return DoubleMutexLock(&mu,&mu2); // expected-warning{{did not expect mutex 'mu2' to be managed by '<temporary>'}}
3581+
} // expected-warning{{mutex 'mu2' is still held at the end of function}}
3582+
3583+
// expected-note@+1{{see attribute on function here}}
3584+
RelockableMutexLock lockTest() EXCLUSIVE_LOCK_FUNCTION(mu) {
3585+
mu.Lock();
3586+
return RelockableMutexLock(&mu2, DeferTraits{}); // expected-warning{{mutex managed by '<temporary>' is 'mu2' instead of 'mu'}}
3587+
}
3588+
3589+
// expected-note@+1{{mutex acquired here}}
3590+
RelockableMutexLock lockTest2() EXCLUSIVE_LOCK_FUNCTION(mu) {
3591+
return RelockableMutexLock(&mu, DeferTraits{});
3592+
} // expected-warning{{expecting mutex 'mu' to be held at the end of function}}
3593+
#endif
35633594

35643595
} // end namespace PassingScope
35653596

0 commit comments

Comments
 (0)