Skip to content

Commit ff8f6e2

Browse files
melvermelver
committed
Thread Safety Analysis: Very basic capability alias-analysis
Add a simple form of alias analysis for capabilities by substituting local pointer variables with their initializers if they are `const` or never reassigned. For example, the analysis will no longer generate false positives for cases such as: void testNestedAccess(Container *c) { Foo *ptr = &c->foo; ptr->mu.Lock(); c->foo.data = 42; // OK - no false positive ptr->mu.Unlock(); } void testNestedAcquire(Container *c) EXCLUSIVE_LOCK_FUNCTION(&c->foo.mu) { Foo *buf = &c->foo; buf->mu.Lock(); // OK - no false positive warning } This implementation would satisfy the basic needs of addressing the concerns for Linux kernel application [1]. Current limitations: * The analysis does not handle pointers that are reassigned; it conservatively assumes they could point to anything after the reassignment. * Aliases created through complex control flow are not tracked. Link: https://lore.kernel.org/all/CANpmjNPquO=W1JAh1FNQb8pMQjgeZAKCPQUAd7qUg=5pjJ6x=Q@mail.gmail.com/ [1]
1 parent 00cdaa5 commit ff8f6e2

File tree

5 files changed

+295
-22
lines changed

5 files changed

+295
-22
lines changed

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

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -386,6 +386,11 @@ class SExprBuilder {
386386
SelfVar->setKind(til::Variable::VK_SFun);
387387
}
388388

389+
// Create placeholder for this: we don't know the VarDecl on construction yet.
390+
til::LiteralPtr *createThisPlaceholder() {
391+
return new (Arena) til::LiteralPtr(nullptr);
392+
}
393+
389394
// Translate a clang expression in an attribute to a til::SExpr.
390395
// Constructs the context from D, DeclExp, and SelfDecl.
391396
CapabilityExpr translateAttrExpr(const Expr *AttrExp, const NamedDecl *D,
@@ -394,8 +399,8 @@ class SExprBuilder {
394399

395400
CapabilityExpr translateAttrExpr(const Expr *AttrExp, CallingContext *Ctx);
396401

397-
// Translate a variable reference.
398-
til::LiteralPtr *createVariable(const VarDecl *VD);
402+
// Translate a VarDecl to its canonical TIL expression.
403+
til::SExpr *translateVarDecl(const VarDecl *VD, CallingContext *Ctx);
399404

400405
// Translate a clang statement or expression to a TIL expression.
401406
// Also performs substitution of variables; Ctx provides the context.
@@ -501,6 +506,9 @@ class SExprBuilder {
501506
void mergeEntryMapBackEdge();
502507
void mergePhiNodesBackEdge(const CFGBlock *Blk);
503508

509+
// Returns true if a variable is assumed to be reassigned.
510+
bool isVariableReassigned(const VarDecl *VD);
511+
504512
private:
505513
// Set to true when parsing capability expressions, which get translated
506514
// inaccurately in order to hack around smart pointers etc.
@@ -531,6 +539,9 @@ class SExprBuilder {
531539
std::vector<til::Phi *> IncompleteArgs;
532540
til::BasicBlock *CurrentBB = nullptr;
533541
BlockInfo *CurrentBlockInfo = nullptr;
542+
543+
// Map caching if a local variable is reassigned.
544+
llvm::DenseMap<const VarDecl *, bool> LocalVariableReassigned;
534545
};
535546

536547
#ifndef NDEBUG

‎clang/lib/Analysis/ThreadSafety.cpp

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1141,11 +1141,10 @@ class ThreadSafetyAnalyzer {
11411141

11421142
void warnIfMutexNotHeld(const FactSet &FSet, const NamedDecl *D,
11431143
const Expr *Exp, AccessKind AK, Expr *MutexExp,
1144-
ProtectedOperationKind POK, til::LiteralPtr *Self,
1144+
ProtectedOperationKind POK, til::SExpr *Self,
11451145
SourceLocation Loc);
11461146
void warnIfMutexHeld(const FactSet &FSet, const NamedDecl *D, const Expr *Exp,
1147-
Expr *MutexExp, til::LiteralPtr *Self,
1148-
SourceLocation Loc);
1147+
Expr *MutexExp, til::SExpr *Self, SourceLocation Loc);
11491148

11501149
void checkAccess(const FactSet &FSet, const Expr *Exp, AccessKind AK,
11511150
ProtectedOperationKind POK);
@@ -1599,7 +1598,7 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
15991598
}
16001599

16011600
void handleCall(const Expr *Exp, const NamedDecl *D,
1602-
til::LiteralPtr *Self = nullptr,
1601+
til::SExpr *Self = nullptr,
16031602
SourceLocation Loc = SourceLocation());
16041603
void examineArguments(const FunctionDecl *FD,
16051604
CallExpr::const_arg_iterator ArgBegin,
@@ -1629,7 +1628,7 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
16291628
/// of at least the passed in AccessKind.
16301629
void ThreadSafetyAnalyzer::warnIfMutexNotHeld(
16311630
const FactSet &FSet, const NamedDecl *D, const Expr *Exp, AccessKind AK,
1632-
Expr *MutexExp, ProtectedOperationKind POK, til::LiteralPtr *Self,
1631+
Expr *MutexExp, ProtectedOperationKind POK, til::SExpr *Self,
16331632
SourceLocation Loc) {
16341633
LockKind LK = getLockKindFromAccessKind(AK);
16351634
CapabilityExpr Cp = SxBuilder.translateAttrExpr(MutexExp, D, Exp, Self);
@@ -1688,8 +1687,7 @@ void ThreadSafetyAnalyzer::warnIfMutexNotHeld(
16881687
/// Warn if the LSet contains the given lock.
16891688
void ThreadSafetyAnalyzer::warnIfMutexHeld(const FactSet &FSet,
16901689
const NamedDecl *D, const Expr *Exp,
1691-
Expr *MutexExp,
1692-
til::LiteralPtr *Self,
1690+
Expr *MutexExp, til::SExpr *Self,
16931691
SourceLocation Loc) {
16941692
CapabilityExpr Cp = SxBuilder.translateAttrExpr(MutexExp, D, Exp, Self);
16951693
if (Cp.isInvalid()) {
@@ -1857,7 +1855,7 @@ void ThreadSafetyAnalyzer::checkPtAccess(const FactSet &FSet, const Expr *Exp,
18571855
/// of an implicitly called cleanup function.
18581856
/// \param Loc If \p Exp = nullptr, the location.
18591857
void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
1860-
til::LiteralPtr *Self, SourceLocation Loc) {
1858+
til::SExpr *Self, SourceLocation Loc) {
18611859
CapExprSet ExclusiveLocksToAdd, SharedLocksToAdd;
18621860
CapExprSet ExclusiveLocksToRemove, SharedLocksToRemove, GenericLocksToRemove;
18631861
CapExprSet ScopedReqsAndExcludes;
@@ -1869,7 +1867,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
18691867
const auto *TagT = Exp->getType()->getAs<TagType>();
18701868
if (D->hasAttrs() && TagT && Exp->isPRValue()) {
18711869
til::LiteralPtr *Placeholder =
1872-
Analyzer->SxBuilder.createVariable(nullptr);
1870+
Analyzer->SxBuilder.createThisPlaceholder();
18731871
[[maybe_unused]] auto inserted =
18741872
Analyzer->ConstructedObjects.insert({Exp, Placeholder});
18751873
assert(inserted.second && "Are we visiting the same expression again?");
@@ -2545,7 +2543,7 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
25452543
}
25462544
if (UnderlyingLocks.empty())
25472545
continue;
2548-
CapabilityExpr Cp(SxBuilder.createVariable(Param), StringRef(),
2546+
CapabilityExpr Cp(SxBuilder.translateVarDecl(Param, nullptr), StringRef(),
25492547
/*Neg=*/false, /*Reentrant=*/false);
25502548
auto ScopedEntry = std::make_unique<ScopedLockableFactEntry>(
25512549
Cp, Param->getLocation(), FactEntry::Declared);
@@ -2662,17 +2660,18 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
26622660
if (!DD->hasAttrs())
26632661
break;
26642662

2665-
LocksetBuilder.handleCall(nullptr, DD,
2666-
SxBuilder.createVariable(AD.getVarDecl()),
2667-
AD.getTriggerStmt()->getEndLoc());
2663+
LocksetBuilder.handleCall(
2664+
nullptr, DD, SxBuilder.translateVarDecl(AD.getVarDecl(), nullptr),
2665+
AD.getTriggerStmt()->getEndLoc());
26682666
break;
26692667
}
26702668

26712669
case CFGElement::CleanupFunction: {
26722670
const CFGCleanupFunction &CF = BI.castAs<CFGCleanupFunction>();
2673-
LocksetBuilder.handleCall(/*Exp=*/nullptr, CF.getFunctionDecl(),
2674-
SxBuilder.createVariable(CF.getVarDecl()),
2675-
CF.getVarDecl()->getLocation());
2671+
LocksetBuilder.handleCall(
2672+
/*Exp=*/nullptr, CF.getFunctionDecl(),
2673+
SxBuilder.translateVarDecl(CF.getVarDecl(), nullptr),
2674+
CF.getVarDecl()->getLocation());
26762675
break;
26772676
}
26782677

‎clang/lib/Analysis/ThreadSafetyCommon.cpp

Lines changed: 120 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
#include "clang/AST/Expr.h"
2020
#include "clang/AST/ExprCXX.h"
2121
#include "clang/AST/OperationKinds.h"
22+
#include "clang/AST/RecursiveASTVisitor.h"
2223
#include "clang/AST/Stmt.h"
2324
#include "clang/AST/Type.h"
2425
#include "clang/Analysis/Analyses/ThreadSafetyTIL.h"
@@ -241,7 +242,21 @@ CapabilityExpr SExprBuilder::translateAttrExpr(const Expr *AttrExp,
241242
return CapabilityExpr(E, AttrExp->getType(), Neg);
242243
}
243244

244-
til::LiteralPtr *SExprBuilder::createVariable(const VarDecl *VD) {
245+
til::SExpr *SExprBuilder::translateVarDecl(const VarDecl *VD,
246+
CallingContext *Ctx) {
247+
assert(VD);
248+
// Substitute local pointer variables with their initializers if they are
249+
// explicitly const or never reassigned.
250+
QualType Ty = VD->getType();
251+
if (Ty->isPointerType() && VD->hasInit() && !isVariableReassigned(VD)) {
252+
const Expr *Init = VD->getInit()->IgnoreParenImpCasts();
253+
// Check for self-initialization to prevent infinite recursion.
254+
if (const auto *InitDRE = dyn_cast<DeclRefExpr>(Init)) {
255+
if (InitDRE->getDecl()->getCanonicalDecl() == VD->getCanonicalDecl())
256+
return new (Arena) til::LiteralPtr(VD);
257+
}
258+
return translate(Init, Ctx);
259+
}
245260
return new (Arena) til::LiteralPtr(VD);
246261
}
247262

@@ -353,6 +368,9 @@ til::SExpr *SExprBuilder::translateDeclRefExpr(const DeclRefExpr *DRE,
353368
: cast<ObjCMethodDecl>(D)->getCanonicalDecl()->getParamDecl(I);
354369
}
355370

371+
if (const auto *VarD = dyn_cast<VarDecl>(VD))
372+
return translateVarDecl(VarD, Ctx);
373+
356374
// For non-local variables, treat it as a reference to a named object.
357375
return new (Arena) til::LiteralPtr(VD);
358376
}
@@ -1012,6 +1030,107 @@ void SExprBuilder::exitCFG(const CFGBlock *Last) {
10121030
IncompleteArgs.clear();
10131031
}
10141032

1033+
bool SExprBuilder::isVariableReassigned(const VarDecl *VD) {
1034+
// Note: The search is performed lazily per-variable and result is cached. An
1035+
// alternative would have been to eagerly create a set of all reassigned
1036+
// variables, but that would consume significantly more memory. The number of
1037+
// variables needing the reassignment check in a single function is expected
1038+
// to be small. Also see createVariable().
1039+
struct ReassignmentFinder : RecursiveASTVisitor<ReassignmentFinder> {
1040+
explicit ReassignmentFinder(const VarDecl *VD) : QueryVD(VD) {
1041+
assert(QueryVD->getCanonicalDecl() == QueryVD);
1042+
}
1043+
1044+
bool VisitBinaryOperator(BinaryOperator *BO) {
1045+
if (!BO->isAssignmentOp())
1046+
return true;
1047+
auto *DRE = dyn_cast<DeclRefExpr>(BO->getLHS()->IgnoreParenImpCasts());
1048+
if (!DRE)
1049+
return true;
1050+
auto *AssignedVD = dyn_cast<VarDecl>(DRE->getDecl());
1051+
if (!AssignedVD)
1052+
return true;
1053+
// Don't count the initialization itself as a reassignment.
1054+
if (AssignedVD->hasInit() &&
1055+
AssignedVD->getInit()->getBeginLoc() == BO->getBeginLoc())
1056+
return true;
1057+
// If query variable appears as LHS of assignment.
1058+
if (QueryVD == AssignedVD->getCanonicalDecl()) {
1059+
FoundReassignment = true;
1060+
return false; // stop
1061+
}
1062+
return true;
1063+
}
1064+
1065+
bool VisitCallExpr(CallExpr *CE) {
1066+
const FunctionDecl *FD = CE->getDirectCallee();
1067+
if (!FD)
1068+
return true;
1069+
1070+
for (unsigned Idx = 0; Idx < CE->getNumArgs(); ++Idx) {
1071+
if (FoundReassignment)
1072+
return false;
1073+
if (Idx >= FD->getNumParams())
1074+
break;
1075+
1076+
const Expr *Arg = CE->getArg(Idx)->IgnoreParenImpCasts();
1077+
const ParmVarDecl *PVD = FD->getParamDecl(Idx);
1078+
QualType ParamType = PVD->getType();
1079+
1080+
// Check if the argument is a reference to our QueryVD.
1081+
if (const auto *DRE = dyn_cast<DeclRefExpr>(Arg)) {
1082+
if (DRE->getDecl()->getCanonicalDecl() == QueryVD) {
1083+
// Potential reassignment if passed by non-const reference.
1084+
if (ParamType->isReferenceType() &&
1085+
!ParamType->getPointeeType().isConstQualified()) {
1086+
FoundReassignment = true;
1087+
}
1088+
}
1089+
}
1090+
// Check if we are taking the address of the variable.
1091+
if (const auto *UO = dyn_cast<UnaryOperator>(Arg);
1092+
UO && UO->getOpcode() == UO_AddrOf) {
1093+
const Expr *SE = UO->getSubExpr()->IgnoreParenImpCasts();
1094+
if (const auto *DRE = dyn_cast<DeclRefExpr>(SE)) {
1095+
if (DRE->getDecl()->getCanonicalDecl() == QueryVD) {
1096+
// Potential reassignment if passed by non-const pointer.
1097+
if (ParamType->isPointerType() &&
1098+
!ParamType->getPointeeType().isConstQualified()) {
1099+
FoundReassignment = true;
1100+
}
1101+
}
1102+
}
1103+
}
1104+
}
1105+
1106+
return !FoundReassignment;
1107+
}
1108+
1109+
const VarDecl *QueryVD;
1110+
bool FoundReassignment = false;
1111+
};
1112+
1113+
if (VD->getType().isConstQualified())
1114+
return false; // Assume UB-freedom.
1115+
if (!VD->isLocalVarDecl())
1116+
return true; // Not a local variable (assume reassigned).
1117+
auto *FD = dyn_cast<FunctionDecl>(VD->getDeclContext());
1118+
if (!FD)
1119+
return true; // Assume reassigned.
1120+
1121+
// Try to look up in cache; use the canonical declaration to ensure consistent
1122+
// lookup in the cache.
1123+
VD = VD->getCanonicalDecl();
1124+
auto It = LocalVariableReassigned.find(VD);
1125+
if (It != LocalVariableReassigned.end())
1126+
return It->second;
1127+
1128+
ReassignmentFinder Visitor(VD);
1129+
// const_cast ok: FunctionDecl not modified.
1130+
Visitor.TraverseDecl(const_cast<FunctionDecl *>(FD));
1131+
return LocalVariableReassigned[VD] = Visitor.FoundReassignment;
1132+
}
1133+
10151134
#ifndef NDEBUG
10161135
namespace {
10171136

‎clang/test/Sema/warn-thread-safety-analysis.c

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -184,9 +184,13 @@ int main(void) {
184184
/// Cleanup functions
185185
{
186186
struct Mutex* const __attribute__((cleanup(unlock_scope))) scope = &mu1;
187-
mutex_exclusive_lock(scope); // Note that we have to lock through scope, because no alias analysis!
187+
mutex_exclusive_lock(scope); // Lock through scope works.
188188
// Cleanup happens automatically -> no warning.
189189
}
190+
{
191+
struct Mutex* const __attribute__((unused, cleanup(unlock_scope))) scope = &mu1;
192+
mutex_exclusive_lock(&mu1); // With basic alias analysis lock through mu1 also works.
193+
}
190194

191195
foo_.a_value = 0; // expected-warning {{writing variable 'a_value' requires holding mutex 'mu_' exclusively}}
192196
*foo_.a_ptr = 1; // expected-warning {{writing the value pointed to by 'a_ptr' requires holding mutex 'bar.other_mu' exclusively}}

0 commit comments

Comments
 (0)