From c2553a315f5c78d73a808526782596d4e3870082 Mon Sep 17 00:00:00 2001
From: Alastair Donaldson <afdx@google.com>
Date: Fri, 9 Oct 2020 07:18:46 +0100
Subject: [PATCH] spirv-fuzz: Do not expose synonym facts for non-existent ids
 (#3891)

Fixes #3888.
---
 .../data_synonym_and_id_equation_facts.cpp    | 19 +++++++++++---
 ...ata_synonym_and_id_equation_facts_test.cpp | 25 +++++++++++++++++++
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/source/fuzz/fact_manager/data_synonym_and_id_equation_facts.cpp b/source/fuzz/fact_manager/data_synonym_and_id_equation_facts.cpp
index f8b971208..44a0851c9 100644
--- a/source/fuzz/fact_manager/data_synonym_and_id_equation_facts.cpp
+++ b/source/fuzz/fact_manager/data_synonym_and_id_equation_facts.cpp
@@ -867,17 +867,30 @@ DataSynonymAndIdEquationFacts::GetSynonymsForId(uint32_t id) const {
 std::vector<const protobufs::DataDescriptor*>
 DataSynonymAndIdEquationFacts::GetSynonymsForDataDescriptor(
     const protobufs::DataDescriptor& data_descriptor) const {
+  std::vector<const protobufs::DataDescriptor*> result;
   if (synonymous_.Exists(data_descriptor)) {
-    return synonymous_.GetEquivalenceClass(data_descriptor);
+    for (auto dd : synonymous_.GetEquivalenceClass(data_descriptor)) {
+      // There may be data descriptors in the equivalence class whose base
+      // objects have been removed from the module.  We do not expose these
+      // data descriptors to clients of the fact manager.
+      if (ir_context_->get_def_use_mgr()->GetDef(dd->object()) != nullptr) {
+        result.push_back(dd);
+      }
+    }
   }
-  return {};
+  return result;
 }
 
 std::vector<uint32_t>
 DataSynonymAndIdEquationFacts::GetIdsForWhichSynonymsAreKnown() const {
   std::vector<uint32_t> result;
   for (auto& data_descriptor : synonymous_.GetAllKnownValues()) {
-    if (data_descriptor->index().empty()) {
+    // We skip any data descriptors whose base objects no longer exist in the
+    // module, and we restrict attention to data descriptors for plain ids,
+    // which have no indices.
+    if (ir_context_->get_def_use_mgr()->GetDef(data_descriptor->object()) !=
+            nullptr &&
+        data_descriptor->index().empty()) {
       result.push_back(data_descriptor->object());
     }
   }
diff --git a/test/fuzz/fact_manager/data_synonym_and_id_equation_facts_test.cpp b/test/fuzz/fact_manager/data_synonym_and_id_equation_facts_test.cpp
index 069268b4f..c9a7005d7 100644
--- a/test/fuzz/fact_manager/data_synonym_and_id_equation_facts_test.cpp
+++ b/test/fuzz/fact_manager/data_synonym_and_id_equation_facts_test.cpp
@@ -20,6 +20,23 @@ namespace spvtools {
 namespace fuzz {
 namespace {
 
+void CheckConsistencyOfSynonymFacts(
+    opt::IRContext* ir_context,
+    const TransformationContext& transformation_context) {
+  for (uint32_t id : transformation_context.GetFactManager()
+                         ->GetIdsForWhichSynonymsAreKnown()) {
+    // Every id reported by the fact manager should exist in the module.
+    ASSERT_NE(ir_context->get_def_use_mgr()->GetDef(id), nullptr);
+    auto synonyms =
+        transformation_context.GetFactManager()->GetSynonymsForId(id);
+    for (auto& dd : synonyms) {
+      // Every reported synonym should have a base object that exists in the
+      // module.
+      ASSERT_NE(ir_context->get_def_use_mgr()->GetDef(dd->object()), nullptr);
+    }
+  }
+}
+
 TEST(DataSynonymAndIdEquationFactsTest, RecursiveAdditionOfFacts) {
   std::string shader = R"(
                OpCapability Shader
@@ -192,6 +209,8 @@ TEST(DataSynonymAndIdEquationFactsTest, HandlesCorollariesWithInvalidIds) {
   transformation_context.GetFactManager()->AddFactDataSynonym(
       MakeDataDescriptor(14, {}), MakeDataDescriptor(17, {}));
 
+  CheckConsistencyOfSynonymFacts(context.get(), transformation_context);
+
   // Apply TransformationMergeBlocks which will remove %17 from the module.
   TransformationMergeBlocks transformation(16);
   ASSERT_TRUE(
@@ -199,6 +218,8 @@ TEST(DataSynonymAndIdEquationFactsTest, HandlesCorollariesWithInvalidIds) {
   transformation.Apply(context.get(), &transformation_context);
   ASSERT_TRUE(IsValid(env, context.get()));
 
+  CheckConsistencyOfSynonymFacts(context.get(), transformation_context);
+
   ASSERT_EQ(context->get_def_use_mgr()->GetDef(17), nullptr);
 
   // Add another equation.
@@ -212,6 +233,8 @@ TEST(DataSynonymAndIdEquationFactsTest, HandlesCorollariesWithInvalidIds) {
   ASSERT_TRUE(transformation_context.GetFactManager()->IsSynonymous(
       MakeDataDescriptor(15, {}), MakeDataDescriptor(14, {})));
 
+  CheckConsistencyOfSynonymFacts(context.get(), transformation_context);
+
   // Remove some instructions from the module. At this point, the equivalence
   // class of %14 has no valid members.
   ASSERT_TRUE(context->KillDef(14));
@@ -220,6 +243,8 @@ TEST(DataSynonymAndIdEquationFactsTest, HandlesCorollariesWithInvalidIds) {
   transformation_context.GetFactManager()->AddFactIdEquation(
       18, SpvOpConvertSToF, {9});
 
+  CheckConsistencyOfSynonymFacts(context.get(), transformation_context);
+
   // We don't create synonyms if at least one of the equivalence classes has no
   // valid members.
   ASSERT_FALSE(transformation_context.GetFactManager()->IsSynonymous(