wasm-ctor-eval: Do not emit invalid code after hitting a GC cycle (#8188)

We break up cycles during serialization of GC data, but we can't always do
so: if the cycle uses immutable and non-nullable fields, we can't write a null
first and close the loop later. Before this PR, we would error inside the
topological sort utility; instead, catch an error from there, and stop our
work.

Unfortunately, it is complicated to detect this situation ahead of time: we
only hit a problem on the specific type of cycle mentioned before, and we
do so while already modifying the module state. Rather than do a large
refactoring to handle this, just give up on the state in memory, clearing it,
losing any evalling work thus far. If this becomes important enough, we
can consider handling it more gracefully later.

Author: kripken

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)
+  )
+ )
+)
+