SILGen: Complete initialization of FinalContext in ConversionInitialization::tryPeephole

The callers to ConversionInitialization::tryPeephole assume that, if the conversion peephole
succeeded, that the peepholed result was fully emitted into the initialization. However, if
the ConversionInitialization sat on top of an in-memory initialization, then tryPeephole would
only set the value of the ConversionInitialization, without forwarding the value into the underlying
initialization, causing code generation to proceed leaving the underlying memory uninitialized.
This problem becomes exposed now when literal closures are emitted with a return value that is
indirectly returned and also reabstracted, and the return expression undergoes a ping-pong reabstraction
pair: we see through the conversions and peephole away the reabstractions, but fail to emplace the
result in the indirect return slot.

Fixes rdar://92654098

Author: jckarter

lib/SILGen/Conversion.h

@@ -231,13 +231,6 @@ canPeepholeConversions(SILGenFunction &SGF,
                        const Conversion &outerConversion,
                        const Conversion &innerConversion);
 
-ManagedValue emitPeepholedConversions(SILGenFunction &SGF, SILLocation loc,
-                                      const Conversion &outerConversion,
-                                      const Conversion &innerConversion,
-                                      ConversionPeepholeHint hint,
-                                      SGFContext C,
-                                      ValueProducerRef produceValue);
-
 /// An initialization where we ultimately want to apply a conversion to
 /// the value before completing the initialization.
 ///
@@ -354,8 +347,6 @@ class ConvertingInitialization final : public Initialization {
   void finishInitialization(SILGenFunction &SGF) override {
     assert(getState() == Initialized);
     State = Finished;
-    if (OwnedSubInitialization)
-      OwnedSubInitialization->finishInitialization(SGF);
   }
 };
 

lib/SILGen/SILGenConvert.cpp

@@ -1074,6 +1074,69 @@ ConvertingInitialization::finishEmission(SILGenFunction &SGF,
   llvm_unreachable("bad state");
 }
 
