[Runtime] Fix a false metadata cycle diagnostic when threads race to instantiate cyclical metadata.

The metadata creation system detects cycles where metadata depends on other metadata which depends on the first one again and raises a fatal error if the cycle can't be fulfilled.

Some cycles can be fulfilled. The cycle may involve a requirement for a metadata state less than full transitive completeness which can be reached without resolving the entire cycle. We only want to raise a fatal error when we detect a cycle that can't be fulfilled.

Normally this happens because the cycle checking in `blockOnMetadataDependency` only sees a cycle when it can't be fulfilled. Metadata initialization is advanced as far as it can be at each stage, so a cycle that can be fulfilled will see a fulfilling state and won't generate the dependency in the first place, since we only generate dependencies that haven't yet been met.

However, when two threads race to create types in a cycle, we can end up with such a dependency, because the dependency may be generated before another thread fulfilled yet. The cycle checker doesn't account for this and incorrectly raises a fatal error in that case.

Fix this by checking the cyclic dependency against the metadata's current state. If we have a dependency that's already been fulfilled, then there isn't really a dependency cycle. In that case, don't raise a fatal error.

rdar://135036243

Author: mikeash

stdlib/public/runtime/Metadata.cpp

@@ -7811,44 +7811,50 @@ checkMetadataDependency(MetadataDependency dependency) {
 void swift::blockOnMetadataDependency(MetadataDependency root,
                                       MetadataDependency firstLink) {
   std::vector<MetadataDependency> links;
-  auto checkNewLink = [&](MetadataDependency newLink) {
-    links.push_back(newLink);
-    for (auto i = links.begin(), e = links.end() - 1; i != e; ++i) {
-      if (i->Value == newLink.Value) {
-        diagnoseMetadataDependencyCycle(
-          llvm::makeArrayRef(&*i, links.end() - i));
-      }
-    }
-  };
-
   links.push_back(root);
 
   // Iteratively add each link, checking for a cycle, until we reach
   // something without a known dependency.
-  checkNewLink(firstLink);
-  while (true) {
+
+  // Start out with firstLink. The initial NewState value won't be
+  // used, so just initialize it to an arbitrary value.
+  MetadataStateWithDependency currentCheckResult{
+      PrivateMetadataState::Allocating, firstLink};
+
+  // If there isn't a known dependency, we can't do any more checking.
+  while (currentCheckResult.Dependency) {
+    // Add this dependency to our links.
+    links.push_back(currentCheckResult.Dependency);
+
     // Try to get a dependency for the metadata in the last link we added.
-    auto checkResult = checkMetadataDependency(links.back());
-
-    // If there isn't a known dependency, we can't do any more checking.
-    if (!checkResult.Dependency) {
-      // In the special case where it's the first link that doesn't have
-      // a known dependency and its current metadata state now satisfies
-      // the dependency leading to it, we can skip waiting.
-      if (links.size() == 2 && 
-          satisfies(checkResult.NewState, links.back().Requirement))
-        return;
-
-      // Otherwise, just make a blocking request for the first link in
-      // the chain.
-      auto request = MetadataRequest(firstLink.Requirement);
-      swift_checkMetadataState(request, firstLink.Value);
-      return;
-    }
+    currentCheckResult = checkMetadataDependency(links.back());
 
-    // Check the new link.
-    checkNewLink(checkResult.Dependency);
+    // Check the last link against the rest of the list.
+    for (auto i = links.begin(), e = links.end() - 1; i != e; ++i) {
+      if (i->Value == links.back().Value) {
+        // If there's a cycle but the new link's current state is now satisfied,
+        // then this is a stale dependency, not a cycle. This can happen when
+        // threads race to build a type in a fulfillable cycle.
+        if (!satisfies(currentCheckResult.NewState, links.back().Requirement))
+          diagnoseMetadataDependencyCycle(
+              llvm::makeArrayRef(&*i, links.end() - i));
+      }
+    }
   }
+
+  // We didn't find any cycles. Make a blocking request if appropriate.
+
+  // In the special case where it's the first link that doesn't have
+  // a known dependency and its current metadata state now satisfies
+  // the dependency leading to it, we can skip waiting.
+  if (links.size() == 2 &&
+      satisfies(currentCheckResult.NewState, links.back().Requirement))
+    return;
+
+  // Otherwise, just make a blocking request for the first link in
+  // the chain.
+  auto request = MetadataRequest(firstLink.Requirement);
+  swift_checkMetadataState(request, firstLink.Value);
 }
 
 /***************************************************************************/

test/Interpreter/metadata_cycles_threaded.swift

@@ -0,0 +1,46 @@
+// RUN: %target-run-simple-swift(%import-libdispatch)
+
+// REQUIRES: executable_test
+// REQUIRES: libdispatch
+// UNSUPPORTED: use_os_stdlib
+// UNSUPPORTED: back_deployment_runtime
+
+import Dispatch
+
+@_optimize(none) @inline(never) func forceTypeInstantiation(_: Any.Type) {}
+
+struct AnyFoo<T, U> {
+  var thing: U
+}
+
+struct S<T> {
+  var thing: T
+  var next: AnyFoo<S, T>?
+}
+
+// We want to ensure that the runtime handles legal metadata cycles when threads
+// race to instantiate the cycle. We have a cycle between S and AnyFoo, but it's
+// resolvable because AnyFoo doesn't depend on S's layout. This tests a fix for
+// a bug where the runtime's cycle detection could be overeager when multiple
+// threads raced, and flag a legal.
+//
+// Since this is a multithreading test, failures are probabilistic and each type
+// can only be tested once. The recursiveTry construct generates a large number
+// of distinct types so we can do many tests.
+func tryWithType<T>(_ t: T.Type) {
+  DispatchQueue.concurrentPerform(iterations: 5) { n in
+    forceTypeInstantiation(AnyFoo<S<T>, T>?.self)
+  }
+}
+
+struct One<T> {}
+struct Two<T> {}
+
+func recursiveTry<T>(_ t: T.Type, depth: Int = 0) {
+  if depth > 10 { return }
+  tryWithType(T.self)
+  recursiveTry(One<T>.self, depth: depth + 1)
+  recursiveTry(Two<T>.self, depth: depth + 1)
+}
+
+recursiveTry(Int.self)