[wasm-ctor-eval] Properly handle multiple ctors with GC (#5522)

Before, a single ctor with GC worked, but any subsequent ones simply dropped
the globals from the previous ones, because we were missing an addGlobal in
an important place.

Also, we can get confused about which global names are in use in the module, so fix
that as well by storing them directly (we keep removing and re-adding globals, so
we can't use the normal module mechanism to find which names are in use).

3 files changed, 104 insertions, 12 deletions

diff --git a/src/tools/wasm-ctor-eval.cpp b/src/tools/wasm-ctor-eval.cpp
index d41094c67..13e2d5767 100644
--- a/src/tools/wasm-ctor-eval.cpp
+++ b/src/tools/wasm-ctor-eval.cpp
@@ -154,6 +154,13 @@ struct CtorEvalExternalInterface : EvallingModuleRunner::ExternalInterface {
   // A representation of the contents of wasm memory as we execute.
   std::unordered_map<Name, std::vector<char>> memories;
 
+  // All the names of globals we've seen in the module. We cannot reuse these.
+  // We must track these manually as we will be adding more, and as we do so we
+  // also reorder them, so we remove and re-add globals, which means the module
+  // itself is not aware of all the globals that belong to it (those that have
+  // not yet been re-added are a blind spot for it).
+  std::unordered_set<Name> usedGlobalNames;
+
   CtorEvalExternalInterface(
     std::map<Name, std::shared_ptr<EvallingModuleRunner>> linkedInstances_ =
       {}) {
@@ -182,6 +189,10 @@ struct CtorEvalExternalInterface : EvallingModuleRunner::ExternalInterface {
         memories[memory->name] = data;
       }
     }
+
+    for (auto& global : wasm->globals) {
+      usedGlobalNames.insert(global->name);
+    }
   }
 
   void importGlobals(GlobalValueSet& globals, Module& wasm_) override {
@@ -526,20 +537,21 @@ private:
       Name name;
       if (!oldGlobal->mutable_ && oldGlobal->type == oldGlobal->init->type) {
         // This has the properties we need of a defining global - immutable and
-        // of the precise type - so use it.
+        // of the precise type - so use it as such.
         name = oldGlobal->name;
       }
 
-      // If there is a value here to serialize, do so. (If there is no value,
-      // then this global was added after the interpreter initialized the
-      // module, which means it is a new global we've added since; we don't need
-      // to do anything for such a global - if it is needed it will show up as a
-      // dependency of something, and be emitted at the right time and place.)
+      // If the instance has an evalled value here, compute the serialization
+      // for it. (If there is no value, then this is a new global we've added
+      // during execution, for whom we've already set up a proper serialized
+      // value when we created it.)
       auto iter = instance->globals.find(oldGlobal->name);
       if (iter != instance->globals.end()) {
         oldGlobal->init = getSerialization(iter->second, name);
-        wasm->addGlobal(std::move(oldGlobal));
       }
+
+      // Add the global back to the module.
+      wasm->addGlobal(std::move(oldGlobal));
     }
   }
 
@@ -616,7 +628,9 @@ public:
       }
 
       // Allocate a new defining global.
-      auto name = Names::getValidGlobalName(*wasm, "ctor-eval$global");
+      auto name =
+        Names::getValidNameGivenExisting("ctor-eval$global", usedGlobalNames);
+      usedGlobalNames.insert(name);
       wasm->addGlobal(builder.makeGlobal(name, type, init, Builder::Immutable));
       definingGlobal = name;
     }
diff --git a/test/ctor-eval/gc.wast.out b/test/ctor-eval/gc.wast.out
index 6ca1f50d9..4a30752e0 100644
--- a/test/ctor-eval/gc.wast.out
+++ b/test/ctor-eval/gc.wast.out
@@ -7,11 +7,11 @@
  (global $global1 (ref $struct) (struct.new $struct
   (i32.const 1337)
  ))
