Compare public fields in execution-results.h

src/tools/execution-results.h

@@ -20,9 +20,11 @@
 
 #include <deque>
 #include <memory>
+#include <unordered_set>
 
 #include "ir/import-names.h"
 #include "ir/import-utils.h"
+#include "ir/module-utils.h"
 #include "shell-interface.h"
 #include "support/utilities.h"
 #include "wasm-type.h"
@@ -437,9 +439,18 @@ struct ExecutionResults {
   // If set, we should ignore this and not compare it to anything.
   bool ignore = false;
 
+  std::unordered_set<HeapType> publicTypes;
+
   // Execute a module and collect the results. Optionally, provide a second
   // module to link with it (like fuzz_shell's second module).
   void collect(Module& wasm, Module* second = nullptr) {
+    auto publicVec = ModuleUtils::getPublicHeapTypes(wasm);
+    publicTypes.insert(publicVec.begin(), publicVec.end());
+    if (second) {
+      auto secondPublicVec = ModuleUtils::getPublicHeapTypes(*second);
+      publicTypes.insert(secondPublicVec.begin(), secondPublicVec.end());
+    }
+
     try {
       // Instantiate the first module.
       LoggingExternalInterface interface(loggings, wasm);
@@ -537,13 +548,22 @@ struct ExecutionResults {
   }
 
   bool areEqual(Literal a, Literal b) {
+    // Values may be recursive, so naively comparing their structures might
+    // recurse forever. Instead we need to check coinductive equality, which
+    // means we assume two values are equivalent until proven otherwise and do
+    // not recursively compare pairs of values we have already seen.
+    std::unordered_set<std::pair<GCData*, GCData*>> compared;
+    return areEqualImpl(a, b, compared);
+  }
+
+  bool areEqualImpl(Literal a,
+                    Literal b,
+                    std::unordered_set<std::pair<GCData*, GCData*>>& compared) {
     // Only compare some references. In general the optimizer may change
     // identities and structures of functions, types, and GC values in ways that
     // are not externally observable. We must therefore limit ourselves to
-    // comparing information that _is_ externally observable.
-    //
-    // TODO: We could compare more information when we know it will be
-    // externally visible, for example when the type of the value is public.
+    // comparing information that _is_ externally observable. This includes
+    // fields that are part of public types as well as configured JS prototypes.
     if (!a.type.isRef() || !b.type.isRef()) {
       return a == b;
     }
@@ -581,22 +601,85 @@ struct ExecutionResults {
     // However, we have no way of comparing pointer identities across
     // executions, so just recursively look for externally observable
     // differences in the prototypes.
-    if (!areEqual(a.getJSPrototype(), b.getJSPrototype())) {
+    if (!areEqualImpl(a.getJSPrototype(), b.getJSPrototype(), compared)) {
       return false;
     }
 
+    // Check for public struct or array content.
+    auto publicType = getClosestPublicAncestor(htA);
+    if (publicType != getClosestPublicAncestor(htB)) {
+      // Since public types are externally observable, having different public
+      // types is an observable difference.
+      return false;
+    }
+    if (publicType && publicType->isData()) {
+      auto* dataA = a.getGCData().get();
+      auto* dataB = b.getGCData().get();
+      if (dataA == dataB) {
+        return true;
+      }
+      if (!compared.insert({dataA, dataB}).second) {
+        // We are already comparing these values. Assume they are equivalent
+        // (until possibly proven otherwise later) and do not recurse further.
+        return true;
+      }
+      compared.insert({dataA, dataB});
+
+      if (publicType->isStruct()) {
+        auto& fields = publicType->getStruct().fields;
+        auto& valuesA = dataA->values;
+        auto& valuesB = dataB->values;
+        for (Index i = 0; i < fields.size(); i++) {
+          if (!areEqualImpl(valuesA[i], valuesB[i], compared)) {
+            return false;
+          }
+        }
+      } else if (publicType->isArray()) {
+        auto& valuesA = dataA->values;
+        auto& valuesB = dataB->values;
+        if (valuesA.size() != valuesB.size()) {
+          return false;
+        }
+        for (Index i = 0; i < valuesA.size(); i++) {
+          if (!areEqualImpl(valuesA[i], valuesB[i], compared)) {
+            return false;
+          }
+        }
+      }
+    }
+
     // Other differences are not observable, so conservatively consider the
     // values equal.
     return true;
   }
 
+  std::optional<HeapType> getClosestPublicAncestor(HeapType ht) {
+    while (true) {
+      if (publicTypes.count(ht)) {
+        return ht;
+      }
+      if (auto super = ht.getDeclaredSuperType()) {
+        ht = *super;
+      } else {
+        return std::nullopt;
+      }
+    }
+  }
+
   bool areEqual(Literals a, Literals b) {
+    std::unordered_set<std::pair<GCData*, GCData*>> compared;
+    return areEqualImpl(a, b, compared);
+  }
+
+  bool areEqualImpl(Literals a,
+                    Literals b,
+                    std::unordered_set<std::pair<GCData*, GCData*>>& compared) {
     if (a.size() != b.size()) {
       std::cout << "literal counts not identical! " << a << " != " << b << '\n';
       return false;
     }
     for (Index i = 0; i < a.size(); i++) {
-      if (!areEqual(a[i], b[i])) {
+      if (!areEqualImpl(a[i], b[i], compared)) {
         std::cout << "values not identical! " << a[i] << " != " << b[i] << '\n';
         return false;
       }

test/gtest/CMakeLists.txt

@@ -12,6 +12,7 @@ set(unittest_SOURCES
   dataflow.cpp
   dfa_minimization.cpp
   disjoint_sets.cpp
+  execution-results.cpp
   glbs.cpp
   interpreter.cpp
   intervals.cpp

test/gtest/execution-results.cpp

@@ -0,0 +1,154 @@
+
+#include "tools/execution-results.h"
+#include "test/gtest/type-test.h"
+#include "wasm-type.h"
+#include "wasm.h"
+#include "gtest/gtest.h"
+
+using namespace wasm;
+
+struct ExecutionResultsTest : public TypeTest {
+  Module wasm;
+};
+
+TEST_F(ExecutionResultsTest, PublicVsPrivate) {
+  TypeBuilder builder(2);
+  // Public struct
+  builder[0] = Struct({Field(Type::i32, Mutable)});
+  // Private struct
+  builder[1] = Struct({Field(Type::f64, Mutable)});
+  auto result = builder.build();
+  ASSERT_TRUE(result);
+  auto& types = *result;
+  HeapType publicHT = types[0];
+  HeapType privateHT = types[1];
+
+  ExecutionResults results;
+  results.publicTypes.insert(publicHT);
+
+  auto data42 = std::make_shared<GCData>(Literals{Literal(int32_t(42))});
+  auto data43 = std::make_shared<GCData>(Literals{Literal(int32_t(43))});
+
+  auto dataF42 = std::make_shared<GCData>(Literals{Literal(double(42.0))});
+  auto dataF43 = std::make_shared<GCData>(Literals{Literal(double(43.0))});
+
+  Literal public42(data42, publicHT);
+  // Same data pointer
+  Literal public42_2(data42, publicHT);
+  // Same content, different pointer
+  Literal public42_3(std::make_shared<GCData>(Literals{Literal(int32_t(42))}),
+                     publicHT);
+  Literal public43(data43, publicHT);
+
+  Literal private42(dataF42, privateHT);
+  Literal private43(dataF43, privateHT);
+
+  // Public types are compared by content
+  EXPECT_TRUE(results.areEqual(public42, public42_2));
+  EXPECT_TRUE(results.areEqual(public42, public42_3));
+  EXPECT_FALSE(results.areEqual(public42, public43));
+
+  // Private types are always equal
+  EXPECT_TRUE(results.areEqual(private42, private43));
+
+  // Public and private are different
+  EXPECT_FALSE(results.areEqual(public42, private42));
+}
+
+TEST_F(ExecutionResultsTest, RecursivePublic) {
+  TypeBuilder builder(1);
+  builder[0] =
+    Struct({Field(builder.getTempRefType(builder[0], Nullable), Mutable)});
+  auto result = builder.build();
+  ASSERT_TRUE(result);
+  auto& types = *result;
+  HeapType ht = types[0];
+
+  ExecutionResults results;
+  results.publicTypes.insert(ht);
+
+  // Create recursive data: A -> A
+  auto dataA = std::make_shared<GCData>(Literals{Literal::makeNull(ht)});
+  Literal litA(dataA, ht);
+  dataA->values[0] = litA;
+
+  // Create another identical recursive data: B -> B
+  auto dataB = std::make_shared<GCData>(Literals{Literal::makeNull(ht)});
+  Literal litB(dataB, ht);
+  dataB->values[0] = litB;
+
+  EXPECT_TRUE(results.areEqual(litA, litB));
+
+  // Create a different recursive data: C -> D -> C
+  auto dataC = std::make_shared<GCData>(Literals{Literal::makeNull(ht)});
+  auto dataD = std::make_shared<GCData>(Literals{Literal::makeNull(ht)});
+  Literal litC(dataC, ht);
+  Literal litD(dataD, ht);
+  dataC->values[0] = litD;
+  dataD->values[0] = litC;
+
+  // They are actually all equivalent as infinite structures of just this one
+  // type.
+  EXPECT_TRUE(results.areEqual(litA, litC));
+}
+
+TEST_F(ExecutionResultsTest, SubtypeOfPublic) {
+  TypeBuilder builder(2);
+  // Public supertype
+  builder[0] = Struct({Field(Type::i32, Mutable)});
+  builder[0].setOpen();
+  // Subtype with extra field
+  builder[1] = Struct({Field(Type::i32, Mutable), Field(Type::f64, Mutable)});
+  builder[1].subTypeOf(builder[0]);
+  auto result = builder.build();
+  ASSERT_TRUE(result);
+  auto& types = *result;
+  HeapType superHT = types[0];
+  HeapType subHT = types[1];
+
+  ExecutionResults results;
+  results.publicTypes.insert(superHT);
+
+  // sub42_0 and sub42_1 have the same public field but different private
+  // fields.
+  auto data42_0 = std::make_shared<GCData>(
+    Literals{Literal(int32_t(42)), Literal(double(0.0))});
+  auto data42_1 = std::make_shared<GCData>(
+    Literals{Literal(int32_t(42)), Literal(double(1.0))});
+  auto data43_0 = std::make_shared<GCData>(
+    Literals{Literal(int32_t(43)), Literal(double(0.0))});
+
+  Literal sub42_0(data42_0, subHT);
+  Literal sub42_1(data42_1, subHT);
+  Literal sub43_0(data43_0, subHT);
+
+  auto data42_super = std::make_shared<GCData>(Literals{Literal(int32_t(42))});
+  Literal super42(data42_super, superHT);
+
+  // Subtype of public type should be compared by content (of the public
+  // ancestor) Extra fields in the subtype should be ignored.
+  EXPECT_TRUE(results.areEqual(sub42_0, sub42_1));
+  EXPECT_TRUE(results.areEqual(sub42_0, super42));
+  EXPECT_FALSE(results.areEqual(sub42_0, sub43_0));
+}
+
+TEST_F(ExecutionResultsTest, PrivateTypes) {
+  TypeBuilder builder(1);
+  builder[0] = Struct({Field(Type::i32, Mutable)});
+  auto result = builder.build();
+  ASSERT_TRUE(result);
+  auto& types = *result;
+  HeapType ht = types[0];
+
+  // publicTypes is empty
+  ExecutionResults results;
+
+  auto data42 = std::make_shared<GCData>(Literals{Literal(int32_t(42))});
+  auto data43 = std::make_shared<GCData>(Literals{Literal(int32_t(43))});
+
+  Literal lit42(data42, ht);
+  Literal lit43(data43, ht);
+
+  // This should not crash and should return true.
+  EXPECT_TRUE(results.areEqual(lit42, lit43));
+}

test/lit/exec/fuzzing-api.wast

@@ -29,6 +29,11 @@
 
  (type $i32 (struct i32))
 
+ (rec
+   (type $rec-A (struct (field (mut (ref null $rec-B)))))
+   (type $rec-B (struct (field (mut (ref null $rec-A)))))
+ )
+
  (table $table 10 20 funcref)
 
  ;; Note that the exported table appears first here, but in the binary and in
@@ -482,7 +487,6 @@
 
  ;; CHECK:      [fuzz-exec] export return-externref-exception
  ;; CHECK-NEXT: [fuzz-exec] note result: return-externref-exception => jserror
- ;; CHECK-NEXT: warning: no passes specified, not doing any work
  (func $return-externref-exception (export "return-externref-exception") (result externref)
   ;; Call JS table.set in a way that throws (on out of bounds). The JS exception
   ;; is caught and returned from the function, so we can see what it looks like
@@ -497,6 +501,21 @@
    (unreachable)
   )
  )
+
+ ;; CHECK:      [fuzz-exec] export recursive-public-data
+ ;; CHECK-NEXT: [fuzz-exec] note result: recursive-public-data => object(null)
+ ;; CHECK-NEXT: warning: no passes specified, not doing any work
+ (func $recursive-public-data (export "recursive-public-data") (result (ref null $rec-A))
+   ;; We should not infinitely recurse when comparing whether this recursive
+   ;; structure is equivalent in the two executions.
+   (local $a (ref null $rec-A))
+   (local $b (ref null $rec-B))
+   (local.set $a (struct.new_default $rec-A))
+   (local.set $b (struct.new $rec-B (local.get $a)))
+   (struct.set $rec-A 0 (local.get $a) (local.get $b))
+   (local.get $a)
+ )
+
 )
 ;; CHECK:      [fuzz-exec] export logging
 ;; CHECK-NEXT: [LoggingExternalInterface logging 42]
@@ -613,12 +632,16 @@
 
 ;; CHECK:      [fuzz-exec] export return-externref-exception
 ;; CHECK-NEXT: [fuzz-exec] note result: return-externref-exception => jserror
+
+;; CHECK:      [fuzz-exec] export recursive-public-data
+;; CHECK-NEXT: [fuzz-exec] note result: recursive-public-data => object(null)
 ;; CHECK-NEXT: [fuzz-exec] comparing catch-js-tag
 ;; CHECK-NEXT: [fuzz-exec] comparing do-sleep
 ;; CHECK-NEXT: [fuzz-exec] comparing export.calling
 ;; CHECK-NEXT: [fuzz-exec] comparing export.calling.catching
 ;; CHECK-NEXT: [fuzz-exec] comparing export.calling.rethrow
 ;; CHECK-NEXT: [fuzz-exec] comparing logging
+;; CHECK-NEXT: [fuzz-exec] comparing recursive-public-data
 ;; CHECK-NEXT: [fuzz-exec] comparing ref.calling
 ;; CHECK-NEXT: [fuzz-exec] comparing ref.calling.catching
 ;; CHECK-NEXT: [fuzz-exec] comparing ref.calling.illegal