Skip to content

Commit cea00f0

Browse files
malek203malek203
committed
Thread Safety Analysis: Support passing scoped locks between functions with appropriate annotations
This is helpful when multiple functions operate on the same capabilities, but we still want to use scoped lockable types for readability and exception safety. - Introduce support for thread safety annotations on function parameters marked with the 'scoped_lockable' attribute. - Add semantic checks for annotated function parameters, ensuring correct usage. - Enhance the analysis to recognize and handle parameters annotated for thread safety, extending the scope of analysis to track these across function boundries. - Verify that the underlying mutexes of function arguments match the expectations set by the annotations. Limitation: This does not work when the attribute arguments are class members, because attributes on function parameters are parsed differently from attributes on functions.
1 parent 800593a commit cea00f0

File tree

6 files changed

+48
-56
lines changed

6 files changed

+48
-56
lines changed

‎clang/docs/ThreadSafetyAnalysis.rst

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -216,13 +216,14 @@ the scoped capability manages the specified capabilities in the given order.
216216

217217
void require(MutexLocker& scope REQUIRES(mu1)) {
218218
scope.Unlock();
219-
a = 0; // Warning! Requires mu1.
219+
a=0; // Warning! Requires mu1.
220220
scope.Lock();
221221
}
222222

223223
void testParameter() {
224-
MutexLocker scope(&mu1), scope2(&mu2);
225-
require(scope2); // Warning! Mutex managed by 'scope2' is 'mu2' instead of 'mu1'
224+
MutexLocker scope(&mu1);
225+
MutexLocker scope2(&mu2);
226+
require(scope2); // Warning! Mutex managed by 'scope' is 'mu2' instead of 'mu1'
226227
require(scope); // OK.
227228
scope.Unlock();
228229
require(scope); // Warning! Requires mu1.
@@ -336,7 +337,7 @@ the scoped capability manages the specified capabilities in the given order.
336337
mu.Unlock();
337338
}
338339

339-
void exclude(MutexLocker& scope LOCKS_EXCLUDED(mu)) {
340+
void exclude(MutexLocker& scope LOCKS_EXCLUDED(mu)){
340341
scope.Unlock(); // Warning! mu is not locked.
341342
scope.Lock();
342343
} // Warning! mu still held at the end of function.

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -268,8 +268,7 @@ class ThreadSafetyHandler {
268268
virtual void handleExpectFewerUnderlyingMutexes(SourceLocation Loc,
269269
SourceLocation DLoc,
270270
Name ScopeName,
271-
StringRef Kind, Name Actual) {
272-
}
271+
StringRef Kind, Name Actual) {}
273272

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

‎clang/lib/Analysis/ThreadSafety.cpp

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -126,8 +126,8 @@ class FactEntry : public CapabilityExpr {
126126
SourceLocation AcquireLoc;
127127

128128
public:
129-
FactEntry(FactEntryKind FK, const CapabilityExpr &CE, LockKind LK,
130-
SourceLocation Loc, SourceKind Src)
129+
FactEntry(FactEntryKind FK, const CapabilityExpr &CE, LockKind LK, SourceLocation Loc,
130+
SourceKind Src)
131131
: CapabilityExpr(CE), Kind(FK), LKind(LK), Source(Src), AcquireLoc(Loc) {}
132132
virtual ~FactEntry() = default;
133133

@@ -1969,7 +1969,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
19691969
else
19701970
llvm_unreachable("Unknown call kind");
19711971
}
1972-
const auto *CalledFunction = dyn_cast<FunctionDecl>(D);
1972+
const FunctionDecl *CalledFunction = dyn_cast<FunctionDecl>(D);
19731973
if (CalledFunction && Args.has_value()) {
19741974
for (auto [Param, Arg] : zip(CalledFunction->parameters(), *Args)) {
19751975
CapExprSet DeclaredLocks;
@@ -1980,7 +1980,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
19801980
Analyzer->getMutexIDs(A->isShared() ? SharedLocksToAdd
19811981
: ExclusiveLocksToAdd,
19821982
A, Exp, D, Self);
1983-
Analyzer->getMutexIDs(DeclaredLocks, A, Exp, D, Self);
1983+
Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
19841984
break;
19851985
}
19861986

@@ -1992,7 +1992,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
19921992
Analyzer->getMutexIDs(SharedLocksToRemove, A, Exp, D, Self);
19931993
else
19941994
Analyzer->getMutexIDs(ExclusiveLocksToRemove, A, Exp, D, Self);
1995-
Analyzer->getMutexIDs(DeclaredLocks, A, Exp, D, Self);
1995+
Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
19961996
break;
19971997
}
19981998

@@ -2002,31 +2002,30 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
20022002
Analyzer->warnIfMutexNotHeld(FSet, D, Exp,
20032003
A->isShared() ? AK_Read : AK_Written,
20042004
Arg, POK_FunctionCall, Self, Loc);
2005-
Analyzer->getMutexIDs(DeclaredLocks, A, Exp, D, Self);
2005+
Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
20062006
break;
20072007
}
20082008