+static ManagedValue
+emitPeepholedConversions(SILGenFunction &SGF, SILLocation loc,
+                                   const Conversion &outerConversion,
+                                   const Conversion &innerConversion,
+                                   ConversionPeepholeHint hint,
+                                   SGFContext C,
+                                   ValueProducerRef produceOrigValue) {
+  auto produceValue = [&](SGFContext C) {
+    if (!hint.isForced()) {
+      return produceOrigValue(SGF, loc, C);
+    }
+
+    auto value = produceOrigValue(SGF, loc, SGFContext());
+    auto &optTL = SGF.getTypeLowering(value.getType());
+    // isForceUnwrap is hardcoded true because hint.isForced() is only
+    // set by implicit force unwraps.
+    return SGF.emitCheckedGetOptionalValueFrom(loc, value,
+                                               /*isForceUnwrap*/ true,
+                                               optTL, C);
+  };
+
+  auto getBridgingSourceType = [&] {
+    CanType sourceType = innerConversion.getBridgingSourceType();
+    if (hint.isForced())
+      sourceType = sourceType.getOptionalObjectType();
+    return sourceType;
+  };
+  auto getBridgingResultType = [&] {
+    return outerConversion.getBridgingResultType();
+  };
+  auto getBridgingLoweredResultType = [&] {
+    return outerConversion.getBridgingLoweredResultType();
+  };
+
+  switch (hint.getKind()) {
+  case ConversionPeepholeHint::Identity:
+    return produceValue(C);
+
+  case ConversionPeepholeHint::BridgeToAnyObject: {
+    auto value = produceValue(SGFContext());
+    return SGF.emitNativeToBridgedValue(loc, value, getBridgingSourceType(),
+                                        getBridgingResultType(),
+                                        getBridgingLoweredResultType(), C);
+  }
+
+  case ConversionPeepholeHint::Subtype: {
+    // Otherwise, emit and convert.
+    // TODO: if the context allows +0, use it in more situations.
+    auto value = produceValue(SGFContext());
+    SILType loweredResultTy = getBridgingLoweredResultType();
+
+    // Nothing to do if the value already has the right representation.
+    if (value.getType().getObjectType() == loweredResultTy.getObjectType())
+      return value;
+
+    CanType sourceType = getBridgingSourceType();
+    CanType resultType = getBridgingResultType();
+    return SGF.emitTransformedValue(loc, value, sourceType, resultType, C);
+  }
+  }
+  llvm_unreachable("bad kind");
+}
+
 bool ConvertingInitialization::tryPeephole(SILGenFunction &SGF,
                                            SILLocation loc,
                                            ManagedValue origValue,
@@ -1104,6 +1167,15 @@ bool ConvertingInitialization::tryPeephole(SILGenFunction &SGF, SILLocation loc,
   ManagedValue value = emitPeepholedConversions(SGF, loc, outerConversion,
                                                 innerConversion, *hint,
                                                 FinalContext, produceValue);
+  
+  // The callers to tryPeephole assume that the initialization is ready to be
+  // finalized after returning. If this conversion sits on top of another
+  // initialization, forward the value into the underlying initialization and
+  // report the value as emitted in context.
+  if (FinalContext.getEmitInto() && !value.isInContext()) {
+    value.ensurePlusOne(SGF, loc).forwardInto(SGF, loc, FinalContext.getEmitInto());
+    value = ManagedValue::forInContext();
+  }
   setConvertedValue(value);
   return true;
 }
@@ -1117,15 +1189,11 @@ void ConvertingInitialization::copyOrInitValueInto(SILGenFunction &SGF,
   // TODO: take advantage of borrowed inputs?
   if (!isInit) formalValue = formalValue.copy(SGF, loc);
   State = Initialized;
-  SGFContext emissionContext = OwnedSubInitialization
-    ? SGFContext() : FinalContext;
 
-  Value = TheConversion.emit(SGF, loc, formalValue, emissionContext);
+  Value = TheConversion.emit(SGF, loc, formalValue, FinalContext);
 
-  if (OwnedSubInitialization) {
-    OwnedSubInitialization->copyOrInitValueInto(SGF, loc,
-                                                Value,
-                                                isInit);
+  if (FinalContext.getEmitInto() && !Value.isInContext()) {
+    Value.forwardInto(SGF, loc, FinalContext.getEmitInto());
     Value = ManagedValue::forInContext();
   }
 }
@@ -1512,65 +1580,3 @@ Lowering::canPeepholeConversions(SILGenFunction &SGF,
   llvm_unreachable("bad kind");
 }
 
-ManagedValue
-Lowering::emitPeepholedConversions(SILGenFunction &SGF, SILLocation loc,
-                                   const Conversion &outerConversion,
-                                   const Conversion &innerConversion,
-                                   ConversionPeepholeHint hint,
-                                   SGFContext C,
-                                   ValueProducerRef produceOrigValue) {
-  auto produceValue = [&](SGFContext C) {
-    if (!hint.isForced()) {
-      return produceOrigValue(SGF, loc, C);
-    }
-
-    auto value = produceOrigValue(SGF, loc, SGFContext());
-    auto &optTL = SGF.getTypeLowering(value.getType());
-    // isForceUnwrap is hardcoded true because hint.isForced() is only
-    // set by implicit force unwraps.
-    return SGF.emitCheckedGetOptionalValueFrom(loc, value,
-                                               /*isForceUnwrap*/ true,
-                                               optTL, C);
-  };
-
-  auto getBridgingSourceType = [&] {
-    CanType sourceType = innerConversion.getBridgingSourceType();
-    if (hint.isForced())
-      sourceType = sourceType.getOptionalObjectType();
-    return sourceType;
-  };
-  auto getBridgingResultType = [&] {
-    return outerConversion.getBridgingResultType();
-  };
-  auto getBridgingLoweredResultType = [&] {
-    return outerConversion.getBridgingLoweredResultType();
-  };
-
-  switch (hint.getKind()) {
-  case ConversionPeepholeHint::Identity:
-    return produceValue(C);
-
-  case ConversionPeepholeHint::BridgeToAnyObject: {
-    auto value = produceValue(SGFContext());
-    return SGF.emitNativeToBridgedValue(loc, value, getBridgingSourceType(),
-                                        getBridgingResultType(),
-                                        getBridgingLoweredResultType(), C);
-  }
-
-  case ConversionPeepholeHint::Subtype: {
-    // Otherwise, emit and convert.
-    // TODO: if the context allows +0, use it in more situations.
-    auto value = produceValue(SGFContext());
-    SILType loweredResultTy = getBridgingLoweredResultType();
-
-    // Nothing to do if the value already has the right representation.
-    if (value.getType().getObjectType() == loweredResultTy.getObjectType())
-      return value;
-
-    CanType sourceType = getBridgingSourceType();
-    CanType resultType = getBridgingResultType();
-    return SGF.emitTransformedValue(loc, value, sourceType, resultType, C);
-  }
-  }
-  llvm_unreachable("bad kind");
-}

test/SILGen/literal_closure_reabstraction_with_peephole.swift

@@ -0,0 +1,32 @@
+// RUN: %target-swift-emit-silgen -verify %s
+
+fileprivate typealias Closure = () -> Void
+
+func crash1() {
+    let closure1: Closure? = nil
+    let closure2: Closure? = nil
+    let closure3: Closure? = nil
+    print("Closures: \(String(describing: closure1)), \(String(describing: closure2)), \(String(describing: closure3))")
+   
+    let closure = closure1 ?? closure2 ?? closure3
+
+    print("\(#line): \(String(describing: closure))")
+    closure?() // <- EXC_BAD_ACCESS
+    assert(closure == nil)
+}
+
+func crash2() {
+    let closure1: Closure? = nil
+    let closure2: Closure? = nil
+    let closure3: Closure? = { }
+    print("Closures: \(String(describing: closure1)), \(String(describing: closure2)), \(String(describing: closure3))")
+
+    let closure = closure1 ?? closure2 ?? closure3
+
+    print("\(#line): \(String(describing: closure))")
+    closure?() // <- EXC_BAD_ACCESS
+    assert(closure != nil)
+}
+
+crash1()
+crash2()