diff --git a/src/passes/GlobalTypeOptimization.cpp b/src/passes/GlobalTypeOptimization.cpp index 0a158aa7f61..8cfa4960cc9 100644 --- a/src/passes/GlobalTypeOptimization.cpp +++ b/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> { 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> { + bool isFunctionParallel() override { return true; } + + GlobalTypeOptimization& parent; + + IndexUpdater(GlobalTypeOptimization& parent) : parent(parent) {} + + std::unique_ptr create() override { + return std::make_unique(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(*this)); + runner.add(std::make_unique(*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); } }; diff --git a/test/lit/passes/gto-jsinterop.wast b/test/lit/passes/gto-jsinterop.wast index 4cabdc0dbc5..4a2e24d8073 100644 --- a/test/lit/passes/gto-jsinterop.wast +++ b/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) + ) + ) +)