[GTO] Update struct field indices after localizing operands

src/passes/GlobalTypeOptimization.cpp

@@ -643,8 +643,26 @@ struct GlobalTypeOptimization : public Pass {
     TypeRewriter(wasm, *this).update();
   }
 
+  Index getNewIndex(HeapType type, Index index) {
+    auto iter = indexesAfterRemovals.find(type);
+    if (iter == indexesAfterRemovals.end()) {
+      return index;
+    }
+    auto& indexesAfterRemoval = iter->second;
+    auto newIndex = indexesAfterRemoval[index];
+    assert(newIndex < IndexAnalysis(indexesAfterRemoval).newSize ||
+           newIndex == RemovedField);
+    return newIndex;
+  }
+
   // After updating the types to remove certain fields, we must also remove
-  // them from struct instructions.
+  // them from struct instructions and update field indices. We do this in two
+  // passes: first FieldRemover removes/reorders StructNew operands and replaces
+  // removed StructSets (which invokes EffectAnalyzer via ChildLocalizer and
+  // getResultOfFirst), and then IndexUpdater updates the field indices on
+  // remaining struct instructions. Keeping the old indices during FieldRemover
+  // is essential because EffectAnalyzer inspects type.getStruct().fields[index]
+  // on the old HeapTypes, which requires old indices.
   void updateInstructions(Module& wasm) {
     struct FieldRemover : public WalkerPass<PostWalker<FieldRemover>> {
       bool isFunctionParallel() override { return true; }
@@ -733,11 +751,9 @@ struct GlobalTypeOptimization : public Pass {
           return;
         }
 
-        auto newIndex = getNewIndex(curr->ref->type.getHeapType(), curr->index);
-        if (newIndex != RemovedField) {
-          // Map to the new index.
-          curr->index = newIndex;
-        } else {
+        auto newIndex =
+          parent.getNewIndex(curr->ref->type.getHeapType(), curr->index);
+        if (newIndex == RemovedField) {
           // This field was removed, so just emit drops of our children, plus a
           // trap if the ref is null. Note that we must preserve the order of
           // operations here: the trap on a null ref happens after the value,
@@ -758,72 +774,92 @@ struct GlobalTypeOptimization : public Pass {
         }
       }
 
-      void visitStructGet(StructGet* curr) {
+      void visitFunction(Function* curr) {
+        if (needEHFixups) {
+          EHUtils::handleBlockNestedPops(curr, *getModule());
+        }
+      }
+    };
+
+    struct IndexUpdater : public WalkerPass<PostWalker<IndexUpdater>> {
+      bool isFunctionParallel() override { return true; }
+
+      GlobalTypeOptimization& parent;
+
+      IndexUpdater(GlobalTypeOptimization& parent) : parent(parent) {}
+
+      std::unique_ptr<Pass> create() override {
+        return std::make_unique<IndexUpdater>(parent);
+      }
+
+      void visitStructSet(StructSet* curr) {
         if (curr->ref->type == Type::unreachable) {
           return;
         }
 
-        auto newIndex = getNewIndex(curr->ref->type.getHeapType(), curr->index);
-        // We must not remove a field that is read from.
+        auto newIndex =
+          parent.getNewIndex(curr->ref->type.getHeapType(), curr->index);
         assert(newIndex != RemovedField);
         curr->index = newIndex;
       }
 
-      void visitStructRMW(StructRMW* curr) {
+      void visitStructGet(StructGet* curr) {
         if (curr->ref->type == Type::unreachable) {
           return;
         }
 
-        auto newIndex = getNewIndex(curr->ref->type.getHeapType(), curr->index);
+        auto newIndex =
+          parent.getNewIndex(curr->ref->type.getHeapType(), curr->index);
         // We must not remove a field that is read from.
         assert(newIndex != RemovedField);
         curr->index = newIndex;
       }
 
-      void visitStructCmpxchg(StructCmpxchg* curr) {
+      void visitStructRMW(StructRMW* curr) {
         if (curr->ref->type == Type::unreachable) {
           return;
         }
 
-        auto newIndex = getNewIndex(curr->ref->type.getHeapType(), curr->index);
+        auto newIndex =
+          parent.getNewIndex(curr->ref->type.getHeapType(), curr->index);
         // We must not remove a field that is read from.
         assert(newIndex != RemovedField);
         curr->index = newIndex;
       }
 
-      void visitFunction(Function* curr) {
-        if (needEHFixups) {
-          EHUtils::handleBlockNestedPops(curr, *getModule());
+      void visitStructCmpxchg(StructCmpxchg* curr) {
+        if (curr->ref->type == Type::unreachable) {
+          return;
         }
-      }
 
-    private:
-      Index getNewIndex(HeapType type, Index index) {
-        auto iter = parent.indexesAfterRemovals.find(type);
-        if (iter == parent.indexesAfterRemovals.end()) {
-          return index;
-        }
-        auto& indexesAfterRemoval = iter->second;
-        auto newIndex = indexesAfterRemoval[index];
-        assert(newIndex < IndexAnalysis(indexesAfterRemoval).newSize ||
-               newIndex == RemovedField);
-        return newIndex;
+        auto newIndex =
+          parent.getNewIndex(curr->ref->type.getHeapType(), curr->index);
+        // We must not remove a field that is read from.
+        assert(newIndex != RemovedField);
+        curr->index = newIndex;
       }
     };
 
-    FieldRemover remover(*this);
-    remover.run(getPassRunner(), &wasm);
-    remover.runOnModuleCode(getPassRunner(), &wasm);
+    PassRunner runner(getPassRunner());
+    runner.add(std::make_unique<FieldRemover>(*this));
+    runner.add(std::make_unique<IndexUpdater>(*this));
+    runner.run();
+
+    FieldRemover moduleRemover(*this);
+    moduleRemover.runOnModuleCode(getPassRunner(), &wasm);
 
     // Insert globals necessary to preserve instantiation-time trapping of
     // removed expressions.
-    for (Index i = 0; i < remover.removedTrappingInits.size(); ++i) {
-      auto* curr = remover.removedTrappingInits[i];
+    for (Index i = 0; i < moduleRemover.removedTrappingInits.size(); ++i) {
+      auto* curr = moduleRemover.removedTrappingInits[i];
       auto name = Names::getValidGlobalName(
         wasm, std::string("gto-removed-") + std::to_string(i));
       wasm.addGlobal(
         Builder::makeGlobal(name, curr->type, curr, Builder::Immutable));
     }
+
+    IndexUpdater moduleUpdater(*this);
+    moduleUpdater.runOnModuleCode(getPassRunner(), &wasm);
   }
 };
 

test/lit/passes/gto-jsinterop.wast

@@ -1711,3 +1711,45 @@
     )
   )
 )
+
+(module
+  ;; A struct.get on a descriptor that receives an i8 placeholder is nested
+  ;; inside a struct.new whose field is removed. FieldRemover runs
+  ;; ChildLocalizer (which uses EffectAnalyzer) on the struct.new before
+  ;; IndexUpdater increments the struct.get index, avoiding an out-of-bounds
+  ;; access into the old struct type's fields.
+  (rec
+    ;; CHECK:      (rec
+    ;; CHECK-NEXT:  (type $struct (descriptor $desc) (struct))
+    (type $struct (descriptor $desc) (struct (field externref)))
+    ;; CHECK:       (type $desc (describes $struct) (struct (field i8) (field externref)))
+    (type $desc (describes $struct) (struct (field (mut externref))))
+  )
+
+  ;; CHECK:      (type $2 (func (result structref)))
+
+  ;; CHECK:      (export "export" (func $test))
+  (export "export" (func $test))
+
+  ;; CHECK:      (func $test (type $2) (result structref)
+  ;; CHECK-NEXT:  (local $d (ref null $desc))
+  ;; CHECK-NEXT:  (local $1 externref)
+  ;; CHECK-NEXT:  (local.set $1
+  ;; CHECK-NEXT:   (struct.get $desc 1
+  ;; CHECK-NEXT:    (local.get $d)
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT:  (struct.new_default_desc $struct
+  ;; CHECK-NEXT:   (struct.new_default $desc)
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT: )
+  (func $test (result structref)
+    (local $d (ref null $desc))
+    (struct.new_desc $struct
+      (struct.get $desc 0
+        (local.get $d)
+      )
+      (struct.new_default $desc)
+    )
+  )
+)