wasm-merge: Sort globals to ensure proper validation (#6221)

If the first module has a global that reads from a global that appears in a later
module, then we need to reorder the globals, because if we just append the
globals from the later module we'd end up with a global reading from another
that is not before it.

Changes to the existing renamings test are just due to the global sorting
pass that now runs (it not only fixes up validation errors but also tries to sort
in a more optimal order for size).

Fixes #6220

5 files changed, 53 insertions, 11 deletions

diff --git a/src/passes/ReorderGlobals.cpp b/src/passes/ReorderGlobals.cpp
index d1c26b2cc..9b5940a21 100644
--- a/src/passes/ReorderGlobals.cpp
+++ b/src/passes/ReorderGlobals.cpp
@@ -58,7 +58,11 @@ private:
 
 struct ReorderGlobals : public Pass {
   // Whether to always reorder globals, even if there are very few and the
-  // benefit is minor. That is useful for testing.
+  // benefit is minor. That is useful for testing, and also internally in passes
+  // that use us to reorder them so dependencies appear first (that is, if a
+  // pass ends up with an earlier global reading a later one, the sorting in
+  // this pass will reorder them properly; we need to take those dependencies
+  // into account anyhow here).
   bool always;
 
   ReorderGlobals(bool always) : always(always) {}
diff --git a/src/tools/wasm-merge.cpp b/src/tools/wasm-merge.cpp
index 088317eec..0884350cb 100644
--- a/src/tools/wasm-merge.cpp
+++ b/src/tools/wasm-merge.cpp
@@ -586,13 +586,17 @@ Note that filenames and modules names are interleaved (which is hopefully less c
   // module.
   fuseImportsAndExports();
 
-  // Remove unused things. This is obviously a useful optimization, but it also
-  // makes using the output easier: if an import was resolved by an export
-  // during the merge, then that import will have no more uses and it will be
-  // optimized out (while if we didn't optimize it out then instantiating the
-  // module would still be forced to provide something for that import).
   {
     PassRunner passRunner(&merged);
+    // We might have made some globals read from others that now appear after
+    // them (if the one they read was appended from a later module). Sort them
+    // to fix that. TODO: we could do this only if we actually append globals
+    passRunner.add("reorder-globals-always");
+    // Remove unused things. This is obviously a useful optimization but it also
+    // makes using the output easier: if an import was resolved by an export
+    // during the merge, then that import will have no more uses and it will be
+    // optimized out (while if we didn't optimize it out then instantiating the
+    // module would still be forced to provide something for that import).
     passRunner.add("remove-unused-module-elements");
     passRunner.run();
   }
diff --git a/test/lit/merge/global-ordering.wat b/test/lit/merge/global-ordering.wat
new file mode 100644
index 000000000..2cc3b504b
--- /dev/null
+++ b/test/lit/merge/global-ordering.wat
@@ -0,0 +1,30 @@
+;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.
+
+;; RUN: wasm-merge %s first %s.second second -all -S -o - | filecheck %s
+
+;; After the merge this module's global will read a value from a global that is
+;; appended after it, from the second module. Those must be reordered so that
+;; we validate, as a global can only read from previous ones.
+
+(module
+  (import "second" "second.global.export" (global i32))
+
+  ;; CHECK:      (type $0 (func (result i32)))
+
+  ;; CHECK:      (global $second.global i32 (i32.const 42))
+
+  ;; CHECK:      (global $first.global (mut i32) (global.get $second.global))
+  (global $first.global (mut i32) (global.get 0))
+
+  ;; CHECK:      (export "run" (func $run))
+
+  ;; CHECK:      (export "second.global.export" (global $second.global))
+
+  ;; CHECK:      (func $run (type $0) (result i32)
+  ;; CHECK-NEXT:  (global.get $first.global)
+  ;; CHECK-NEXT: )
+  (func $run (export "run") (result i32)
+    ;; Use the global to avoid it being removed.
+    (global.get $first.global)
+  )
+)
diff --git a/test/lit/merge/global-ordering.wat.second b/test/lit/merge/global-ordering.wat.second
new file mode 100644
index 000000000..7676599e4
--- /dev/null
+++ b/test/lit/merge/global-ordering.wat.second
@@ -0,0 +1,3 @@
+(module
+  (global $second.global (export "second.global.export") i32 (i32.const 42))
+)
diff --git a/test/lit/merge/renamings.wat b/test/lit/merge/renamings.wat
index 6d4de5c65..98aac63be 100644
--- a/test/lit/merge/renamings.wat
+++ b/test/lit/merge/renamings.wat
@@ -23,20 +23,21 @@
 
   ;; CHECK:      (import "elsewhere" "some.tag" (tag $imported (param f64)))
 
+  ;; CHECK:      (global $bar_2 i32 (i32.const 4))
+
+  ;; CHECK:      (global $other i32 (i32.const 3))
+
+  ;; CHECK:      (global $bar i32 (i32.const 2))
+
   ;; CHECK:      (global $foo i32 (i32.const 1))
   (global $foo i32 (i32.const 1))
 
   ;; This global has a conflict in second.wat, and so second.wat's $bar
   ;; will be renamed.
-  ;; CHECK:      (global $bar i32 (i32.const 2))
   (global $bar i32 (i32.const 2))
 
   ;; This memory has a conflict in second.wat, and so second.wat's $foo
   ;; will be renamed.
-  ;; CHECK:      (global $other i32 (i32.const 3))
-
-  ;; CHECK:      (global $bar_2 i32 (i32.const 4))
-
   ;; CHECK:      (memory $foo 10 20)
   (memory $foo 10 20)