Fuzzer: Better fuzzing of globals

src/support/topological_sort.h

@@ -27,7 +27,7 @@ namespace wasm {
 // CRTP utility that provides an iterator through arbitrary directed acyclic
 // graphs of data that will visit the data in a topologically sorted order
 // (https://en.wikipedia.org/wiki/Topological_sorting). In other words, the
-// iterator will produce each item only after all that items predecessors have
+// iterator will produce each item only after all that item's predecessors have
 // been produced.
 //
 // Subclasses should call `push` on all the root items in their constructors and

src/tools/fuzzing.h

@@ -106,6 +106,8 @@ class TranslateToFuzzReader {
 
   std::unordered_map<Type, std::vector<Name>> globalsByType;
   std::unordered_map<Type, std::vector<Name>> mutableGlobalsByType;
+  std::unordered_map<Type, std::vector<Name>> immutableGlobalsByType;
+  std::unordered_map<Type, std::vector<Name>> importedImmutableGlobalsByType;
 
   std::vector<Type> loggableTypes;
 

src/tools/fuzzing/fuzzing.cpp

@@ -379,26 +379,46 @@ void TranslateToFuzzReader::setupGlobals() {
     }
   }
 
+  auto useGlobalLater = [&](Global* global) {
+    auto type = global->type;
+    auto name = global->name;
+    globalsByType[type].push_back(name);
+    if (global->mutable_) {
+      mutableGlobalsByType[type].push_back(name);
+    } else {
+      immutableGlobalsByType[type].push_back(name);
+      if (global->imported()) {
+        importedImmutableGlobalsByType[type].push_back(name);
+      }
+    }
+  };
+
   // Randomly assign some globals from initial content to be ignored for the
   // fuzzer to use. Such globals will only be used from initial content. This is
   // important to preserve some real-world patterns, like the "once" pattern in
   // which a global is used in one function only. (If we randomly emitted gets
   // and sets of such globals, we'd with very high probability end up breaking
   // that pattern, and not fuzzing it at all.)
-  //
-  // Pick a percentage of initial globals to ignore later down when we decide
-  // which to allow uses from.
-  auto numInitialGlobals = wasm.globals.size();
-  unsigned percentIgnoredInitialGlobals = 0;
-  if (numInitialGlobals) {
-    // Only generate this random number if it will be used.
-    percentIgnoredInitialGlobals = upTo(100);
+  if (!wasm.globals.empty()) {
+    unsigned percentUsedInitialGlobals = upTo(100);
+    for (auto& global : wasm.globals) {
+      if (upTo(100) < percentUsedInitialGlobals) {
+        useGlobalLater(global.get());
+      }
+    }
   }
 
   // Create new random globals.
   for (size_t index = upTo(MAX_GLOBALS); index > 0; --index) {
     auto type = getConcreteType();
-    auto* init = makeConst(type);
+    // Prefer immutable ones as they can be used in global.gets in other
+    // globals, for more interesting patterns.
+    auto mutability = oneIn(3) ? Builder::Mutable : Builder::Immutable;
+
+    // We can only make something trivial (like a constant) in a global
+    // initializer.
+    auto* init = makeTrivial(type);
+
     if (!FindAll<RefAs>(init).list.empty()) {
       // When creating this initial value we ended up emitting a RefAs, which
       // means we had to stop in the middle of an overly-nested struct or array,
@@ -407,27 +427,16 @@ void TranslateToFuzzReader::setupGlobals() {
       // validate in a global. Switch to something safe instead.
       type = getMVPType();
       init = makeConst(type);
+    } else if (type.isTuple() && !init->is<TupleMake>()) {
+      // For now we disallow anything but tuple.make at the top level of tuple
+      // globals (see details in wasm-binary.cpp). In the future we may allow
+      // global.get or other things here.
+      init = makeConst(type);
+      assert(init->is<TupleMake>());
     }
-    auto mutability = oneIn(2) ? Builder::Mutable : Builder::Immutable;
     auto global = builder.makeGlobal(
       Names::getValidGlobalName(wasm, "global$"), type, init, mutability);
-    wasm.addGlobal(std::move(global));
-  }
-
-  // Set up data structures for picking globals later for get/set operations.
-  for (Index i = 0; i < wasm.globals.size(); i++) {
-    auto& global = wasm.globals[i];
-
-    // Apply the chance for initial globals to be ignored, see above.
-    if (i < numInitialGlobals && upTo(100) < percentIgnoredInitialGlobals) {
-      continue;
-    }
-
-    // This is a global we can use later, note it.
-    globalsByType[global->type].push_back(global->name);
-    if (global->mutable_) {
-      mutableGlobalsByType[global->type].push_back(global->name);
-    }
+    useGlobalLater(wasm.addGlobal(std::move(global)));
   }
 }
 
@@ -1477,9 +1486,12 @@ Expression* TranslateToFuzzReader::makeTrivial(Type type) {
     // using it before it was set, which would trap.
     if (funcContext && oneIn(type.isNonNullable() ? 10 : 2)) {
       return makeLocalGet(type);
-    } else {
-      return makeConst(type);
     }
+
+    // Either make a const, or a global.get (which may fail to find a suitable
+    // global, especially in a non-function context, and if so it will make a
+    // constant instead).
+    return oneIn(2) ? makeConst(type) : makeGlobalGet(type);
   } else if (type == Type::none) {
     return makeNop(type);
   }
@@ -1900,9 +1912,17 @@ Expression* TranslateToFuzzReader::makeLocalSet(Type type) {
 }
 
 Expression* TranslateToFuzzReader::makeGlobalGet(Type type) {
-  auto it = globalsByType.find(type);
-  if (it == globalsByType.end() || it->second.empty()) {
-    return makeTrivial(type);
+  // In a non-function context, like in another global, we can only get from an
+  // immutable global. Whether GC is enabled also matters, as it allows getting
+  // from a non-import.
+  auto& relevantGlobals =
+    funcContext ? globalsByType
+                : (wasm.features.hasGC() ? immutableGlobalsByType
+                                         : importedImmutableGlobalsByType);
+  auto it = relevantGlobals.find(type);
+  // If we have no such relevant globals give up and emit a constant instead.
+  if (it == relevantGlobals.end() || it->second.empty()) {
+    return makeConst(type);
   }
 
   auto name = pick(it->second);

src/tools/fuzzing/parameters.h

@@ -30,7 +30,7 @@ constexpr int MAX_PARAMS = 10;
 constexpr int MAX_VARS = 20;
 
 // The maximum number of globals in a module.
-constexpr int MAX_GLOBALS = 20;
+constexpr int MAX_GLOBALS = 30;
 
 // The maximum number of tuple elements.
 constexpr int MAX_TUPLE_SIZE = 6;

src/wasm/wasm-binary.cpp

@@ -572,8 +572,23 @@ void WasmBinaryWriter::writeGlobals() {
       o << U32LEB(global->mutable_);
       if (global->type.size() == 1) {
         writeExpression(global->init);
+      } else if (auto* make = global->init->dynCast<TupleMake>()) {
+        // Emit the proper lane for this global.
+        writeExpression(make->operands[i]);
       } else {
-        writeExpression(global->init->cast<TupleMake>()->operands[i]);
+        // For now tuple globals must contain tuple.make. We could perhaps
+        // support more operations, like global.get, but the code would need to
+        // look something like this:
+        //
+        //   auto parentIndex = getGlobalIndex(get->name);
+        //   o << int8_t(BinaryConsts::GlobalGet) << U32LEB(parentIndex + i);
+        //
+        // That is, we must emit the instruction here, and not defer to
+        // writeExpression, as writeExpression writes an entire expression at a
+        // time (and not just one of the lanes). As emitting an instruction here
+        // is less clean, and there is no important use case for global.get of
+        // one tuple global to another, we disallow this.
+        WASM_UNREACHABLE("unsupported tuple global operation");
       }
       o << int8_t(BinaryConsts::End);
       ++i;

test/passes/fuzz_metrics_noprint.bin.txt

@@ -1,34 +1,34 @@
 total
- [exports]      : 18      
- [funcs]        : 21      
+ [exports]      : 23      
+ [funcs]        : 34      
  [globals]      : 9       
  [imports]      : 4       
  [memories]     : 1       
  [memory-data]  : 2       
- [table-data]   : 7       
+ [table-data]   : 6       
  [tables]       : 1       
  [tags]         : 0       
- [total]        : 11685   
- [vars]         : 64      
- Binary         : 848     
- Block          : 1846    
- Break          : 456     
- Call           : 275     
- CallIndirect   : 117     
- Const          : 1952    
- Drop           : 86      
- GlobalGet      : 844     
- GlobalSet      : 679     
- If             : 625     
- Load           : 236     
- LocalGet       : 1050    
- LocalSet       : 764     
- Loop           : 301     
- Nop            : 143     
- RefFunc        : 7       
- Return         : 103     
- Select         : 77      
- Store          : 111     
- Switch         : 3       
- Unary          : 835     
- Unreachable    : 327     
+ [total]        : 4303    
+ [vars]         : 100     
+ Binary         : 355     
+ Block          : 684     
+ Break          : 149     
+ Call           : 219     
+ CallIndirect   : 23      
+ Const          : 643     
+ Drop           : 50      
+ GlobalGet      : 367     
+ GlobalSet      : 258     
+ If             : 206     
+ Load           : 78      
+ LocalGet       : 339     
+ LocalSet       : 236     
+ Loop           : 93      
+ Nop            : 41      
+ RefFunc        : 6       
+ Return         : 45      
+ Select         : 41      
+ Store          : 36      
+ Switch         : 1       
+ Unary          : 304     
+ Unreachable    : 129     

test/passes/translate-to-fuzz_all-features_metrics_noprint.txt

@@ -1,47 +1,59 @@
 total
- [exports]      : 5       
- [funcs]        : 7       
- [globals]      : 14      
+ [exports]      : 3       
+ [funcs]        : 4       
+ [globals]      : 24      
  [imports]      : 5       
  [memories]     : 1       
  [memory-data]  : 20      
- [table-data]   : 2       
+ [table-data]   : 3       
  [tables]       : 1       
  [tags]         : 1       
- [total]        : 467     
- [vars]         : 40      
- ArrayGet       : 1       
- ArrayLen       : 1       
- ArrayNew       : 6       
+ [total]        : 750     
+ [vars]         : 30      
+ ArrayCopy      : 1       
+ ArrayGet       : 2       
+ ArrayLen       : 5       
+ ArrayNew       : 24      
  ArrayNewFixed  : 1       
- Binary         : 67      
- Block          : 44      
- Break          : 5       
- Call           : 21      
- Const          : 106     
- Drop           : 7       
- GlobalGet      : 20      
- GlobalSet      : 18      
- If             : 14      
- Load           : 17      
- LocalGet       : 45      
- LocalSet       : 28      
- Loop           : 3       
- MemoryFill     : 1       
- Nop            : 5       
- RefAs          : 1       
- RefCast        : 1       
+ ArraySet       : 1       
+ AtomicCmpxchg  : 1       
+ AtomicFence    : 1       
+ AtomicRMW      : 1       
+ Binary         : 84      
+ Block          : 58      
+ Break          : 12      
+ Call           : 13      
+ Const          : 175     
+ Drop           : 2       
+ GlobalGet      : 45      
+ GlobalSet      : 20      
+ I31Get         : 2       
+ If             : 21      
+ Load           : 20      
+ LocalGet       : 70      
+ LocalSet       : 46      
+ Loop           : 7       
+ MemoryCopy     : 1       
+ Nop            : 11      
+ Pop            : 3       
+ RefAs          : 7       
  RefEq          : 1       
- RefFunc        : 3       
- RefI31         : 4       
- RefNull        : 3       
- Return         : 3       
- Select         : 2       
+ RefFunc        : 5       
+ RefI31         : 7       
+ RefIsNull      : 3       
+ RefNull        : 19      
+ RefTest        : 3       
+ Return         : 2       
+ SIMDTernary    : 1       
+ Select         : 3       
  Store          : 1       
- StringConst    : 3       
+ StringConst    : 8       
  StringEncode   : 1       
- StringEq       : 1       
- StructNew      : 5       
- TupleMake      : 5       
- Unary          : 13      
+ StringMeasure  : 1       
+ StringWTF16Get : 1       
+ StructGet      : 1       
+ StructNew      : 21      
+ Try            : 3       
+ TupleMake      : 6       
+ Unary          : 19      
  Unreachable    : 10