Page MenuHomePhabricator

Block+lambda: allow reference capture
Needs ReviewPublic

Authored by ahatanak on Feb 12 2019, 9:34 PM.



Capturing a C++ object by reference wasn't quite working when mixing block and lambda.


Diff Detail

rC Clang

Event Timeline

jfb created this revision.Feb 12 2019, 9:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2019, 9:34 PM

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.

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'
jfb updated this revision to Diff 186752.Feb 13 2019, 3:07 PM
  • Check for references when looking for copyexpr directly.
jfb added a comment.Feb 13 2019, 3:07 PM

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.

jfb edited the summary of this revision. (Show Details)Feb 14 2019, 12:48 PM
ahatanak accepted this revision.Feb 14 2019, 9:31 PM

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.

1 ↗(On Diff #186752)

I think you can simplify the test case a bit more:

  • remove -fcxx-exceptions.
  • remove 'extern "C"'.
  • remove the derp's move constructor and destructor.
  • remove the try/catch and just call 'c.cancel' in the function.
This revision is now accepted and ready to land.Feb 14 2019, 9:31 PM
ahatanak requested changes to this revision.Feb 14 2019, 9:33 PM

Sorry, I didn't mean to accept this yet. I think this is something that is better fixed in Sema.

This revision now requires changes to proceed.Feb 14 2019, 9:33 PM

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.

ahatanak commandeered this revision.Jun 14 2019, 4:48 PM
ahatanak edited reviewers, added: jfb; removed: ahatanak.
ahatanak updated this revision to Diff 204882.Jun 14 2019, 4:51 PM
  • 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.

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/ 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().

Richard, could you shed light on why it's done this way?

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?

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.

Okay. In that case, yeah, it'd be good to get Richard's opinion about the right way to get that information here.

ahatanak updated this revision to Diff 212016.Jul 26 2019, 3:35 PM


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.