[Wasm GC] Improve GC operation coverage by using locals more (#5661)

When we emit e.g. a struct.get's reference, this PR makes us prefer a non-nullable
value, and even to reuse an existing local if possible. By doing that we reduce
the risk of a trap, and also by using locals we end up testing operations on the
same data, like this:

x = new A();
x.a = ..
foo(x.a)

In contrast, without this PR each of those x. uses might be new A().

2 files changed, 60 insertions, 19 deletions

diff --git a/src/tools/fuzzing.h b/src/tools/fuzzing.h
index c0f6ab335..abc0a4248 100644
--- a/src/tools/fuzzing.h
+++ b/src/tools/fuzzing.h
@@ -302,6 +302,7 @@ private:
   Expression* makeLoad(Type type);
   Expression* makeNonAtomicStore(Type type);
   Expression* makeStore(Type type);
+
   // Makes a small change to a constant value.
   Literal tweak(Literal value);
   Literal makeLiteral(Type type);
@@ -318,6 +319,11 @@ private:
   Expression* makeBasicRef(Type type);
   Expression* makeCompoundRef(Type type);
 
+  // Similar to makeBasic/CompoundRef, but indicates that this value will be
+  // used in a place that will trap on null. For example, the reference of a
+  // struct.get or array.set would use this.
+  Expression* makeTrappingRefUse(HeapType type);
+
   Expression* buildUnary(const UnaryArgs& args);
   Expression* makeUnary(Type type);
   Expression* buildBinary(const BinaryArgs& args);
diff --git a/src/tools/fuzzing/fuzzing.cpp b/src/tools/fuzzing/fuzzing.cpp
index 062e66260..c88b2f13d 100644
--- a/src/tools/fuzzing/fuzzing.cpp
+++ b/src/tools/fuzzing/fuzzing.cpp
@@ -2316,11 +2316,19 @@ Expression* TranslateToFuzzReader::makeCompoundRef(Type type) {
     return builder.makeRefNull(heapType);
   }
 
-  // If the type is non-nullable, and we've run out of input, emit a cast to
-  // make this validate, but it will trap at runtime which is not ideal. This at
-  // least avoids infinite recursion here, and we emit a valid (but not that
-  // useful) wasm.
+  // If the type is non-nullable, but we've run out of input, then we need to do
+  // something here to avoid infinite recursion. In the worse case we'll emit a
+  // cast to non-null of a null, which validates, but it will trap at runtime
+  // which is not ideal.
   if (type.isNonNullable() && (random.finished() || nesting >= LIMIT)) {
+    // If we have a function context then we can at least emit a local.get,
+    // perhaps, which is less bad. Note that we need to check typeLocals
+    // manually here to avoid infinite recursion (as makeLocalGet will fall back
+    // to us, if there is no local).
+    // TODO: we could also look for locals containing subtypes
+    if (funcContext && !funcContext->typeLocals[type].empty()) {
+      return makeLocalGet(type);
+    }
     return builder.makeRefAs(RefAsNonNull, builder.makeRefNull(heapType));
   }
 
@@ -2369,6 +2377,43 @@ Expression* TranslateToFuzzReader::makeCompoundRef(Type type) {
   }
 }
 