- (global $ctor-eval$global (ref $struct) (struct.new $struct
+ (global $ctor-eval$global_1 (ref $struct) (struct.new $struct
   (i32.const 42)
  ))
- (global $global2 (mut (ref null $struct)) (global.get $ctor-eval$global))
- (global $ctor-eval$global_0 (ref $struct) (struct.new $struct
+ (global $global2 (mut (ref null $struct)) (global.get $ctor-eval$global_1))
+ (global $ctor-eval$global_2 (ref $struct) (struct.new $struct
   (i32.const 99)
  ))
  (export "test1" (func $0_0))
@@ -29,7 +29,7 @@
  (func $0_0 (type $none_=>_none)
   (local $0 (ref $struct))
   (local.set $0
-   (global.get $ctor-eval$global_0)
+   (global.get $ctor-eval$global_2)
   )
   (call $import
    (ref.null none)
diff --git a/test/lit/ctor-eval/ctor_after_serialization.wat b/test/lit/ctor-eval/ctor_after_serialization.wat
new file mode 100644
index 000000000..7a4871398
--- /dev/null
+++ b/test/lit/ctor-eval/ctor_after_serialization.wat
@@ -0,0 +1,78 @@
+;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.
+;; RUN: foreach %s %t wasm-ctor-eval --ctors=new,nop --kept-exports=new,nop --quiet -all -S -o - | filecheck %s
+
+;; We can eval $new, and afterwards also eval $nop as well. When doing the
+;; latter we should not undo or break the previous work in any way. In
+;; particular, we should have a valid global for $new to refer to (which
+;; contains the serialization of the struct.new instruction).
+
+(module
+ ;; CHECK:      (type $A (struct ))
+ (type $A (struct))
+
+ ;; CHECK:      (type $none_=>_ref|any| (func (result (ref any))))
+
+ ;; CHECK:      (type $none_=>_none (func))
+
+ ;; CHECK:      (global $ctor-eval$global (ref $A) (struct.new_default $A))
+
+ ;; CHECK:      (export "new" (func $new_0))
+ (export "new" (func $new))
+ ;; CHECK:      (export "nop" (func $nop_0))
+ (export "nop" (func $nop))
+
+ (func $new (result (ref any))
+  (struct.new $A)
+ )
+
+ (func $nop
+  (nop)
+ )
+)
+
+;; CHECK:      (func $new_0 (type $none_=>_ref|any|) (result (ref any))
+;; CHECK-NEXT:  (global.get $ctor-eval$global)
+;; CHECK-NEXT: )
+
+;; CHECK:      (func $nop_0 (type $none_=>_none)
+;; CHECK-NEXT:  (nop)
+;; CHECK-NEXT: )
+(module
+ ;; As above, but now there is an existing global with the name that we want to
+ ;; use. We should not collide.
+
+ ;; CHECK:      (type $A (struct ))
+ (type $A (struct))
+
+ ;; CHECK:      (type $none_=>_ref|any| (func (result (ref any))))
+
+ ;; CHECK:      (type $none_=>_anyref (func (result anyref)))
+
+ ;; CHECK:      (global $ctor-eval$global (ref $A) (struct.new_default $A))
+ (global $ctor-eval$global (ref $A)
+  (struct.new_default $A)
+ )
+
+ ;; CHECK:      (global $ctor-eval$global_0 (ref $A) (struct.new_default $A))
+
+ ;; CHECK:      (export "new" (func $new_0))
+ (export "new" (func $new))
+ ;; CHECK:      (export "nop" (func $nop_0))
+ (export "nop" (func $nop))
+
+ (func $new (result (ref any))
+  (struct.new $A)
+ )
+
+ (func $nop (result anyref)
+  ;; Use the existing global to keep it alive.
+  (global.get $ctor-eval$global)
+ )
+)
+;; CHECK:      (func $new_0 (type $none_=>_ref|any|) (result (ref any))
+;; CHECK-NEXT:  (global.get $ctor-eval$global_0)
+;; CHECK-NEXT: )
+
+;; CHECK:      (func $nop_0 (type $none_=>_anyref) (result anyref)
+;; CHECK-NEXT:  (global.get $ctor-eval$global)
+;; CHECK-NEXT: )