[PATCH] D51564: Distinguish `__block` variables that are captured by escaping blocks from those that aren't

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.


================
Comment at: include/clang/AST/Decl.h:977
+
+    unsigned EscapingByref : 1;
   };
----------------
rjmccall wrote:
> This doesn't actually seem to be initialized anywhere.
VarDecl's constructor in Decl.cpp initializes it to false.

```
  AllBits = 0;
  VarDeclBits.SClass = SC;
  // Everything else is implicitly initialized to false.
```


================
Comment at: lib/CodeGen/CGBlocks.cpp:497
+  return VD->isNonEscapingByref() ?
+         CGF.getContext().getLValueReferenceType(VD->getType()) : VD->getType();
 }
----------------
rjmccall wrote:
> Do you need to worry about the variable already having a reference type?
Yes, I found out that the previous patch didn't handle `__block` variables with reference types correctly and caused assertion failures. To fix this, I made changes in computeBlockInfo so that the `__block` qualifier is ignored if the variable already has a reference type (I'm treating `__block T&` as `T&` ). There are no changes to this function.


================
Comment at: lib/CodeGen/CGBlocks.cpp:1310
 
-  if (capture.fieldType()->isReferenceType())
+  if (capture.fieldType()->isReferenceType() || variable->isNonEscapingByref())
     addr = EmitLoadOfReference(MakeAddrLValue(addr, capture.fieldType()));
----------------
rjmccall wrote:
> Isn't the field type already a reference type in this case?
> 
> You should leave a comment that you're relying on that, of course.
I added an assertion that checks non-escaping variable fields have reference types.


================
Comment at: lib/Sema/Sema.cpp:1451
+  // Call this function before popping the current function scope.
+  markEscapingByrefs(*FunctionScopes.back(), *this);
+
----------------
rjmccall wrote:
> Why before?
I remember I had to move the call to markEscapingByrefs to this place because it was making clang crash. It crashed when markEscapingByrefs triggered a call to PushFunctionScope, which caused clearing out PreallocatedFunctionScope, but I can't reproduce the crash anymore. I don't think markEscapingByrefs can cause PushFunctionScope to be called, so I'm moving the call back to the original location.


Repository:
  rC Clang

https://reviews.llvm.org/D51564