Merge pull request #81480 from eeckstein/fix-fso

FunctionSignatureOptimization: don't convert indirect `@in` to `@in_guaranteed` if the argument is mutated

Author: eeckstein

include/swift/SIL/InstructionUtils.h

@@ -247,6 +247,13 @@ lookUpFunctionInWitnessTable(WitnessMethodInst *wmi, SILModule::LinkingMode link
 /// struct-with-deinit drops the deinit.
 bool shouldExpand(SILModule &module, SILType ty);
 
+/// Returns true if `arg` is mutated.
+/// if `ignoreDestroys` is true, `destroy_addr` instructions are ignored.
+/// `defaultIsMutating` specifies the state of instructions which are not explicitly handled.
+/// For historical reasons this utility is implemented in SILVerifier.cpp.
+bool isIndirectArgumentMutated(SILFunctionArgument *arg, bool ignoreDestroys = false,
+                               bool defaultIsMutating = false);
+
 } // end namespace swift
 
 #endif

lib/SIL/Verifier/SILVerifier.cpp

@@ -35,6 +35,7 @@
 #include "swift/SIL/DebugUtils.h"
 #include "swift/SIL/Dominance.h"
 #include "swift/SIL/DynamicCasts.h"
+#include "swift/SIL/InstructionUtils.h"
 #include "swift/SIL/MemAccessUtils.h"
 #include "swift/SIL/OwnershipLiveness.h"
 #include "swift/SIL/OwnershipUtils.h"
@@ -572,6 +573,11 @@ void verifyKeyPathComponent(SILModule &M,
 /// open_existential_addr. We should expand it as needed.
 struct ImmutableAddressUseVerifier {
   SmallVector<Operand *, 32> worklist;
+  bool ignoreDestroys;
+  bool defaultIsMutating;
+
+  ImmutableAddressUseVerifier(bool ignoreDestroys = false, bool defaultIsMutating = false)
+    : ignoreDestroys(ignoreDestroys), defaultIsMutating(defaultIsMutating) {}
 
   bool isConsumingOrMutatingArgumentConvention(SILArgumentConvention conv) {
     switch (conv) {
@@ -704,10 +710,9 @@ struct ImmutableAddressUseVerifier {
           }
         }
 
-        // Otherwise this is a builtin that we are not expecting to see, so bail
-        // and assert.
-        llvm::errs() << "Unhandled, unexpected builtin instruction: " << *inst;
-        llvm_unreachable("invoking standard assertion failure");
+        // Otherwise this is a builtin that we are not expecting to see.
+        if (defaultIsMutating)
+          return true;
         break;
       }
       case SILInstructionKind::MarkDependenceInst:
@@ -775,7 +780,9 @@ struct ImmutableAddressUseVerifier {
         else
           break;
       case SILInstructionKind::DestroyAddrInst:
-        return true;
+        if (!ignoreDestroys)
+          return true;
+        break;
       case SILInstructionKind::UpcastInst:
       case SILInstructionKind::UncheckedAddrCastInst: {
         if (isAddrCastToNonConsuming(cast<SingleValueInstruction>(inst))) {
@@ -842,9 +849,7 @@ struct ImmutableAddressUseVerifier {
           }
           break;
         }
-        llvm::errs() << "Unhandled, unexpected instruction: " << *inst;
-        llvm_unreachable("invoking standard assertion failure");
-        break;
+        return true;
       }
       case SILInstructionKind::TuplePackElementAddrInst: {
         if (&cast<TuplePackElementAddrInst>(inst)->getOperandRef(
@@ -866,8 +871,8 @@ struct ImmutableAddressUseVerifier {
         return false;
       }
       default:
-        llvm::errs() << "Unhandled, unexpected instruction: " << *inst;
-        llvm_unreachable("invoking standard assertion failure");
+        if (defaultIsMutating)
+          return true;
         break;
       }
     }
@@ -7386,6 +7391,11 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
 #undef require
 #undef requireObjectType
 
+bool swift::isIndirectArgumentMutated(SILFunctionArgument *arg, bool ignoreDestroys,
+                                      bool defaultIsMutating) {
+  return ImmutableAddressUseVerifier(ignoreDestroys, defaultIsMutating).isMutatingOrConsuming(arg);
+}
+
 //===----------------------------------------------------------------------===//
 //                     Out of Line Verifier Run Functions
 //===----------------------------------------------------------------------===//

lib/SILOptimizer/FunctionSignatureTransforms/OwnedToGuaranteedTransform.cpp

@@ -88,6 +88,15 @@ bool FunctionSignatureTransform::OwnedToGuaranteedAnalyzeParameters() {
       continue;
     }
 
+    // Make sure that an @in argument is not mutated otherwise than destroyed,
+    // because an @in_guaranteed argument must not be mutated.
+    if (A.hasConvention(SILArgumentConvention::Indirect_In) &&
+        // With opaque values, @in arguments can have non-address types.
+        A.Arg->getType().isAddress() &&
+        isIndirectArgumentMutated(A.Arg, /*ignoreDestroys=*/ true, /*defaultIsMutating=*/true)) {
+      continue;
+    }
+  
     // See if we can find a ref count equivalent strong_release or release_value
     // at the end of this function if our argument is an @owned parameter.
     // See if we can find a destroy_addr at the end of this function if our

test/SILOptimizer/functionsigopts_crash.swift

@@ -35,3 +35,21 @@ func testit(_ p: P) {
 public func callit(s: S) {
   testit(s)
 }
+
+// Check that FSO does not trigger a verifier error caused by a mutated @in argument which is 
+// converted to an @in_guaranteed argument (which must not be mutated).
+
+public protocol IP<Element> {
+  associatedtype Element
+
+  init<Iterator>(iterator: consuming Iterator) where Iterator: IteratorProtocol, Iterator.Element == Element
+}
+
+extension Array: IP {
+  public init<Iterator>(iterator: consuming Iterator) where Iterator: IteratorProtocol, Iterator.Element == Element {
+    self.init()
+    while let next = iterator.next() {
+      append(next)
+    }
+  }
+}