wasm-ctor-eval: Do not emit invalid code after hitting a GC cycle

src/support/topological_sort.h

@@ -33,6 +33,9 @@ namespace wasm {
 
 namespace TopologicalSort {
 
+// Thrown when no valid topological sort exists due to a cycle.
+struct CycleException {};
+
 // An adjacency list containing edges from vertices to their successors. Uses
 // `Index` because we are primarily sorting elements of Wasm modules. If we ever
 // need to sort signficantly larger objects, we might need to switch to
@@ -220,7 +223,10 @@ template<typename Cmp> struct TopologicalOrdersImpl {
     // Select the next available vertex, decrement in-degrees, and update the
     // sequence of available vertices. Return the Selector for the next vertex.
     Selector select(TopologicalOrdersImpl& ctx) {
-      assert(count >= 1);
+      if (count == 0) {
+        // No choices remain, indicating a cycle.
+        throw TopologicalSort::CycleException();
+      }
       assert(start + count <= ctx.buf.size());
       if constexpr (TopologicalOrdersImpl::useMinHeap) {
         ctx.buf[start] = ctx.popChoice();

src/tools/wasm-ctor-eval.cpp

@@ -1328,6 +1328,12 @@ EvalCtorOutcome evalCtor(EvallingModuleRunner& instance,
   }
 }
 
+// This is set when we find a situation we cannot handle in the middle of our
+// work. In that case we give up, throwing away the invalid state in memory.
+// TODO: Handle all those situations more gracefully, if that becomes useful
+//       enough at some point.
+static bool invalidState = false;
+
 // Eval all ctors in a module.
 void evalCtors(Module& wasm,
                std::vector<std::string>& ctors,
@@ -1432,7 +1438,19 @@ void evalCtors(Module& wasm,
       std::cout << "  ...stopping since could not create module instance: "
                 << fail.why << "\n";
     }
-    return;
+  } catch (TopologicalSort::CycleException e) {
+    // We use a topological sort for GC globals. If there is a non-breakable
+    // cycle there, we will hit an error (we can break cycles in nullable and
+    // mutable fields by setting a null and filling in the value later, but
+    // other situations are a problem).
+    if (!quiet) {
+      std::cout << "  ...stopping since global sorting hit a cycle\n";
+    }
+
+    // We found the cycle during the processing of globals, making wasm->globals
+    // invalid. Rather than refactor the code heavily to handle this very rare
+    // case, just give up entirely, and avoid writing out the current state.
+    invalidState = true;
   }
 }
 
@@ -1552,6 +1570,17 @@ int main(int argc, const char* argv[]) {
   if (canEval(wasm)) {
     evalCtors(wasm, ctors, keptExports);
 
+    if (invalidState) {
+      // We ended up in a state that cannot be written out. Forget about all of
+      // it, by re-reading the input and continuing from there (with nothing at
+      // all evalled, effectively).
+      auto features = wasm.features;
+      ModuleUtils::clearModule(wasm);
+      wasm.features = features;
+      ModuleReader reader;
+      reader.read(options.extra["infile"], wasm);
+    }
+
     if (!WasmValidator().validate(wasm)) {
       std::cout << wasm << '\n';
       Fatal() << "error in validating output";

test/lit/ctor-eval/gc-cycle.wast

@@ -1291,3 +1291,59 @@
 ;; CHECK-NEXT:   (global.get $ctor-eval$global)
 ;; CHECK-NEXT:  )
 ;; CHECK-NEXT: )
+(module
+ ;; A circular reference using a non-nullable (but mutable) field. Unlike cases
+ ;; above, we cannot break up such cycles, and must give up. We should at least
+ ;; not error.
+
+ ;; CHECK:      (type $array (array i8))
+ (type $array  (array i8))
+ ;; CHECK:      (type $struct (struct (field (mut (ref any)))))
+ (type $struct (struct (field (mut (ref any)))))
+
+ ;; CHECK:      (type $2 (func))
+
+ ;; CHECK:      (global $global (mut i32) (i32.const 42))
+ (global $global (mut i32) (i32.const 42))
+
+ ;; CHECK:      (export "test" (func $test))
+
+ ;; CHECK:      (func $test (type $2)
+ ;; CHECK-NEXT:  (local $temp (ref $struct))
+ ;; CHECK-NEXT:  (global.set $global
+ ;; CHECK-NEXT:   (i32.const 1337)
+ ;; CHECK-NEXT:  )
+ ;; CHECK-NEXT:  (local.set $temp
+ ;; CHECK-NEXT:   (struct.new $struct
+ ;; CHECK-NEXT:    (array.new_fixed $array 0)
+ ;; CHECK-NEXT:   )
+ ;; CHECK-NEXT:  )
+ ;; CHECK-NEXT:  (struct.set $struct 0
+ ;; CHECK-NEXT:   (local.get $temp)
+ ;; CHECK-NEXT:   (local.get $temp)
+ ;; CHECK-NEXT:  )
+ ;; CHECK-NEXT: )
+ (func $test (export "test")
+  (local $temp (ref $struct))
+
+  ;; Set the global. This will not get written out, as we will cancel all our
+  ;; work when we hit the cycle below. (TODO: improve that)
+  (global.set $global
+   (i32.const 1337)
+  )
+
+  ;; Start with the struct referring to an array.
+  (local.set $temp
+   (struct.new $struct
+    (array.new_fixed $array 0)
+   )
+  )
+
+  ;; Make the struct refer to itself, circularly.
+  (struct.set $struct 0
+   (local.get $temp)
+   (local.get $temp)
+  )
+ )
+)
+