Merge branch 'main' into unsubypting-propagate-left

Author: tlively

scripts/fuzz_opt.py

@@ -1582,6 +1582,9 @@ def handle(self, wasm):
 
         run([in_bin('wasm-split'), wasm, '--split',
              '--split-funcs', ','.join(split_funcs),
+             # make the new exports easily identifiable, as we need to ignore
+             # them in part of fuzz_shell.js
+             '--export-prefix=__fuzz_split_',
              '--primary-output', primary,
              '--secondary-output', secondary] + split_feature_opts)
 
@@ -1616,7 +1619,12 @@ def optimize(name):
 
         # get the output from the split modules, linking them using JS
         # TODO run liftoff/turboshaft/etc.
-        linked_output = run_d8_wasm(primary, args=[secondary, exports_to_call])
+        args = [
+            secondary,
+            exports_to_call,
+            '--fuzz-split',
+        ]
+        linked_output = run_d8_wasm(primary, args=args)
         linked_output = fix_output(linked_output)
 
         # see D8.can_compare_to_self: we cannot compare optimized outputs if

scripts/fuzz_shell.js

@@ -47,9 +47,24 @@ if (!binary) {
 // passed a final parameter in the form of "exports:X,Y,Z" then we call
 // specifically the exports X, Y, and Z.
 var exportsToCall;
-if (argv.length > 0 && argv[argv.length - 1].startsWith('exports:')) {
-  exportsToCall = argv[argv.length - 1].substr('exports:'.length).split(',');
-  argv.pop();
+
+// Passing --fuzz-split makes us treat the two input files as split off
+// from a single one, and we will run them as that single file (ignoring extra
+// exports from the second one, and from wasm-split itself). This allows us to
+// get the same behavior from split modules as before the split.
+var fuzzSplit = false;
+
+for (var i = 0; i < argv.length; i++) {
+  var curr = argv[i];
+  if (curr.startsWith('exports:')) {
+    exportsToCall = curr.substr('exports:'.length).split(',');
+    argv.splice(i, 1);
+    i--;
+  } else if (curr == '--fuzz-split') {
+    fuzzSplit = true;
+    argv.splice(i, 1);
+    i--;
+  }
 }
 
 // If a second parameter is given, it is a second binary that we will link in
@@ -417,8 +432,15 @@ if (secondBinary) {
   });
 }
 
-// Compile and instantiate a wasm file.
-function build(binary) {
+// Compile and instantiate a wasm file. Receives the binary to build, and
+// whether it is the second one.
+function build(binary, isSecond) {
+  if (fuzzSplit && isSecond) {
+    assert(secondBinary);
+    // Provide the primary module's exports to the secondary.
+    imports['primary'] = exports;
+  }
+
   var module = new WebAssembly.Module(binary);
 
   var instance;
@@ -429,6 +451,14 @@ function build(binary) {
     quit();
   }
 
+  // Do not add the second instance's exports to the list, as that would be
+  // noticeable by calls to call-export-*. When fuzzing wasm-split, we want the
+  // original module's exports to be provided from the primary module, and it is
+  // the only interface to the outside.
+  if (fuzzSplit && isSecond) {
+    return;
+  }
+
   // Update the exports. Note that this adds onto |exports| and |exportList|,
   // which is intentional: if we build another wasm, or build this one more
   // than once, we want to be able to call them all, so we unify all their
@@ -446,6 +476,13 @@ function build(binary) {
     var value = instance.exports[key];
     value = wrapExportForJSPI(value);
     exports[key] = value;
+
+    if (fuzzSplit && key.startsWith('__fuzz_split_')) {
+      // We are fuzzing wasm-split, and this is a new export generated by
+      // wasm-split. Do not note these exports as callable from call-export*,
+      // as they do not match the original pre-split module.
+      continue;
+    }
     exportList.push({ name: key, value: value });
   }
 }
@@ -559,7 +596,7 @@ build(binary);
 
 // Build the second wasm, if one was provided.
 if (secondBinary) {
-  build(secondBinary);
+  build(secondBinary, true);
 }
 
 // Run.

scripts/test/fuzzing.py

@@ -107,6 +107,9 @@
     'instrument-branch-hints.wast',
     # Contains a subtype chain that exceeds depth limits.
     'reorder-types-real.wast',
+    # Contains a name with "__fuzz_split", indicating it is emitted by
+    # wasm-split, confusing the fuzzer because it is in the initial content.
+    'fuzz_shell_second.wast',
 ]
 
 