20092009
case attr::LocksExcluded: {
20102010
const auto *A = cast<LocksExcludedAttr>(At);
20112011
for (auto *Arg : A->args())
20122012
Analyzer->warnIfMutexHeld(FSet, D, Exp, Arg, Self, Loc);
2013-
Analyzer->getMutexIDs(DeclaredLocks, A, Exp, D, Self);
2013+
Analyzer->getMutexIDs(DeclaredLocks, A, nullptr, Param);
20142014
break;
20152015
}
20162016

20172017
default:
20182018
break;
20192019
}
20202020
}
2021-
if (DeclaredLocks.empty())
2021+
if (DeclaredLocks.size() == 0)
20222022
continue;
2023-
CapabilityExpr Cp(Analyzer->SxBuilder.translate(Arg, nullptr),
2024-
StringRef("mutex"), false);
2023+
CapabilityExpr Cp = Analyzer->SxBuilder.translateAttrExpr(Arg, nullptr);
20252024
if (const auto *CBTE = dyn_cast<CXXBindTemporaryExpr>(Arg->IgnoreCasts());
20262025
Cp.isInvalid() && CBTE) {
20272026
if (auto Object = Analyzer->ConstructedObjects.find(CBTE->getSubExpr());
20282027
Object != Analyzer->ConstructedObjects.end())
2029-
Cp = CapabilityExpr(Object->second, StringRef("mutex"), false);
2028+
Cp = CapabilityExpr(Object->second, StringRef(), false);
20302029
}
20312030
const FactEntry *Fact = FSet.findLock(Analyzer->FactMan, Cp);
20322031
if (!Fact) {
@@ -2035,7 +2034,8 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
20352034
Exp->getExprLoc());
20362035
continue;
20372036
}
2038-
const auto *Scope = cast<ScopedLockableFactEntry>(Fact);
2037+
const auto *Scope =
2038+
cast<ScopedLockableFactEntry>(Fact);
20392039
for (const auto &[a, b] :
20402040
zip_longest(DeclaredLocks, Scope->getUnderlyingMutexes())) {
20412041
if (!a.has_value()) {
@@ -2046,7 +2046,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
20462046
Analyzer->Handler.handleExpectMoreUnderlyingMutexes(
20472047
Exp->getExprLoc(), D->getLocation(), Scope->toString(),
20482048
a.value().getKind(), a.value().toString());
2049-
} else if (!a.value().equals(b.value())) {
2049+
} else if (!a.value().matches(b.value())) {
20502050
Analyzer->Handler.handleUnmatchedUnderlyingMutexes(
20512051
Exp->getExprLoc(), D->getLocation(), Scope->toString(),
20522052
a.value().getKind(), a.value().toString(), b.value().toString());
@@ -2541,7 +2541,7 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
25412541
}
25422542
if (UnderlyingLocks.empty())
25432543
continue;
2544-
CapabilityExpr Cp(SxBuilder.createVariable(Param), StringRef(), false);
2544+
CapabilityExpr Cp (SxBuilder.createVariable(Param), StringRef(), false);
25452545
auto ScopedEntry = std::make_unique<ScopedLockableFactEntry>(
25462546
Cp, Param->getLocation(), FactEntry::Declared);
25472547
for (const CapabilityExpr &M : UnderlyingLocks)

‎clang/lib/Sema/AnalysisBasedWarnings.cpp

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1802,8 +1802,7 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
18021802
OptionalNotes makeManagedMismatchNoteForParam(SourceLocation DeclLoc) {
18031803
return DeclLoc.isValid()
18041804
? getNotes(PartialDiagnosticAt(
1805-
DeclLoc,
1806-
S.PDiag(diag::note_managed_mismatch_here_for_param)))
1805+
DeclLoc, S.PDiag(diag::note_managed_mismatch_here_for_param)))
18071806
: getNotes();
18081807
}
18091808

