[PATCH] D43477: [CFG] [analyzer] Add MaterializeTemporaryExpr into the construction context.
lxfind added a comment.
Herald added subscribers: manas, steakhal, ASDenysPetrov, martong, dkrupp,
donat.nagy, Szelethus, mikhail.ramalho, baloghadamsoftware.
Hi! I have a question regarding the implementation of
"VisitMaterializeTemporaryExpr". Specifically, I wonder if we should skip
visiting the children? Would't visiting the children of
MaterializeTemporaryExpr cause the same expression to be visited twice?
I am debugging a crash in ThreadSafetyAnalyzer, which triggers this assertion:
https://github.com/llvm/llvm-project/blob/main/clang/lib/Analysis/ThreadSafety.cpp#L534
Basically it's adding the same declaration twice from the same CFG block.
And I found that the redundant declaration is added during CFG construction,
when processing cpp source code like this:
co_return ({ static constexpr mydomain::logdevice::ErrorStacktrace::Frame
frame{ __FUNCTION__, "logdevice/common/ZookeeperClientBase.cpp", 103};
mydomain::logdevice::detail::makeUnexpected(, toStatus(result.rc_)); });
which corresponds to the following AST:
| `-CompoundStmt 0x227b13f8
| `-CoreturnStmt 0x227b13d0
| |-CXXBindTemporaryExpr 0x22776218 'folly::Unexpected':'class
folly::Unexpected' (CXXTemporary 0x22776218)
| | `-StmtExpr 0x227761f0 'folly::Unexpected':'class
folly::Unexpected'
| | `-CompoundStmt 0x227761d0
| | |-DeclStmt 0x22771e88
| | | `-VarDecl 0x22771c50 used frame 'const
mydomain::logdevice::class ErrorStacktrace::Frame':'const struct
mydomain::logdevice::ErrorStacktrace::Frame' static constexpr listinit
| | | |-value: Struct
| | | | `-fields: LValue , LValue , Int 103
| | | `-InitListExpr 0x22771da8 'const mydomain::logdevice::class
ErrorStacktrace::Frame':'const struct
mydomain::logdevice::ErrorStacktrace::Frame'
| | | |-ImplicitCastExpr 0x22771e00 'const char *'
| | | | `-PredefinedExpr 0x22771cd8 'const char [8]' lvalue
__FUNCTION__
| | | | `-StringLiteral 0x22771cb8 'const char [8]' lvalue
"getData"
| | | |-ImplicitCastExpr 0x22771e18 'const char *'
| | | | `-StringLiteral 0x22771cf0 'const char [41]' lvalue
"logdevice/common/ZookeeperClientBase.cpp"
| | | `-IntegerLiteral 0x22771d30 'int' 103
| | `-ExprWithCleanups 0x227761b8 'folly::Unexpected':'class
folly::Unexpected'
| | `-CXXBindTemporaryExpr 0x22776198
'folly::Unexpected':'class folly::Unexpected' (CXXTemporary 0x22776198)
| | `-CallExpr 0x22776160 'folly::Unexpected':'class
folly::Unexpected'
| | |-ImplicitCastExpr 0x22776148 'folly::Unexpected
(*)(const class ErrorStacktrace::Frame *, mydomain::logdevice::Status)'
| | | `-DeclRefExpr 0x227760b8 'folly::Unexpected
(const class ErrorStacktrace::Frame *, mydomain::logdevice::Status)' lvalue
Function 0x18b49568 'makeUnexpected' 'folly::Unexpected (const class
ErrorStacktrace::Frame *, mydomain::logdevice::Status)'
| | |-UnaryOperator 0x22771fb8 'const
mydomain::logdevice::class ErrorStacktrace::Frame *' prefix '&' cannot overflow
| | | `-DeclRefExpr 0x22771f68 'const
mydomain::logdevice::class ErrorStacktrace::Frame':'const struct
mydomain::logdevice::ErrorStacktrace::Frame' lvalue Var 0x22771c50 'frame'
'const mydomain::logdevice::class ErrorStacktrace::Frame':'const struct
mydomain::logdevice::ErrorStacktrace::Frame'
| | `-CallExpr 0x227720e0 'mydomain::logdevice::Status':'enum
mydomain::logdevice::E'
| | |-ImplicitCastExpr 0x227720c8
'mydomain::logdevice::Status (*)(int)'
| | | `-DeclRefExpr 0x22771ff8 'mydomain::logdevice::Status
(int)' lvalue CXXMethod 0x221cddd0 'toStatus' 'mydomain::logdevice::Status
(int)'
| | `-ImplicitCastExpr 0x22772108 'int'
| | `-MemberExpr 0x22772038 'int' lvalue .rc_ 0x21f24480
| | `-DeclRefExpr 0x22772018 'struct
mydomain::logdevice::zk::GetResponse':'struct
mydomain::logdevice::zk::GetResponse' lvalue Var 0x22731508 'result' 'struct
mydomain::logdevice::zk::GetResponse':'struct
mydomain::logdevice::zk::GetResponse'
| `-ExprWithCleanups 0x227b13b8 'void'
| `-CXXMemberCallExpr 0x227b1378 'void'
| |-MemberExpr 0x227b1330 ''
.return_value 0x227b1230
| | `-DeclRefExpr 0x22776238 'std::__coroutine_traits_impl >
>::promise_type':'class folly::coro::detail::TaskPromise >' lvalue Var 0x227330a0 '__promise'
'std::__coroutine_traits_impl > >::promise_type':'class
folly::coro::detail::TaskPromise >'
| `-MaterializeTemporaryExpr 0x227b13a0
'folly::Unexpected':'class folly::Unexpected' xvalue
| `-CXXBindTemporaryExpr 0x22776218
'folly::Unexpected':'class folly::Unexpected' (CXXTemporary 0x22776218)
| `-StmtExpr 0x227761f0
[PATCH] D43477: [CFG] [analyzer] Add MaterializeTemporaryExpr into the construction context.
This revision was automatically updated to reflect the committed changes.
Closed by commit rC326014: [CFG] Provide construction contexts for
lifetime-extended temporaries. (authored by dergachev, committed by ).
Repository:
rC Clang
https://reviews.llvm.org/D43477
Files:
include/clang/Analysis/CFG.h
lib/Analysis/CFG.cpp
test/Analysis/cfg-rich-constructors.cpp
test/Analysis/temp-obj-dtors-cfg-output.cpp
Index: lib/Analysis/CFG.cpp
===
--- lib/Analysis/CFG.cpp
+++ lib/Analysis/CFG.cpp
@@ -548,6 +548,8 @@
Stmt *Term,
CFGBlock *TrueBlock,
CFGBlock *FalseBlock);
+ CFGBlock *VisitMaterializeTemporaryExpr(MaterializeTemporaryExpr *MTE,
+ AddStmtChoice asc);
CFGBlock *VisitMemberExpr(MemberExpr *M, AddStmtChoice asc);
CFGBlock *VisitObjCAtCatchStmt(ObjCAtCatchStmt *S);
CFGBlock *VisitObjCAtSynchronizedStmt(ObjCAtSynchronizedStmt *S);
@@ -1840,6 +1842,10 @@
case Stmt::LambdaExprClass:
return VisitLambdaExpr(cast(S), asc);
+case Stmt::MaterializeTemporaryExprClass:
+ return VisitMaterializeTemporaryExpr(cast(S),
+ asc);
+
case Stmt::MemberExprClass:
return VisitMemberExpr(cast(S), asc);
@@ -2975,6 +2981,16 @@
return EntryConditionBlock;
}
+CFGBlock *
+CFGBuilder::VisitMaterializeTemporaryExpr(MaterializeTemporaryExpr *MTE,
+ AddStmtChoice asc) {
+ findConstructionContexts(
+ ConstructionContext::create(cfg->getBumpVectorContext(), MTE),
+ MTE->getTemporary());
+
+ return VisitStmt(MTE, asc);
+}
+
CFGBlock *CFGBuilder::VisitMemberExpr(MemberExpr *M, AddStmtChoice asc) {
if (asc.alwaysAdd(*this, M)) {
autoCreateBlock();
@@ -4706,13 +4722,20 @@
} else if (const CXXConstructExpr *CCE = dyn_cast(S)) {
OS << " (CXXConstructExpr, ";
if (Optional CE = E.getAs()) {
+// TODO: Refactor into ConstructionContext::print().
if (const Stmt *S = CE->getTriggerStmt())
- Helper.handledStmt((const_cast(S)), OS);
+ Helper.handledStmt(const_cast(S), OS);
else if (const CXXCtorInitializer *I = CE->getTriggerInit())
print_initializer(OS, Helper, I);
else
llvm_unreachable("Unexpected trigger kind!");
OS << ", ";
+if (const Stmt *S = CE->getMaterializedTemporary()) {
+ if (S != CE->getTriggerStmt()) {
+Helper.handledStmt(const_cast(S), OS);
+OS << ", ";
+ }
+}
}
OS << CCE->getType().getAsString() << ")";
} else if (const CastExpr *CE = dyn_cast(S)) {
Index: include/clang/Analysis/CFG.h
===
--- include/clang/Analysis/CFG.h
+++ include/clang/Analysis/CFG.h
@@ -193,6 +193,17 @@
return Trigger.dyn_cast();
}
+ const MaterializeTemporaryExpr *getMaterializedTemporary() const {
+// TODO: Be more careful to ensure that there's only one MTE around.
+for (const ConstructionContext *CC = this; CC; CC = CC->getParent()) {
+ if (const auto *MTE = dyn_cast_or_null(
+ CC->getTriggerStmt())) {
+return MTE;
+ }
+}
+return nullptr;
+ }
+
bool isSameAsPartialContext(const ConstructionContext *Other) const {
assert(Other);
return (Trigger == Other->Trigger);
@@ -248,6 +259,10 @@
return getConstructionContext()->getTriggerInit();
}
+ const MaterializeTemporaryExpr *getMaterializedTemporary() const {
+return getConstructionContext()->getMaterializedTemporary();
+ }
+
private:
friend class CFGElement;
Index: test/Analysis/temp-obj-dtors-cfg-output.cpp
===
--- test/Analysis/temp-obj-dtors-cfg-output.cpp
+++ test/Analysis/temp-obj-dtors-cfg-output.cpp
@@ -714,7 +714,9 @@
// CHECK: Preds (1): B3
// CHECK: Succs (1): B0
// CHECK: [B3]
-// CHECK: 1: D() (CXXConstructExpr, struct D)
+// WARNINGS: 1: D() (CXXConstructExpr, struct D)
+// CXX98-ANALYZER: 1: D() (CXXConstructExpr, struct D)
+// CXX11-ANALYZER: 1: D() (CXXConstructExpr, [B3.2], struct D)
// CXX98: 2: [B3.1] (ImplicitCastExpr, NoOp, const struct D)
// CXX98: 3: [B3.2]
// CXX98-WARNINGS: 4: [B3.3] (CXXConstructExpr, struct D)
@@ -925,7 +927,8 @@
// CHECK: Succs (1): B4
// CHECK: [B7]
// WARNINGS: 1: A() (CXXConstructExpr, class A)
-// ANALYZER: 1: A() (CXXConstructExpr, [B7.2], class A)
+// ANALYZER-CXX98: 1: A() (CXXConstructExpr, [B7.2], [B7.3], class A)
+// ANALYZER-CXX11: 1: A() (CXXConstructExpr, [B7.2], class A)
// CHECK: 2: [B7.1] (BindTemporary)
// CXX98: 3: [B7.2].operator bool
//
[PATCH] D43477: [CFG] [analyzer] Add MaterializeTemporaryExpr into the construction context.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL326014: [CFG] Provide construction contexts for
lifetime-extended temporaries. (authored by dergachev, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D43477?vs=134957=135752#toc
Repository:
rL LLVM
https://reviews.llvm.org/D43477
Files:
cfe/trunk/include/clang/Analysis/CFG.h
cfe/trunk/lib/Analysis/CFG.cpp
cfe/trunk/test/Analysis/cfg-rich-constructors.cpp
cfe/trunk/test/Analysis/temp-obj-dtors-cfg-output.cpp
Index: cfe/trunk/lib/Analysis/CFG.cpp
===
--- cfe/trunk/lib/Analysis/CFG.cpp
+++ cfe/trunk/lib/Analysis/CFG.cpp
@@ -548,6 +548,8 @@
Stmt *Term,
CFGBlock *TrueBlock,
CFGBlock *FalseBlock);
+ CFGBlock *VisitMaterializeTemporaryExpr(MaterializeTemporaryExpr *MTE,
+ AddStmtChoice asc);
CFGBlock *VisitMemberExpr(MemberExpr *M, AddStmtChoice asc);
CFGBlock *VisitObjCAtCatchStmt(ObjCAtCatchStmt *S);
CFGBlock *VisitObjCAtSynchronizedStmt(ObjCAtSynchronizedStmt *S);
@@ -1840,6 +1842,10 @@
case Stmt::LambdaExprClass:
return VisitLambdaExpr(cast(S), asc);
+case Stmt::MaterializeTemporaryExprClass:
+ return VisitMaterializeTemporaryExpr(cast(S),
+ asc);
+
case Stmt::MemberExprClass:
return VisitMemberExpr(cast(S), asc);
@@ -2975,6 +2981,16 @@
return EntryConditionBlock;
}
+CFGBlock *
+CFGBuilder::VisitMaterializeTemporaryExpr(MaterializeTemporaryExpr *MTE,
+ AddStmtChoice asc) {
+ findConstructionContexts(
+ ConstructionContext::create(cfg->getBumpVectorContext(), MTE),
+ MTE->getTemporary());
+
+ return VisitStmt(MTE, asc);
+}
+
CFGBlock *CFGBuilder::VisitMemberExpr(MemberExpr *M, AddStmtChoice asc) {
if (asc.alwaysAdd(*this, M)) {
autoCreateBlock();
@@ -4706,13 +4722,20 @@
} else if (const CXXConstructExpr *CCE = dyn_cast(S)) {
OS << " (CXXConstructExpr, ";
if (Optional CE = E.getAs()) {
+// TODO: Refactor into ConstructionContext::print().
if (const Stmt *S = CE->getTriggerStmt())
- Helper.handledStmt((const_cast(S)), OS);
+ Helper.handledStmt(const_cast(S), OS);
else if (const CXXCtorInitializer *I = CE->getTriggerInit())
print_initializer(OS, Helper, I);
else
llvm_unreachable("Unexpected trigger kind!");
OS << ", ";
+if (const Stmt *S = CE->getMaterializedTemporary()) {
+ if (S != CE->getTriggerStmt()) {
+Helper.handledStmt(const_cast(S), OS);
+OS << ", ";
+ }
+}
}
OS << CCE->getType().getAsString() << ")";
} else if (const CastExpr *CE = dyn_cast(S)) {
Index: cfe/trunk/include/clang/Analysis/CFG.h
===
--- cfe/trunk/include/clang/Analysis/CFG.h
+++ cfe/trunk/include/clang/Analysis/CFG.h
@@ -193,6 +193,17 @@
return Trigger.dyn_cast();
}
+ const MaterializeTemporaryExpr *getMaterializedTemporary() const {
+// TODO: Be more careful to ensure that there's only one MTE around.
+for (const ConstructionContext *CC = this; CC; CC = CC->getParent()) {
+ if (const auto *MTE = dyn_cast_or_null(
+ CC->getTriggerStmt())) {
+return MTE;
+ }
+}
+return nullptr;
+ }
+
bool isSameAsPartialContext(const ConstructionContext *Other) const {
assert(Other);
return (Trigger == Other->Trigger);
@@ -248,6 +259,10 @@
return getConstructionContext()->getTriggerInit();
}
+ const MaterializeTemporaryExpr *getMaterializedTemporary() const {
+return getConstructionContext()->getMaterializedTemporary();
+ }
+
private:
friend class CFGElement;
Index: cfe/trunk/test/Analysis/temp-obj-dtors-cfg-output.cpp
===
--- cfe/trunk/test/Analysis/temp-obj-dtors-cfg-output.cpp
+++ cfe/trunk/test/Analysis/temp-obj-dtors-cfg-output.cpp
@@ -714,7 +714,9 @@
// CHECK: Preds (1): B3
// CHECK: Succs (1): B0
// CHECK: [B3]
-// CHECK: 1: D() (CXXConstructExpr, struct D)
+// WARNINGS: 1: D() (CXXConstructExpr, struct D)
+// CXX98-ANALYZER: 1: D() (CXXConstructExpr, struct D)
+// CXX11-ANALYZER: 1: D() (CXXConstructExpr, [B3.2], struct D)
// CXX98: 2: [B3.1] (ImplicitCastExpr, NoOp, const struct D)
// CXX98: 3: [B3.2]
// CXX98-WARNINGS: 4: [B3.3] (CXXConstructExpr, struct D)
@@ -925,7 +927,8 @@
// CHECK: Succs (1): B4
// CHECK: [B7]
// WARNINGS: 1: A() (CXXConstructExpr, class A)
-// ANALYZER: 1: A()
[PATCH] D43477: [CFG] [analyzer] Add MaterializeTemporaryExpr into the construction context.
dcoughlin accepted this revision. dcoughlin added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D43477 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43477: [CFG] [analyzer] Add MaterializeTemporaryExpr into the construction context.
NoQ added a comment.
> eg. `const C (123);` or the actual (not the elidable copy) constructor in
> `C foo() { return C(123); }`
Emm, sry, never mind, forget it, i was trying to say that the reason why we
don't have a `CXXBindTemporary` is because we don't have a destructor in class
`C`, not because we have specific syntax patterns.
This fix does not cause changes in the analyzer yet. Even though we are
providing construction contexts in a bit more cases, and even if we used them
in the analyzer, we wouldn't get any functional change yet, because temporary
constructors that require no destructors are inlined anyway, regardless of
construction context, and an exact same temporary region is accidentally
created for them. So the point of this patch is not to make more construction
contexts available, but to provide better construction contexts in the
`CXXBindTemporary` case, which will be used later.
Repository:
rC Clang
https://reviews.llvm.org/D43477
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43477: [CFG] [analyzer] Add MaterializeTemporaryExpr into the construction context.
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs.
`MaterializeTemporaryExpr` captures lifetime extension information. In the
analyzer it is important to have this information at construction site because
we would be dealing only with rvalue expressions for quite a while, but in
order to perform lifetime extension later, we need to put extra effort into
keeping the lvalue around.
`MaterializeTemporaryExpr` is added either as a trigger statement if there's no
`CXXBindTemporaryExpr` within it (eg. `const C (123);` or the actual (not the
elidable copy) constructor in `C foo() { return C(123); }`), or as a parent
construction context if there already is a `CXXBindTemporary`. In the latter
case, it is included in the dump and tested accordingly. I understand that this
is quite counter-intuitive at the moment (easy to construct, hard to use) and
i'd make super sure that `ConstructionContext` is refactored later to become
easy to understand and work with - separation of different "kinds" of
construction contexts and fancy kind-specific helper methods are absolutely
necessary. But for now i'm focused on getting it to work.
Repository:
rC Clang
https://reviews.llvm.org/D43477
Files:
include/clang/Analysis/CFG.h
lib/Analysis/CFG.cpp
test/Analysis/cfg-rich-constructors.cpp
test/Analysis/temp-obj-dtors-cfg-output.cpp
Index: test/Analysis/temp-obj-dtors-cfg-output.cpp
===
--- test/Analysis/temp-obj-dtors-cfg-output.cpp
+++ test/Analysis/temp-obj-dtors-cfg-output.cpp
@@ -714,7 +714,9 @@
// CHECK: Preds (1): B3
// CHECK: Succs (1): B0
// CHECK: [B3]
-// CHECK: 1: D() (CXXConstructExpr, struct D)
+// WARNINGS: 1: D() (CXXConstructExpr, struct D)
+// CXX98-ANALYZER: 1: D() (CXXConstructExpr, struct D)
+// CXX11-ANALYZER: 1: D() (CXXConstructExpr, [B3.2], struct D)
// CXX98: 2: [B3.1] (ImplicitCastExpr, NoOp, const struct D)
// CXX98: 3: [B3.2]
// CXX98-WARNINGS: 4: [B3.3] (CXXConstructExpr, struct D)
@@ -925,7 +927,8 @@
// CHECK: Succs (1): B4
// CHECK: [B7]
// WARNINGS: 1: A() (CXXConstructExpr, class A)
-// ANALYZER: 1: A() (CXXConstructExpr, [B7.2], class A)
+// ANALYZER-CXX98: 1: A() (CXXConstructExpr, [B7.2], [B7.3], class A)
+// ANALYZER-CXX11: 1: A() (CXXConstructExpr, [B7.2], class A)
// CHECK: 2: [B7.1] (BindTemporary)
// CXX98: 3: [B7.2].operator bool
// CXX98: 4: [B7.2]
@@ -992,7 +995,8 @@
// CHECK: 1: foo
// CHECK: 2: [B7.1] (ImplicitCastExpr, FunctionToPointerDecay, void (*)(const class A &))
// WARNINGS: 3: A() (CXXConstructExpr, class A)
-// ANALYZER: 3: A() (CXXConstructExpr, [B7.4], class A)
+// ANALYZER-CXX98: 3: A() (CXXConstructExpr, [B7.4], class A)
+// ANALYZER-CXX11: 3: A() (CXXConstructExpr, class A)
// CHECK: 4: [B7.3] (BindTemporary)
// CXX98: 5: [B7.4].operator bool
// CXX98: 6: [B7.4]
@@ -1043,7 +1047,8 @@
// CHECK: Succs (1): B9
// CHECK: [B12]
// WARNINGS: 1: A() (CXXConstructExpr, class A)
-// ANALYZER: 1: A() (CXXConstructExpr, [B12.2], class A)
+// ANALYZER-CXX98: 1: A() (CXXConstructExpr, [B12.2], [B12.3], class A)
+// ANALYZER-CXX11: 1: A() (CXXConstructExpr, [B12.2], class A)
// CHECK: 2: [B12.1] (BindTemporary)
// CXX98: 3: [B12.2].operator bool
// CXX98: 4: [B12.2]
@@ -1199,7 +1204,8 @@
// CHECK: [B2 (NORETURN)]
// CHECK: 1: int a;
// WARNINGS: 2: NoReturn() (CXXConstructExpr, class NoReturn)
-// ANALYZER: 2: NoReturn() (CXXConstructExpr, [B2.3], class NoReturn)
+// ANALYZER-CXX98: 2: NoReturn() (CXXConstructExpr, [B2.3], [B2.4], class NoReturn)
+// ANALYZER-CXX11: 2: NoReturn() (CXXConstructExpr, [B2.3], class NoReturn)
// CHECK: 3: [B2.2] (BindTemporary)
// CHECK: [[MEMBER:[45]]]: [B2.{{[34]}}].f
// CHECK: {{[56]}}: [B2.[[MEMBER]]]()
Index: test/Analysis/cfg-rich-constructors.cpp
===
--- test/Analysis/cfg-rich-constructors.cpp
+++ test/Analysis/cfg-rich-constructors.cpp
@@ -133,11 +133,10 @@
C c = coin ? C::get() : C(0);
}
-// TODO: Should find construction target here.
// CHECK: void referenceVariableWithConstructor()
// CHECK: 1: 0
// CHECK-NEXT: 2: [B1.1] (ImplicitCastExpr, NullToPointer, class C *)
-// CHECK-NEXT: 3: [B1.2] (CXXConstructExpr, const class C)
+// CHECK-NEXT: 3: [B1.2] (CXXConstructExpr, [B1.4], const class C)
// CHECK-NEXT: 4: [B1.3]
// CHECK-NEXT: 5: const C (0);
void referenceVariableWithConstructor() {
@@ -267,9 +266,8 @@
return {123, 456};
}
-// TODO: Should find construction targets for the first constructor as well.
// CHECK: C returnTemporary()
-// CHECK: 1: C() (CXXConstructExpr, class C)
+// CHECK: 1: C() (CXXConstructExpr,