src/passes/AbstractTypeRefining.cpp

@@ -394,6 +394,22 @@ struct AbstractTypeRefining : public Pass {
         }
       }
 
+      // If we optimize away a descriptor type, then we must fix up any
+      // ref.get_desc of it, as ReFinalize would fix us up to return it. This
+      // can only happen when no such descriptor is created, which means the
+      // instruction will never be reached (no struct with such a descriptor was
+      // created).
+      void visitRefGetDesc(RefGetDesc* curr) {
+        auto optimized = getOptimized(curr->type);
+        if (!optimized) {
+          return;
+        }
+
+        Builder builder(*getModule());
+        replaceCurrent(builder.makeSequence(builder.makeDrop(curr->ref),
+                                            builder.makeUnreachable()));
+      }
+
       void visitBrOn(BrOn* curr) {
         if (curr->op == BrOnNull || curr->op == BrOnNonNull) {
           return;

src/passes/ConstantFieldPropagation.cpp

@@ -220,6 +220,15 @@ struct FunctionOptimizer : public WalkerPass<PostWalker<FunctionOptimizer>> {
 
   void visitRefGetDesc(RefGetDesc* curr) {
     optimizeRead(curr, curr->ref, StructUtils::DescriptorIndex);
+
+    // RefGetDesc has the interesting property that we can write a value into
+    // the field that cannot be read from it: it is valid to write a null, but
+    // a null can never be read (it would have trapped on the write). Fix that
+    // up as needed to not break validation.
+    if (!Type::isSubType(getCurrent()->type, curr->type)) {
+      Builder builder(*getModule());
+      replaceCurrent(builder.makeRefAs(RefAsNonNull, getCurrent()));
+    }
   }
 
   void optimizeRead(Expression* curr,
@@ -485,6 +494,10 @@ struct PCVScanner
                       Index index,
                       PossibleConstantValues& info) {
     info.note(expr, *getModule());
+    // TODO: For descriptors we can ignore nullable values that are written, as
+    //       they trap. That is, if one place writes a null and another writes a
+    //       global, only the global is readable, and we can optimize there -
+    //       the null is not a second value.
   }
 
   void noteDefault(Type fieldType,

src/passes/Heap2Local.cpp

@@ -604,6 +604,10 @@ struct Struct2Local : PostWalker<Struct2Local> {
   Builder builder;
   const FieldList& fields;
 
+  // The descriptor can arrive as nullable, but we trap if it is null, so there
+  // is only something to store if it is non-nullable, and we store it that way.
+  Type descType;
+
   Struct2Local(StructNew* allocation,
                EscapeAnalyzer& analyzer,
                Function* func,
@@ -616,7 +620,8 @@ struct Struct2Local : PostWalker<Struct2Local> {
       localIndexes.push_back(builder.addVar(func, field.type));
     }
     if (allocation->desc) {
-      localIndexes.push_back(builder.addVar(func, allocation->desc->type));
+      descType = allocation->desc->type.with(NonNullable);
+      localIndexes.push_back(builder.addVar(func, descType));
     }
 
     // Replace the things we need to using the visit* methods.
@@ -744,7 +749,7 @@ struct Struct2Local : PostWalker<Struct2Local> {
       }
     }
     if (curr->desc) {
-      tempIndexes.push_back(builder.addVar(func, curr->desc->type));
+      tempIndexes.push_back(builder.addVar(func, descType));
     }
 
     // Store the initial values into the temp locals.
@@ -773,8 +778,7 @@ struct Struct2Local : PostWalker<Struct2Local> {
       contents.push_back(builder.makeLocalSet(localIndexes[i], val));
     }
     if (curr->desc) {
-      auto* val =
-        builder.makeLocalGet(tempIndexes[numTemps - 1], curr->desc->type);
+      auto* val = builder.makeLocalGet(tempIndexes[numTemps - 1], descType);
       contents.push_back(
         builder.makeLocalSet(localIndexes[fields.size()], val));
     }
@@ -902,13 +906,12 @@ struct Struct2Local : PostWalker<Struct2Local> {
         } else {
           // The cast succeeds iff the optimized allocation's descriptor is the
           // same as the given descriptor and traps otherwise.
-          auto type = allocation->desc->type;
           replaceCurrent(builder.blockify(
             builder.makeDrop(curr->ref),
             builder.makeIf(
               builder.makeRefEq(
                 curr->desc,
-                builder.makeLocalGet(localIndexes[fields.size()], type)),
+                builder.makeLocalGet(localIndexes[fields.size()], descType)),
               builder.makeRefNull(allocation->type.getHeapType()),
               builder.makeUnreachable())));
         }
@@ -939,19 +942,13 @@ struct Struct2Local : PostWalker<Struct2Local> {
       return;
     }
 
-    auto type = allocation->desc->type;
-    Expression* value = builder.makeLocalGet(localIndexes[fields.size()], type);
-    if (type != curr->type) {
-      // We know exactly the allocation that flows into this expression, so we
-      // know the exact type of the descriptor. This type may be more precise
-      // than the static type of this expression.
-      refinalize = true;
-      if (type.isNull()) {
-        // This traps.
-        value = builder.makeUnreachable();
-      }
-    }
+    auto descIndex = localIndexes[fields.size()];
+    Expression* value = builder.makeLocalGet(descIndex, descType);
     replaceCurrent(builder.blockify(builder.makeDrop(curr->ref), value));
+
+    // After removing the ref.get_desc, a null may be falling through,
+    // requiring refinalization to update parents.
+    refinalize = true;
   }
 
   void visitStructSet(StructSet* curr) {

src/passes/Print.cpp

@@ -3903,4 +3903,12 @@ std::ostream& operator<<(std::ostream& o, wasm::ModuleType pair) {
   return o;
 }
 
+std::ostream& operator<<(std::ostream& o, wasm::ModuleHeapType pair) {
+  if (auto it = pair.first.typeNames.find(pair.second);
+      it != pair.first.typeNames.end()) {
+    return o << it->second.name;
+  }
+  return o << "(unnamed)";
+}
+
 } // namespace std

src/passes/Unsubtyping.cpp

@@ -129,15 +129,6 @@ template<typename K, typename V> using Map = std::unordered_map<K, V>;
 template<typename T> using Set = std::unordered_set<T>;
 #endif
 
-#if UNSUBTYPING_DEBUG
-Name getTypeName(Module& wasm, HeapType type) {
-  if (auto it = wasm.typeNames.find(type); it != wasm.typeNames.end()) {
-    return it->second.name;
-  }
-  return Name("(unnamed)");
-}
-#endif
-
 // A tree (or rather a forest) of types with the ability to query and set
 // supertypes in constant time and efficiently iterate over supertypes and
 // subtypes.
@@ -291,13 +282,13 @@ struct TypeTree {
 #if UNSUBTYPING_DEBUG
   void dump(Module& wasm) {
     for (auto& node : nodes) {
-      std::cerr << getTypeName(wasm, node.type);
+      std::cerr << ModuleHeapType(wasm, node.type);
       if (auto super = getSupertype(node.type)) {
-        std::cerr << " <: " << getTypeName(wasm, *super);
+        std::cerr << " <: " << ModuleHeapType(wasm, *super);
       }
       std::cerr << ", children:";
       for (auto child : node.children) {
-        std::cerr << " " << getTypeName(wasm, nodes[child].type);
+        std::cerr << " " << ModuleHeapType(wasm, nodes[child].type);
       }
       std::cerr << '\n';
     }
@@ -360,8 +351,8 @@ struct Unsubtyping : Pass {
     if (sub == super || sub.isBottom()) {
       return;
     }
-    DBG(std::cerr << "noting " << getTypeName(*wasm, sub)
-                  << " <: " << getTypeName(*wasm, super) << '\n');
+    DBG(std::cerr << "noting " << ModuleHeapType(*wasm, sub)
+                  << " <: " << ModuleHeapType(*wasm, super) << '\n');
     work.push_back({sub, super});
   }
 
@@ -520,8 +511,8 @@ struct Unsubtyping : Pass {
   }
 
   void process(HeapType sub, HeapType super) {
-    DBG(std::cerr << "processing " << getTypeName(*wasm, sub)
-                  << " <: " << getTypeName(*wasm, super) << '\n');
+    DBG(std::cerr << "processing " << ModuleHeapType(*wasm, sub)
+                  << " <: " << ModuleHeapType(*wasm, super) << '\n');
     assert(HeapType::isSubType(sub, super));
     auto oldSuper = types.getSupertype(sub);
     if (oldSuper) {

src/wasm.h

@@ -2623,8 +2623,9 @@ class Module {
 // Utility for printing an expression with named types.
 using ModuleExpression = std::pair<Module&, Expression*>;
 
-// Utility for printing an type with a name, if the module defines a name.
+// Utilities for printing an type with a name, if the module defines a name.
 using ModuleType = std::pair<Module&, Type>;
+using ModuleHeapType = std::pair<Module&, HeapType>;
 
 // Utility for printing only the top level of an expression. Named types will be
 // used if `module` is non-null.
@@ -2648,6 +2649,7 @@ std::ostream& operator<<(std::ostream& o, wasm::Expression& expression);
 std::ostream& operator<<(std::ostream& o, wasm::ModuleExpression pair);
 std::ostream& operator<<(std::ostream& o, wasm::ShallowExpression expression);
 std::ostream& operator<<(std::ostream& o, wasm::ModuleType pair);
+std::ostream& operator<<(std::ostream& o, wasm::ModuleHeapType pair);
 
 } // namespace std
 

test/lit/node/fuzz_shell_second.wast

@@ -5,6 +5,10 @@
   (func $first (export "first") (result i32)
     (i32.const 42)
   )
+
+  (func $split (export "__fuzz_split_func") (result i32)
+    (i32.const 999)
+  )
 )
 
 ;; Build both files to binary.
@@ -18,6 +22,8 @@
 ;;
 ;; CHECK: [fuzz-exec] calling first
 ;; CHECK: [fuzz-exec] note result: first => 42
+;; CHECK: [fuzz-exec] calling __fuzz_split_func
+;; CHECK: [fuzz-exec] note result: __fuzz_split_func => 999
 ;; CHECK: [fuzz-exec] calling second
 ;; CHECK: [fuzz-exec] note result: second => 1337
 
@@ -29,4 +35,16 @@
 ;; REVERSE: [fuzz-exec] note result: second => 1337
 ;; REVERSE: [fuzz-exec] calling first
 ;; REVERSE: [fuzz-exec] note result: first => 42
+;; REVERSE: [fuzz-exec] calling __fuzz_split_func
+;; REVERSE: [fuzz-exec] note result: __fuzz_split_func => 999
+
+;; Run with --fuzz-split, which does not run exports from the second one,
+;; and also ignores exports starting with "__fuzz_split_"
+;;
+;; RUN: node %S/../../../scripts/fuzz_shell.js %t.wasm %t.second.wasm --fuzz-split | filecheck %s --check-prefix=WASM_SPLIT
+;;
+;; WASM_SPLIT: [fuzz-exec] calling first
+;; WASM_SPLIT: [fuzz-exec] note result: first => 42
+;; WASM_SPLIT-NOT: [fuzz-exec] calling second
+;; WASM_SPLIT-NOT: [fuzz-exec] calling __fuzz_split_func