Skip to content

Commit c190269

Browse files
ziqingluo-90Michael137
authored andcommitted
Thread safety analysis: Fix a bug in handling temporary constructors (llvm#74020)
Extends the lifetime of the map `ConstructedObjects` to be of the whole CFG so that the map can connect temporary Ctor and Dtor in different CFG blocks. (cherry picked from commit a341e17)
1 parent 90e4a9a commit c190269

File tree

2 files changed

+21
-13
lines changed

2 files changed

+21
-13
lines changed

clang/lib/Analysis/ThreadSafety.cpp

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1010,6 +1010,8 @@ class ThreadSafetyAnalyzer {
10101010
ThreadSafetyHandler &Handler;
10111011
const CXXMethodDecl *CurrentMethod;
10121012
LocalVariableMap LocalVarMap;
1013+
// Maps constructed objects to `this` placeholder prior to initialization.
1014+
llvm::SmallDenseMap<const Expr *, til::LiteralPtr *> ConstructedObjects;
10131015
FactManager FactMan;
10141016
std::vector<CFGBlockInfo> BlockInfo;
10151017

@@ -1528,8 +1530,6 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
15281530

15291531
ThreadSafetyAnalyzer *Analyzer;
15301532
FactSet FSet;
1531-
/// Maps constructed objects to `this` placeholder prior to initialization.
1532-
llvm::SmallDenseMap<const Expr *, til::LiteralPtr *> ConstructedObjects;
15331533
LocalVariableMap::Context LVarCtx;
15341534
unsigned CtxIndex;
15351535

@@ -1790,7 +1790,7 @@ void BuildLockset::handleCall(const Expr *Exp, const NamedDecl *D,
17901790
std::pair<til::LiteralPtr *, StringRef> Placeholder =
17911791
Analyzer->SxBuilder.createThisPlaceholder(Exp);
17921792
[[maybe_unused]] auto inserted =
1793-
ConstructedObjects.insert({Exp, Placeholder.first});
1793+
Analyzer->ConstructedObjects.insert({Exp, Placeholder.first});
17941794
assert(inserted.second && "Are we visiting the same expression again?");
17951795
if (isa<CXXConstructExpr>(Exp))
17961796
Self = Placeholder.first;
@@ -2109,10 +2109,10 @@ void BuildLockset::VisitDeclStmt(const DeclStmt *S) {
21092109
E = EWC->getSubExpr()->IgnoreParens();
21102110
E = UnpackConstruction(E);
21112111

2112-
if (auto Object = ConstructedObjects.find(E);
2113-
Object != ConstructedObjects.end()) {
2112+
if (auto Object = Analyzer->ConstructedObjects.find(E);
2113+
Object != Analyzer->ConstructedObjects.end()) {
21142114
Object->second->setClangDecl(VD);
2115-
ConstructedObjects.erase(Object);
2115+
Analyzer->ConstructedObjects.erase(Object);
21162116
}
21172117
}
21182118
}
@@ -2121,11 +2121,11 @@ void BuildLockset::VisitDeclStmt(const DeclStmt *S) {
21212121
void BuildLockset::VisitMaterializeTemporaryExpr(
21222122
const MaterializeTemporaryExpr *Exp) {
21232123
if (const ValueDecl *ExtD = Exp->getExtendingDecl()) {
2124-
if (auto Object =
2125-
ConstructedObjects.find(UnpackConstruction(Exp->getSubExpr()));
2126-
Object != ConstructedObjects.end()) {
2124+
if (auto Object = Analyzer->ConstructedObjects.find(
2125+
UnpackConstruction(Exp->getSubExpr()));
2126+
Object != Analyzer->ConstructedObjects.end()) {
21272127
Object->second->setClangDecl(ExtD);
2128-
ConstructedObjects.erase(Object);
2128+
Analyzer->ConstructedObjects.erase(Object);
21292129
}
21302130
}
21312131
}
@@ -2419,15 +2419,15 @@ void ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext &AC) {
24192419

24202420
// Clean up constructed object even if there are no attributes to
24212421
// keep the number of objects in limbo as small as possible.
2422-
if (auto Object = LocksetBuilder.ConstructedObjects.find(
2422+
if (auto Object = ConstructedObjects.find(
24232423
TD.getBindTemporaryExpr()->getSubExpr());
2424-
Object != LocksetBuilder.ConstructedObjects.end()) {
2424+
Object != ConstructedObjects.end()) {
24252425
const auto *DD = TD.getDestructorDecl(AC.getASTContext());
24262426
if (DD->hasAttrs())
24272427
// TODO: the location here isn't quite correct.
24282428
LocksetBuilder.handleCall(nullptr, DD, Object->second,
24292429
TD.getBindTemporaryExpr()->getEndLoc());
2430-
LocksetBuilder.ConstructedObjects.erase(Object);
2430+
ConstructedObjects.erase(Object);
24312431
}
24322432
break;
24332433
}

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1702,6 +1702,8 @@ struct TestScopedLockable {
17021702

17031703
bool getBool();
17041704

1705+
bool lock2Bool(MutexLock);
1706+
17051707
void foo1() {
17061708
MutexLock mulock(&mu1);
17071709
a = 5;
@@ -1718,6 +1720,12 @@ struct TestScopedLockable {
17181720
MutexLock{&mu1}, a = 5;
17191721
}
17201722

1723+
void temporary_cfg(int x) {
1724+
// test the case where a pair of temporary Ctor and Dtor is in different CFG blocks
1725+
lock2Bool(MutexLock{&mu1}) || x;
1726+
MutexLock{&mu1}; // no-warn
1727+
}
1728+
17211729
void lifetime_extension() {
17221730
const MutexLock &mulock = MutexLock(&mu1);
17231731
a = 5;

0 commit comments

Comments
 (0)