[PATCH] D47096: CodeGen: block capture shouldn't ICE
jfb closed this revision. jfb added a comment. r332801 Repository: rC Clang https://reviews.llvm.org/D47096 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47096: CodeGen: block capture shouldn't ICE
jfb added a comment. In https://reviews.llvm.org/D47096#1105492, @rjmccall wrote: > Test case should go in test/CodeGenCXX. Also, there already is a > `blocks.cpp` there. I moved it, but didn't merge with the existing block.cpp because it just checks for not crashing. I'd rather make sure that the checks don't inadvertently pick up the wrong test. Repository: rC Clang https://reviews.llvm.org/D47096 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47096: CodeGen: block capture shouldn't ICE
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D47096 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47096: CodeGen: block capture shouldn't ICE
jfb updated this revision to Diff 147648.
jfb added a comment.
- move test
Repository:
rC Clang
https://reviews.llvm.org/D47096
Files:
lib/CodeGen/CGDecl.cpp
test/CodeGenCXX/block-capture.cpp
Index: test/CodeGenCXX/block-capture.cpp
===
--- /dev/null
+++ test/CodeGenCXX/block-capture.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -x c++ -std=c++11 -fblocks -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: %struct.__block_byref_baz = type { i8*, %struct.__block_byref_baz*,
i32, i32, i32 }
+// CHECK: [[baz:%[0-9a-z_]*]] = alloca %struct.__block_byref_baz
+// CHECK: [[bazref:%[0-9a-z_\.]*]] = getelementptr inbounds
%struct.__block_byref_baz, %struct.__block_byref_baz* [[baz]], i32 0, i32 1
+// CHECK: store %struct.__block_byref_baz* [[baz]],
%struct.__block_byref_baz** [[bazref]]
+// CHECK: [[disposable:%[0-9a-z_]*]] = bitcast %struct.__block_byref_baz*
[[baz]] to i8*
+// CHECK: call void @_Block_object_dispose(i8* [[disposable]]
+
+int main() {
+ __block int baz = [&]() { return 0; }();
+ return 0;
+}
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1244,37 +1244,50 @@
return emission;
}
+static bool isCapturedBy(const VarDecl &, const Expr *);
+
+/// Determines whether the given __block variable is potentially
+/// captured by the given statement.
+static bool isCapturedBy(const VarDecl , const Stmt *S) {
+ if (const Expr *E = dyn_cast(S))
+return isCapturedBy(Var, E);
+ for (const Stmt *SubStmt : S->children())
+if (isCapturedBy(Var, SubStmt))
+ return true;
+ return false;
+}
+
/// Determines whether the given __block variable is potentially
/// captured by the given expression.
-static bool isCapturedBy(const VarDecl , const Expr *e) {
+static bool isCapturedBy(const VarDecl , const Expr *E) {
// Skip the most common kinds of expressions that make
// hierarchy-walking expensive.
- e = e->IgnoreParenCasts();
+ E = E->IgnoreParenCasts();
- if (const BlockExpr *be = dyn_cast(e)) {
-const BlockDecl *block = be->getBlockDecl();
-for (const auto : block->captures()) {
- if (I.getVariable() == )
+ if (const BlockExpr *BE = dyn_cast(E)) {
+const BlockDecl *Block = BE->getBlockDecl();
+for (const auto : Block->captures()) {
+ if (I.getVariable() == )
return true;
}
// No need to walk into the subexpressions.
return false;
}
- if (const StmtExpr *SE = dyn_cast(e)) {
+ if (const StmtExpr *SE = dyn_cast(E)) {
const CompoundStmt *CS = SE->getSubStmt();
for (const auto *BI : CS->body())
- if (const auto *E = dyn_cast(BI)) {
-if (isCapturedBy(var, E))
-return true;
+ if (const auto *BIE = dyn_cast(BI)) {
+if (isCapturedBy(Var, BIE))
+ return true;
}
else if (const auto *DS = dyn_cast(BI)) {
// special case declarations
for (const auto *I : DS->decls()) {
if (const auto *VD = dyn_cast((I))) {
const Expr *Init = VD->getInit();
-if (Init && isCapturedBy(var, Init))
+if (Init && isCapturedBy(Var, Init))
return true;
}
}
@@ -1286,8 +1299,8 @@
return false;
}
- for (const Stmt *SubStmt : e->children())
-if (isCapturedBy(var, cast(SubStmt)))
+ for (const Stmt *SubStmt : E->children())
+if (isCapturedBy(Var, SubStmt))
return true;
return false;
Index: test/CodeGenCXX/block-capture.cpp
===
--- /dev/null
+++ test/CodeGenCXX/block-capture.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -x c++ -std=c++11 -fblocks -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: %struct.__block_byref_baz = type { i8*, %struct.__block_byref_baz*, i32, i32, i32 }
+// CHECK: [[baz:%[0-9a-z_]*]] = alloca %struct.__block_byref_baz
+// CHECK: [[bazref:%[0-9a-z_\.]*]] = getelementptr inbounds %struct.__block_byref_baz, %struct.__block_byref_baz* [[baz]], i32 0, i32 1
+// CHECK: store %struct.__block_byref_baz* [[baz]], %struct.__block_byref_baz** [[bazref]]
+// CHECK: [[disposable:%[0-9a-z_]*]] = bitcast %struct.__block_byref_baz* [[baz]] to i8*
+// CHECK: call void @_Block_object_dispose(i8* [[disposable]]
+
+int main() {
+ __block int baz = [&]() { return 0; }();
+ return 0;
+}
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1244,37 +1244,50 @@
return emission;
}
+static bool isCapturedBy(const VarDecl &, const Expr *);
+
+/// Determines whether the given __block variable is potentially
+/// captured by the given statement.
+static bool isCapturedBy(const VarDecl , const Stmt *S) {
+ if (const Expr *E = dyn_cast(S))
+return isCapturedBy(Var, E);
+ for (const Stmt *SubStmt :
[PATCH] D47096: CodeGen: block capture shouldn't ICE
rjmccall added a comment. Test case should go in test/CodeGenCXX. Also, there already is a `blocks.cpp` there. Repository: rC Clang https://reviews.llvm.org/D47096 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47096: CodeGen: block capture shouldn't ICE
jfb added a comment. In https://reviews.llvm.org/D47096#1105455, @rjmccall wrote: > `children()` is actually defined at the `Stmt` level, and if you look at how > it's implemented on e.g. `IfStmt`, you can see that it visits all of the > child `Stmt`s, including the if-condition. So it should be fine. Thanks for pointing this out! I was reading the code but missed that, and visitor seemed like the only reliable way to do what I wanted. Here's an update patch. Repository: rC Clang https://reviews.llvm.org/D47096 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47096: CodeGen: block capture shouldn't ICE
jfb updated this revision to Diff 147647.
jfb added a comment.
- Follow John's suggestion.
Repository:
rC Clang
https://reviews.llvm.org/D47096
Files:
lib/CodeGen/CGDecl.cpp
test/CodeGen/block-capture.cpp
Index: test/CodeGen/block-capture.cpp
===
--- /dev/null
+++ test/CodeGen/block-capture.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -x c++ -std=c++11 -fblocks -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: %struct.__block_byref_baz = type { i8*, %struct.__block_byref_baz*,
i32, i32, i32 }
+// CHECK: [[baz:%[0-9a-z_]*]] = alloca %struct.__block_byref_baz
+// CHECK: [[bazref:%[0-9a-z_\.]*]] = getelementptr inbounds
%struct.__block_byref_baz, %struct.__block_byref_baz* [[baz]], i32 0, i32 1
+// CHECK: store %struct.__block_byref_baz* [[baz]],
%struct.__block_byref_baz** [[bazref]]
+// CHECK: [[disposable:%[0-9a-z_]*]] = bitcast %struct.__block_byref_baz*
[[baz]] to i8*
+// CHECK: call void @_Block_object_dispose(i8* [[disposable]]
+
+int main() {
+ __block int baz = [&]() { return 0; }();
+ return 0;
+}
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1244,37 +1244,50 @@
return emission;
}
+static bool isCapturedBy(const VarDecl &, const Expr *);
+
+/// Determines whether the given __block variable is potentially
+/// captured by the given statement.
+static bool isCapturedBy(const VarDecl , const Stmt *S) {
+ if (const Expr *E = dyn_cast(S))
+return isCapturedBy(Var, E);
+ for (const Stmt *SubStmt : S->children())
+if (isCapturedBy(Var, SubStmt))
+ return true;
+ return false;
+}
+
/// Determines whether the given __block variable is potentially
/// captured by the given expression.
-static bool isCapturedBy(const VarDecl , const Expr *e) {
+static bool isCapturedBy(const VarDecl , const Expr *E) {
// Skip the most common kinds of expressions that make
// hierarchy-walking expensive.
- e = e->IgnoreParenCasts();
+ E = E->IgnoreParenCasts();
- if (const BlockExpr *be = dyn_cast(e)) {
-const BlockDecl *block = be->getBlockDecl();
-for (const auto : block->captures()) {
- if (I.getVariable() == )
+ if (const BlockExpr *BE = dyn_cast(E)) {
+const BlockDecl *Block = BE->getBlockDecl();
+for (const auto : Block->captures()) {
+ if (I.getVariable() == )
return true;
}
// No need to walk into the subexpressions.
return false;
}
- if (const StmtExpr *SE = dyn_cast(e)) {
+ if (const StmtExpr *SE = dyn_cast(E)) {
const CompoundStmt *CS = SE->getSubStmt();
for (const auto *BI : CS->body())
- if (const auto *E = dyn_cast(BI)) {
-if (isCapturedBy(var, E))
-return true;
+ if (const auto *BIE = dyn_cast(BI)) {
+if (isCapturedBy(Var, BIE))
+ return true;
}
else if (const auto *DS = dyn_cast(BI)) {
// special case declarations
for (const auto *I : DS->decls()) {
if (const auto *VD = dyn_cast((I))) {
const Expr *Init = VD->getInit();
-if (Init && isCapturedBy(var, Init))
+if (Init && isCapturedBy(Var, Init))
return true;
}
}
@@ -1286,8 +1299,8 @@
return false;
}
- for (const Stmt *SubStmt : e->children())
-if (isCapturedBy(var, cast(SubStmt)))
+ for (const Stmt *SubStmt : E->children())
+if (isCapturedBy(Var, SubStmt))
return true;
return false;
Index: test/CodeGen/block-capture.cpp
===
--- /dev/null
+++ test/CodeGen/block-capture.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -x c++ -std=c++11 -fblocks -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: %struct.__block_byref_baz = type { i8*, %struct.__block_byref_baz*, i32, i32, i32 }
+// CHECK: [[baz:%[0-9a-z_]*]] = alloca %struct.__block_byref_baz
+// CHECK: [[bazref:%[0-9a-z_\.]*]] = getelementptr inbounds %struct.__block_byref_baz, %struct.__block_byref_baz* [[baz]], i32 0, i32 1
+// CHECK: store %struct.__block_byref_baz* [[baz]], %struct.__block_byref_baz** [[bazref]]
+// CHECK: [[disposable:%[0-9a-z_]*]] = bitcast %struct.__block_byref_baz* [[baz]] to i8*
+// CHECK: call void @_Block_object_dispose(i8* [[disposable]]
+
+int main() {
+ __block int baz = [&]() { return 0; }();
+ return 0;
+}
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -1244,37 +1244,50 @@
return emission;
}
+static bool isCapturedBy(const VarDecl &, const Expr *);
+
+/// Determines whether the given __block variable is potentially
+/// captured by the given statement.
+static bool isCapturedBy(const VarDecl , const Stmt *S) {
+ if (const Expr *E = dyn_cast(S))
+return isCapturedBy(Var, E);
+ for (const Stmt *SubStmt :
[PATCH] D47096: CodeGen: block capture shouldn't ICE
rjmccall added a comment. In https://reviews.llvm.org/D47096#1105374, @jfb wrote: > In https://reviews.llvm.org/D47096#1105368, @rjmccall wrote: > > > RecursiveASTVisitor instantiations are huge. Can you just make the > > function take a Stmt and then do the first few checks if it happens to be > > an Expr? > > > I'm not super-familiar with the code, so I might be doing something silly. > > I did something like this initially (leave the top of the function as-is, and > instead of cast do dyn_cast to Expr and if that fails to CompoundStmt, > recursively iterating all the children of the CompoundStmt). My worry was > that I wasn't traversing all actual children (just CompountStmt's children), > and AFAICT there's no easy way to say "take any Stmt, and visit its children > if it has such a method". I could hard-code more Stmt derivatives but that > seems brittle, I could use the "detection idiom" but that's silly if there's > already a visitor which does The Right Thing through tablegen magic. > > What I can do is what I did earlier, and conservatively say it was captured > if it's neither an Expr nor a CompoundStmt? Or should I special-case other > things as well? `children()` is actually defined at the `Stmt` level, and if you look at how it's implemented on e.g. `IfStmt`, you can see that it visits all of the child `Stmt`s, including the if-condition. So it should be fine. Repository: rC Clang https://reviews.llvm.org/D47096 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47096: CodeGen: block capture shouldn't ICE
jfb added a comment. In https://reviews.llvm.org/D47096#1105368, @rjmccall wrote: > RecursiveASTVisitor instantiations are huge. Can you just make the function > take a Stmt and then do the first few checks if it happens to be an Expr? I'm not super-familiar with the code, so I might be doing something silly. I did something like this initially (leave the top of the function as-is, and instead of cast do dyn_cast to Expr and if that fails to CompoundStmt, recursively iterating all the children of the CompoundStmt). My worry was that I wasn't traversing all actual children (just CompountStmt's children), and AFAICT there's no easy way to say "take any Stmt, and visit its children if it has such a method". I could hard-code more Stmt derivatives but that seems brittle, I could use the "detection idiom" but that's silly if there's already a visitor which does The Right Thing through tablegen magic. What I can do is what I did earlier, and conservatively say it was captured if it's neither an Expr nor a CompoundStmt? Or should I special-case other things as well? Repository: rC Clang https://reviews.llvm.org/D47096 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47096: CodeGen: block capture shouldn't ICE
jfb created this revision.
jfb added a reviewer: rjmccall.
Herald added subscribers: cfe-commits, aheejin.
When a lambda capture captures a __block in the same statement, the compiler
asserts out because isCapturedBy assumes that an Expr can only be a BlockExpr,
StmtExpr, or if it's a Stmt then all the statement's children are expressions.
That's wrong, we need to visit all sub-statements even if they're not
expressions to see if they also capture.
Fix this issue by pulling out the isCapturedBy logic to use RecursiveASTVisitor.
rdar://problem/39926584
Repository:
rC Clang
https://reviews.llvm.org/D47096
Files:
lib/CodeGen/CGDecl.cpp
test/CodeGen/block-capture.cpp
Index: test/CodeGen/block-capture.cpp
===
--- /dev/null
+++ test/CodeGen/block-capture.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -x c++ -std=c++11 -fblocks -emit-llvm %s -o - | FileCheck %s
+
+// CHECK: %struct.__block_byref_baz = type { i8*, %struct.__block_byref_baz*, i32, i32, i32 }
+// CHECK: [[baz:%[0-9a-z_]*]] = alloca %struct.__block_byref_baz
+// CHECK: [[bazref:%[0-9a-z_\.]*]] = getelementptr inbounds %struct.__block_byref_baz, %struct.__block_byref_baz* [[baz]], i32 0, i32 1
+// CHECK: store %struct.__block_byref_baz* [[baz]], %struct.__block_byref_baz** [[bazref]]
+// CHECK: [[disposable:%[0-9a-z_]*]] = bitcast %struct.__block_byref_baz* [[baz]] to i8*
+// CHECK: call void @_Block_object_dispose(i8* [[disposable]]
+
+int main() {
+ __block int baz = [&]() { return 0; }();
+ return 0;
+}
Index: lib/CodeGen/CGDecl.cpp
===
--- lib/CodeGen/CGDecl.cpp
+++ lib/CodeGen/CGDecl.cpp
@@ -26,6 +26,7 @@
#include "clang/AST/Decl.h"
#include "clang/AST/DeclObjC.h"
#include "clang/AST/DeclOpenMP.h"
+#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Basic/TargetInfo.h"
#include "clang/CodeGen/CGFunctionInfo.h"
@@ -1244,53 +1245,65 @@
return emission;
}
-/// Determines whether the given __block variable is potentially
-/// captured by the given expression.
-static bool isCapturedBy(const VarDecl , const Expr *e) {
- // Skip the most common kinds of expressions that make
- // hierarchy-walking expensive.
- e = e->IgnoreParenCasts();
+static bool isCapturedBy(const VarDecl &, const Expr *);
- if (const BlockExpr *be = dyn_cast(e)) {
-const BlockDecl *block = be->getBlockDecl();
-for (const auto : block->captures()) {
- if (I.getVariable() == )
-return true;
-}
+class IsCapturedBy : public RecursiveASTVisitor {
+ bool IsCaptured = false;
+ const VarDecl
+ bool setCaptured() {
+IsCaptured = true;
+// End visitation early, we know it's captured.
+return false;
+ }
+public:
+ IsCapturedBy(const VarDecl ) : Var(Var) {}
+ bool isCaptured() const { return IsCaptured; }
+
+ bool TraverseBlockExpr(BlockExpr *BE) {
+const BlockDecl *Block = BE->getBlockDecl();
+for (const auto : Block->captures())
+ if (I.getVariable() == )
+return setCaptured();
// No need to walk into the subexpressions.
return false;
}
- if (const StmtExpr *SE = dyn_cast(e)) {
+ bool TraverseStmtExpr(StmtExpr *SE) {
const CompoundStmt *CS = SE->getSubStmt();
for (const auto *BI : CS->body())
if (const auto *E = dyn_cast(BI)) {
-if (isCapturedBy(var, E))
-return true;
- }
- else if (const auto *DS = dyn_cast(BI)) {
- // special case declarations
- for (const auto *I : DS->decls()) {
- if (const auto *VD = dyn_cast((I))) {
-const Expr *Init = VD->getInit();
-if (Init && isCapturedBy(var, Init))
- return true;
- }
+if (isCapturedBy(Var, E))
+ return setCaptured();
+ } else if (const auto *DS = dyn_cast(BI)) {
+// special case declarations
+for (const auto *I : DS->decls()) {
+ if (const auto *VD = dyn_cast((I))) {
+const Expr *Init = VD->getInit();
+if (Init && isCapturedBy(Var, Init))
+ return setCaptured();
}
+}
}
else
-// FIXME. Make safe assumption assuming arbitrary statements cause capturing.
-// Later, provide code to poke into statements for capture analysis.
-return true;
-return false;
+// FIXME. Make safe assumption assuming arbitrary statements cause
+// capturing. Later, provide code to poke into statements for capture
+// analysis.
+return setCaptured();
+return true;
}
+};
- for (const Stmt *SubStmt : e->children())
-if (isCapturedBy(var, cast(SubStmt)))
- return true;
+/// Determines whether the given __block variable is potentially
+/// captured by the given expression.
+static bool isCapturedBy(const VarDecl , const Expr *E) {
+ // Skip the most common
