Variable auto-init of blocks capturing self after init bugfix

[jfb]

Summary

Blocks that capture themselves (and escape) after initialization currently codegen wrong because this:

bool capturedByInit =
    Init && emission.IsEscapingByRef && isCapturedBy(D, Init);

Address Loc =
    capturedByInit ? emission.Addr : emission.getObjectAddress(*this);

Already adjusts Loc from thr alloca to a GEP. This code:

if (emission.IsEscapingByRef)
  Loc = emitBlockByrefAddress(Loc, &D, /*follow=*/false);

Was trying to do the same adjustment, and a GEP on a GEP (returning an int) triggers an assertion.

rdar://problem/47943027

[rjmccall]

lib/CodeGen/CGDecl.cpp

1640 ↗

(On Diff #186776)

This seems like something we're likely regret eventually. If we've already drilled down to the storage, we should propagate that information into this rather than trying to detect it retroactively here.

[jfb]

lib/CodeGen/CGDecl.cpp

1640 ↗

(On Diff #186776)

You mean that I should check !capturedByInit instead? I initially did that, but I was worried about someone adding other drill-downs and then going out of sync. My rationale was: you always start with alloca, so anything else means a drill-down occurred.

That being said, this isn't a strongly held belief. Happy to go your way if you think it's better, you know this code more than I do :)

[rjmccall]

Well, you can always make a variable with a more directly-applicable name than capturedByInit and update it as appropriate, like locIsByrefHeader.

[jfb]

• Update with John's suggestion.

[jfb]

In D58218#1398096, @rjmccall wrote:

Well, you can always make a variable with a more directly-applicable name than capturedByInit and update it as appropriate, like locIsByrefHeader.

Sounds good. I made it const too, to avoid inadvertent modification.

[rjmccall]

In D58218#1398124, @jfb wrote:

In D58218#1398096, @rjmccall wrote:

Well, you can always make a variable with a more directly-applicable name than capturedByInit and update it as appropriate, like locIsByrefHeader.

Sounds good. I made it const too, to avoid inadvertent modification.

This is actually the reverse of the sense I would expect it to have based on the name. If you want these semantics, maybe locIsObject?

[jfb]

• Fix polarity

[jfb]

In D58218#1398251, @rjmccall wrote:

In D58218#1398124, @jfb wrote:

In D58218#1398096, @rjmccall wrote:

Well, you can always make a variable with a more directly-applicable name than capturedByInit and update it as appropriate, like locIsByrefHeader.

Sounds good. I made it const too, to avoid inadvertent modification.

This is actually the reverse of the sense I would expect it to have based on the name. If you want these semantics, maybe locIsObject?

You're right, I've fixed the polarity.

[rjmccall]

LGTM.

cfe/trunk/lib/CodeGen/CGDecl.cpp

void CodeGenFunction::EmitAutoVarInit(const AutoVarEmission &emission) {
   }
 
   // Check whether this is a byref variable that's potentially
   // captured and moved by its own initializer.  If so, we'll need to
   // emit the initializer first, then copy into the variable.
   bool capturedByInit =
       Init && emission.IsEscapingByRef && isCapturedBy(D, Init);
 
-  Address Loc =
-      capturedByInit ? emission.Addr : emission.getObjectAddress(*this);
+  bool locIsByrefHeader = !capturedByInit;
+  const Address Loc =
+      locIsByrefHeader ? emission.getObjectAddress(*this) : emission.Addr;
 
   // Note: constexpr already initializes everything correctly.
   LangOptions::TrivialAutoVarInitKind trivialAutoVarInit =
       (D.isConstexpr()
            ? LangOptions::TrivialAutoVarInitKind::Uninitialized
            : (D.getAttr<UninitializedAttr>()
                   ? LangOptions::TrivialAutoVarInitKind::Uninitialized
                   : getContext().getLangOpts().getTrivialAutoVarInit()));
 
   auto initializeWhatIsTechnicallyUninitialized = [&](Address Loc) {
     if (trivialAutoVarInit ==
         LangOptions::TrivialAutoVarInitKind::Uninitialized)
       return;
 
     // Only initialize a __block's storage: we always initialize the header.
-    if (emission.IsEscapingByRef)
+    if (emission.IsEscapingByRef && !locIsByrefHeader)
       Loc = emitBlockByrefAddress(Loc, &D, /*follow=*/false);
 
     CharUnits Size = getContext().getTypeSizeInChars(type);
     if (!Size.isZero()) {
       switch (trivialAutoVarInit) {
       case LangOptions::TrivialAutoVarInitKind::Uninitialized:
         llvm_unreachable("Uninitialized handled above");
       case LangOptions::TrivialAutoVarInitKind::Zero:
void CodeGenFunction::EmitAutoVarInit(const AutoVarEmission &emission) {
   if (!emission.IsConstantAggregate) {
     // For simple scalar/complex initialization, store the value directly.
     LValue lv = MakeAddrLValue(Loc, type);
     lv.setNonGC(true);
     return EmitStoreThroughLValue(RValue::get(constant), lv, true);
   }
 
   llvm::Type *BP = CGM.Int8Ty->getPointerTo(Loc.getAddressSpace());
-  if (Loc.getType() != BP)
-    Loc = Builder.CreateBitCast(Loc, BP);
-
-  emitStoresForConstant(CGM, D, Loc, isVolatile, Builder, constant);
+  emitStoresForConstant(
+      CGM, D, (Loc.getType() == BP) ? Loc : Builder.CreateBitCast(Loc, BP),
+      isVolatile, Builder, constant);
 }
 
 /// Emit an expression as an initializer for an object (variable, field, etc.)
 /// at the given location.  The expression is not necessarily the normal
 /// initializer for the object, and the address is not necessarily
 /// its normal location.
 ///
 /// \param init the initializing expression

cfe/trunk/test/CodeGenCXX/trivial-auto-var-init.cpp

 // ZERO:          %captured1 = getelementptr inbounds %struct.__block_byref_captured, %struct.__block_byref_captured* %captured, i32 0, i32 4
 // ZERO-NEXT:     store %struct.XYZ* null, %struct.XYZ** %captured1, align 8
 // ZERO:          %call = call %struct.XYZ* @create(
 // PATTERN-LABEL: test_block_self_init(
 // PATTERN:       %block = alloca <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, i8* }>, align 8
 // PATTERN:       %captured1 = getelementptr inbounds %struct.__block_byref_captured, %struct.__block_byref_captured* %captured, i32 0, i32 4
 // PATTERN-NEXT:  store %struct.XYZ* inttoptr (i64 -6148914691236517206 to %struct.XYZ*), %struct.XYZ** %captured1, align 8
 // PATTERN:       %call = call %struct.XYZ* @create(
-void test_block_self_init() {
-  using Block = void (^)();
-  typedef struct XYZ {
-    Block block;
-  } * xyz_t;
+void test_block_self_init() {
   extern xyz_t create(Block block);
   __block xyz_t captured = create(^() {
-    (void)captured;
+    used(captured);
+  });
+}
+
+// Capturing with escape after initialization is also an edge case.
+//
+// UNINIT-LABEL:  test_block_captures_self_after_init(
+// ZERO-LABEL:    test_block_captures_self_after_init(
+// ZERO:          %block = alloca <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, i8* }>, align 8
+// ZERO:          %captured1 = getelementptr inbounds %struct.__block_byref_captured.1, %struct.__block_byref_captured.1* %captured, i32 0, i32 4
+// ZERO-NEXT:     store %struct.XYZ* null, %struct.XYZ** %captured1, align 8
+// ZERO:          %call = call %struct.XYZ* @create(
+// PATTERN-LABEL: test_block_captures_self_after_init(
+// PATTERN:       %block = alloca <{ i8*, i32, i32, i8*, %struct.__block_descriptor*, i8* }>, align 8
+// PATTERN:       %captured1 = getelementptr inbounds %struct.__block_byref_captured.1, %struct.__block_byref_captured.1* %captured, i32 0, i32 4
+// PATTERN-NEXT:  store %struct.XYZ* inttoptr (i64 -6148914691236517206 to %struct.XYZ*), %struct.XYZ** %captured1, align 8
+// PATTERN:       %call = call %struct.XYZ* @create(
+void test_block_captures_self_after_init() {
+  extern xyz_t create(Block block);
+  __block xyz_t captured;
+  captured = create(^() {
+    used(captured);
   });
 }
 
 // This type of code is currently not handled by zero / pattern initialization.
 // The test will break when that is fixed.
 // UNINIT-LABEL:  test_goto_unreachable_value(
 // ZERO-LABEL:    test_goto_unreachable_value(
 // ZERO-NOT: store {{.*}}%oops