+Expression* TranslateToFuzzReader::makeTrappingRefUse(HeapType type) {
+  auto percent = upTo(100);
+  // Only give a low probability to emit a nullable reference.
+  if (percent < 5) {
+    return make(Type(type, Nullable));
+  }
+  // Otherwise, usually emit a non-nullable one.
+  auto nonNull = Type(type, NonNullable);
+  if (percent < 70 || !funcContext) {
+    return make(nonNull);
+  }
+  // With significant probability, try to use an existing value. it is better to
+  // have patterns like this:
+  //
+  //  (local.set $ref (struct.new $..
+  //  (struct.get (local.get $ref))
+  //
+  // Rather than constantly operating on new data each time:
+  //
+  //  (local.set $ref (struct.new $..
+  //  (struct.get (struct.new $..
+  //
+  // By using local values more, we get more coverage of interesting sequences
+  // of reads and writes to the same objects.
+  auto& typeLocals = funcContext->typeLocals[nonNull];
+  if (!typeLocals.empty()) {
+    return builder.makeLocalGet(pick(typeLocals), nonNull);
+  }
+  // Add a new local and tee it, so later operations can use it.
+  auto index = builder.addVar(funcContext->func, nonNull);
+  // Note we must create the child ref here before adding the local to
+  // typeLocals (or else we might end up using it prematurely).
+  auto* tee = builder.makeLocalTee(index, make(nonNull), nonNull);
+  funcContext->typeLocals[nonNull].push_back(index);
+  return tee;
+}
+
 Expression* TranslateToFuzzReader::buildUnary(const UnaryArgs& args) {
   return builder.makeUnary(args.a, args.b);
 }
@@ -3270,12 +3315,7 @@ Expression* TranslateToFuzzReader::makeStructGet(Type type) {
   auto& structFields = typeStructFields[type];
   assert(!structFields.empty());
   auto [structType, fieldIndex] = pick(structFields);
-  // TODO: also nullable ones? that would increase the risk of traps
-  // TODO: Ensure a good chance to use a local.get or tee here, as we want to
-  //       test the same reference having multiple sets/gets on it, and not
-  //       gets/sets of struct.news everywhere. Also in struct.set, array.get,
-  //       array.set.
-  auto* ref = make(Type(structType, NonNullable));
+  auto* ref = makeTrappingRefUse(structType);
   // TODO: fuzz signed and unsigned
   return builder.makeStructGet(fieldIndex, ref, type);
 }
@@ -3287,8 +3327,7 @@ Expression* TranslateToFuzzReader::makeStructSet(Type type) {
   }
   auto [structType, fieldIndex] = pick(mutableStructFields);
   auto fieldType = structType.getStruct().fields[fieldIndex].type;
-  // TODO: also nullable ones? that would increase the risk of traps
-  auto* ref = make(Type(structType, NonNullable));
+  auto* ref = makeTrappingRefUse(structType);
   auto* value = make(fieldType);
   return builder.makeStructSet(fieldIndex, ref, value);
 }
@@ -3321,8 +3360,7 @@ Expression* TranslateToFuzzReader::makeArrayGet(Type type) {
   auto& arrays = typeArrays[type];
   assert(!arrays.empty());
   auto arrayType = pick(arrays);
-  // TODO: also nullable ones? that would increase the risk of traps
-  auto* ref = make(Type(arrayType, NonNullable));
+  auto* ref = makeTrappingRefUse(arrayType);
   auto* index = make(Type::i32);
   // Only rarely emit a plain get which might trap. See related logic in
   // ::makePointer().
@@ -3348,8 +3386,7 @@ Expression* TranslateToFuzzReader::makeArraySet(Type type) {
   auto arrayType = pick(mutableArrays);
   auto elementType = arrayType.getArray().element.type;
   auto* index = make(Type::i32);
-  // TODO: also nullable ones? that would increase the risk of traps
-  auto* ref = make(Type(arrayType, NonNullable));
+  auto* ref = makeTrappingRefUse(arrayType);
   auto* value = make(elementType);
   // Only rarely emit a plain get which might trap. See related logic in
   // ::makePointer().
@@ -3369,9 +3406,7 @@ Expression* TranslateToFuzzReader::makeArraySet(Type type) {
 Expression* TranslateToFuzzReader::makeI31Get(Type type) {
   assert(type == Type::i32);
   assert(wasm.features.hasReferenceTypes() && wasm.features.hasGC());
-  // TODO: Maybe this should be nullable?
-  // https://github.com/WebAssembly/gc/issues/312
-  auto* i31 = make(Type(HeapType::i31, NonNullable));
+  auto* i31 = makeTrappingRefUse(HeapType::i31);
   return builder.makeI31Get(i31, bool(oneIn(2)));
 }