@@ -1839,7 +1838,7 @@ class ThreadSafetyReporter : public clang::threadSafety::ThreadSafetyHandler {
18391838

18401839
void handleExpectMoreUnderlyingMutexes(SourceLocation Loc,
18411840
SourceLocation DLoc, Name scopeName,
1842-
StringRef Kind,
1841+
StringRef Kind,
18431842
Name expected) override {
18441843
PartialDiagnosticAt Warning(
18451844
Loc, S.PDiag(diag::warn_expect_more_underlying_mutexes)

‎clang/lib/Sema/SemaDeclAttr.cpp

Lines changed: 17 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -430,14 +430,12 @@ static void checkAttrArgsAreCapabilityObjs(Sema &S, Decl *D,
430430
}
431431
}
432432

433-
static bool checkFunParamsAreScopedLockable(Sema &S,
434-
const ParmVarDecl *ParamDecl,
433+
static bool checkFunParamsAreScopedLockable(Sema &S, const ParmVarDecl *ParamDecl,
435434
const ParsedAttr &AL) {
436435
QualType ParamType = ParamDecl->getType();
437-
if (const auto *RefType = ParamType->getAs<ReferenceType>();
438-
RefType &&
439-
checkRecordTypeForScopedCapability(S, RefType->getPointeeType()))
440-
return true;
436+
if (const auto *RefType = ParamType->getAs<ReferenceType>())
437+
if (checkRecordTypeForScopedCapability(S, RefType->getPointeeType()))
438+
return true;
441439
S.Diag(AL.getLoc(), diag::warn_thread_attribute_not_on_scoped_lockable_param)
442440
<< AL;
443441
return false;
@@ -670,9 +668,10 @@ static void handleLockReturnedAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
670668
}
671669

672670
static void handleLocksExcludedAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
673-
if (const auto *ParmDecl = dyn_cast<ParmVarDecl>(D);
674-
ParmDecl && !checkFunParamsAreScopedLockable(S, ParmDecl, AL))
675-
return;
671+
672+
if (const auto *ParmDecl = dyn_cast<ParmVarDecl>(D))
673+
if (!checkFunParamsAreScopedLockable(S, ParmDecl, AL))
674+
return;
676675

677676
if (!AL.checkAtLeastNumArgs(S, 1))
678677
return;
@@ -6197,9 +6196,9 @@ static void handleAssertCapabilityAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
61976196

61986197
static void handleAcquireCapabilityAttr(Sema &S, Decl *D,
61996198
const ParsedAttr &AL) {
6200-
if (const auto *ParmDecl = dyn_cast<ParmVarDecl>(D);
6201-
ParmDecl && !checkFunParamsAreScopedLockable(S, ParmDecl, AL))
6202-
return;
6199+
if (const auto *ParmDecl = dyn_cast<ParmVarDecl>(D))
6200+
if (!checkFunParamsAreScopedLockable(S, ParmDecl, AL))
6201+
return;
62036202

62046203
SmallVector<Expr*, 1> Args;
62056204
if (!checkLockFunAttrCommon(S, D, AL, Args))
@@ -6221,9 +6220,9 @@ static void handleTryAcquireCapabilityAttr(Sema &S, Decl *D,
62216220

62226221
static void handleReleaseCapabilityAttr(Sema &S, Decl *D,
62236222
const ParsedAttr &AL) {
6224-
if (const auto *ParmDecl = dyn_cast<ParmVarDecl>(D);
6225-
ParmDecl && !checkFunParamsAreScopedLockable(S, ParmDecl, AL))
6226-
return;
6223+
if (const auto *ParmDecl = dyn_cast<ParmVarDecl>(D))
6224+
if (!checkFunParamsAreScopedLockable(S, ParmDecl, AL))
6225+
return;
62276226
// Check that all arguments are lockable objects.
62286227
SmallVector<Expr *, 1> Args;
62296228
checkAttrArgsAreCapabilityObjs(S, D, AL, Args, 0, true);
@@ -6234,9 +6233,9 @@ static void handleReleaseCapabilityAttr(Sema &S, Decl *D,
62346233

62356234
static void handleRequiresCapabilityAttr(Sema &S, Decl *D,
62366235
const ParsedAttr &AL) {
6237-
if (const auto *ParmDecl = dyn_cast<ParmVarDecl>(D);
6238-
ParmDecl && !checkFunParamsAreScopedLockable(S, ParmDecl, AL))
6239-
return;
6236+
if (const auto *ParmDecl = dyn_cast<ParmVarDecl>(D))
6237+
if (!checkFunParamsAreScopedLockable(S, ParmDecl, AL))
6238+
return;
62406239

62416240
if (!AL.checkAtLeastNumArgs(S, 1))
62426241
return;

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

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -3496,10 +3496,17 @@ void acquireCallWithAlreadyHeldLock() {
34963496

34973497
void excludeCallWithAlreadyHeldLock() {
34983498
RelockableScope scope(&mu);
3499-
exclude(scope); // expected-warning{{cannot call function 'exclude' while mutex 'mu' is held}}
3499+
exclude(scope);// expected-warning{{cannot call function 'exclude' while mutex 'mu' is held}}
35003500
x = 2; // OK.
35013501
}
35023502

3503+
// Passing a scope by value
3504+
RelockableScope lock() EXCLUSIVE_LOCK_FUNCTION(mu);
3505+
void destruct(RelockableScope scope) {}
3506+
void passScopeByValue() {
3507+
destruct(lock());
3508+
}
3509+
35033510
void requireConst(const ReleasableMutexLock& scope EXCLUSIVE_LOCKS_REQUIRED(mu));
35043511
void requireConstCall() {
35053512
requireConst(ReleasableMutexLock(&mu));
@@ -3534,7 +3541,7 @@ void requireCallWithReleasedLock2() {
35343541
}
35353542

35363543
void requireDecl(RelockableScope &scope EXCLUSIVE_LOCKS_REQUIRED(mu));
3537-
void requireDecl(RelockableScope &scope) {
3544+
void requireDecl(RelockableScope &scope){
35383545
scope.Release();
35393546
scope.Acquire();
35403547
}
@@ -3553,19 +3560,6 @@ struct foo
35533560
}
35543561
};
35553562

3556-
struct ObjectWithMutex {
3557-
Mutex mu;
3558-
int x GUARDED_BY(mu);
3559-
};
3560-
void releaseMember(ObjectWithMutex& object, ReleasableMutexLock& scope EXCLUSIVE_UNLOCK_FUNCTION(object.mu)) {
3561-
object.x = 1;
3562-
scope.Release();
3563-
}
3564-
void releaseMemberCall() {
3565-
ObjectWithMutex obj;
3566-
ReleasableMutexLock lock(&obj.mu);
3567-
releaseMember(obj, lock);
3568-
}
35693563

35703564
} // end namespace PassingScope
35713565

0 commit comments

Comments
 (0)