Page MenuHomePhabricator

CodeGen: block capture shouldn't ICE
ClosedPublic

Authored by jfb on May 18 2018, 4:26 PM.

Details

Summary

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

Diff Detail

Event Timeline

jfb created this revision.May 18 2018, 4:26 PM

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?

jfb added a comment.May 18 2018, 4:51 PM

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?

In D47096#1105374, @jfb 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 Stmts, including the if-condition. So it should be fine.

jfb updated this revision to Diff 147647.May 18 2018, 9:05 PM
  • Follow John's suggestion.
jfb added a comment.May 18 2018, 9:06 PM

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 Stmts, 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.

Test case should go in test/CodeGenCXX. Also, there already is a blocks.cpp there.

jfb updated this revision to Diff 147648.May 18 2018, 9:22 PM
  • move test
jfb added a comment.May 18 2018, 9:23 PM

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.

rjmccall accepted this revision.May 18 2018, 9:23 PM

LGTM.

This revision is now accepted and ready to land.May 18 2018, 9:23 PM