Capturing a C++ object by reference wasn't quite working when mixing block and lambda.
rdar://problem/47550338
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
The code is crashing here because the loop in computeBlockInfo is trying to capture a variable that is captured by reference by the enclosing lambda as if it were captured by value. This is the type of VT:
LValueReferenceType 0x1138008c0 'struct derp &' `-RecordType 0x113052580 'struct derp' `-CXXRecord 0x1130524e8 'derp'
So in this case, the type doesn't require non-trivial copy construction.
Comment Actions
I think the root of the problem is that BlockDecl knows the captured variable but doesn't know the type of the capture. The type of the variable and the type of the capture can be different if the block is nested inside a lambda or another block, which is why CI.hasCopyExpr() can return a non-null value when VT is a lvalue reference:
`-FunctionDecl 0x11306dc28 <line:11:1, line:23:1> line:11:6 test 'void ()'
|-DeclStmt 0x11306ddb0 <line:12:3, col:9>
| `-VarDecl 0x11306dd20 <col:3, col:8> col:8 used c 'derp' callinit
...
| |-CXXRecordDecl 0x11306dec0 <line:14:12> col:12 implicit class definition
...
| | |-FieldDecl 0x11309cc58 <line:16:7> col:7 implicit 'derp &'
...
| | | | `-BlockDecl 0x11309cb28 <line:15:18, line:17:5> line:15:18
| | | | |-capture nested Var 0x11306dd20 'c' 'derp'Comment Actions
I talked to Akira in meatspace, and it seems like this updated patch does the right thing. He suggested changing the AST as a longer-term solution, but for now this approach seems simple enough.
Comment Actions
Sorry, I was misunderstanding the problem.
I was trying to understand why the crash goes away if I change the generic lambda to a non-generic one. What I found was that, when the lambda is generic, captureInBlock is passed a CaptureType that isn't an lvalue reference type and therefore performs copy initialization and sets the copy expression of the Capture to a non-null expression. This happens because isVariableAlreadyCapturedInScopeInfo doesn't assign the correct type (which should be an LValueReferenceType) to CaptureType when the variable is captured by reference.
I think this can be fixed if we check whether CaptureType is a reference capture in isVariableAlreadyCapturedInScopeInfo and, if it is, turn it into an lvalue reference type.
| test/CodeGenCXX/lambda-capturing-block.cpp | ||
|---|---|---|
| 1 ↗ | (On Diff #186752) | I think you can simplify the test case a bit more:
|
Comment Actions
Sorry, I didn't mean to accept this yet. I think this is something that is better fixed in Sema.
Comment Actions
I agree. There should just never be a copy expression if the capture is of reference type, and there should therefore be no need to special-case that in IRGen.
Comment Actions
- Add another test case which has a block nested inside a lambda and causes clang to crash.
- Fix the capture type passed to addCapture in RebuildLambdaScopeInfo.
Comment Actions
I think I now have a better idea of what's causing the crash in IRGen.
The root of the problem is that, when RebuildLambdaScopeInfo is called to rebuild the scope info for the generic lambda, the type of the captured variable (s in test2 and test3 in test/CodeGenObjCXX/block-nested-in-lambda.mm) is passed to addCapture, which is different from the type of the non-static member of the closure object. For example, the member type of the generic lambda in test2 is S& whereas the type of variable s is S. Sema::ActOnBlockStmtExpr in SemaExpr.cpp uses the type passed to addCapture to determine whether copying from the lambda member to the field in the block structure requires a copy constructor, but since it isn't passed the correct type, it incorrectly determines that a copy constructor is needed when the capture is a by-reference capture (for example, in test2) and isn't needed when the capture is a by-copy capture (for example, in test3), which causes crashes in IRGen.
I think the fix is to pass the correct capture type, which I think is whatever I->getType() returns, to addCapture in RebuildLambdaScopeInfo, but I'm wondering whether there is a reason VD->getType() has to be called here when the other two cases (this and VLA) uses I->getType().
Comment Actions
I'm not sure we've ever written down what the semantics of the block capture are actually supposed to be in this situation. I guess capturing a reference is the right thing to do? Is that what we generally do if this is a POD type?
Comment Actions
Currently a block captures a variable (POD or non-POD) by reference if the enclosing lambda captures it by reference and captures by copy if the enclosing lambda captures by copy.
Comment Actions
Okay. In that case, yeah, it'd be good to get Richard's opinion about the right way to get that information here.
Comment Actions
Rebase.
I think the fix is correct. When the lambda expression for the generic lambda is built, BuildLambdaExpr passes a Capture object in LambdaScopeInfo::Captures to BuildCaptureField to build the closure class field for the capture, so it should be okay to read the type of the field and pass it to LambdaScopeInfo::addCapture when rebuilding the lambda scope info.
Comment Actions
Okay, I think this is a reasonable fix. If @rsmith has thoughts about this, I guess they'll have to happen post-review.