spirv-fuzz: Pass submanagers to other submanagers when necessary (#3796)

This PR changes the fact manager so that, when calling some of the functions in submanagers, passes references to other submanagers if necessary (e.g. to make consistency checks). In particular: - DataSynonymAndIdEquationFacts is passed to the AddFactIdIsIrrelevant function of IrrelevantValueFacts - IrrelevantValueFacts is passed to the AddFact functions of DataSynonymAndIdEquationFacts The IRContext is also passed when necessary and the calls to the corresponding functions in FactManager were updated to be valid and always use an updated context. Fixes #3550.
KhronosGroup · Sep 15, 2020 · 1e1c308 · 1e1c308
1 parent f62357e
commit 1e1c308
Show file tree

Hide file tree

Showing 35 changed files with 238 additions and 152 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
@@ -52,19 +52,26 @@ bool DataSynonymAndIdEquationFacts::OperationEquals::operator()(
 }
 
 void DataSynonymAndIdEquationFacts::AddFact(
-    const protobufs::FactDataSynonym& fact, opt::IRContext* context) {
-  // TODO(https://github.com/KhronosGroup/SPIRV-Tools/issues/3550)
-  //  Assert that ids are not irrelevant.
+    const protobufs::FactDataSynonym& fact,
+    const IrrelevantValueFacts& irrelevant_value_facts,
+    opt::IRContext* context) {
+  (void)irrelevant_value_facts;  // Keep release compilers happy.
+  assert(!irrelevant_value_facts.IdIsIrrelevant(fact.data1().object()) &&
+         !irrelevant_value_facts.IdIsIrrelevant(fact.data2().object()) &&
+         "Irrelevant ids cannot be synonymous with other ids.");
 
   // Add the fact, including all facts relating sub-components of the data
   // descriptors that are involved.
   AddDataSynonymFactRecursive(fact.data1(), fact.data2(), context);
 }
 
 void DataSynonymAndIdEquationFacts::AddFact(
-    const protobufs::FactIdEquation& fact, opt::IRContext* context) {
-  // TODO(https://github.com/KhronosGroup/SPIRV-Tools/issues/3550)
-  //  Assert that ids are not irrelevant.
+    const protobufs::FactIdEquation& fact,
+    const IrrelevantValueFacts& irrelevant_value_facts,
+    opt::IRContext* context) {
+  (void)irrelevant_value_facts;  // Keep release compilers happy.
+  assert(!irrelevant_value_facts.IdIsIrrelevant(fact.lhs_id()) &&
+         "Irrelevant ids are not allowed.");
 
   protobufs::DataDescriptor lhs_dd = MakeDataDescriptor(fact.lhs_id(), {});
 
@@ -75,8 +82,8 @@ void DataSynonymAndIdEquationFacts::AddFact(
   // equation.
   std::vector<const protobufs::DataDescriptor*> rhs_dds;
   for (auto rhs_id : fact.rhs_id()) {
-    // TODO(https://github.com/KhronosGroup/SPIRV-Tools/issues/3550)
-    //  Assert that ids are not irrelevant.
+    assert(!irrelevant_value_facts.IdIsIrrelevant(rhs_id) &&
+           "Irrelevant ids are not allowed.");
 
     // Register a data descriptor based on this id in the equivalence relation
     // if needed, and then record the equivalence class representative.
@@ -828,6 +835,11 @@ bool DataSynonymAndIdEquationFacts::DataDescriptorsAreWellFormedAndComparable(
           get_bit_count_for_numeric_type(*type_b));
 }
 
+std::vector<const protobufs::DataDescriptor*>
+DataSynonymAndIdEquationFacts::GetSynonymsForId(uint32_t id) const {
+  return GetSynonymsForDataDescriptor(MakeDataDescriptor(id, {}));
+}
+
 std::vector<const protobufs::DataDescriptor*>
 DataSynonymAndIdEquationFacts::GetSynonymsForDataDescriptor(
     const protobufs::DataDescriptor& data_descriptor) const {

diff --git a/source/fuzz/fact_manager/data_synonym_and_id_equation_facts.h b/source/fuzz/fact_manager/data_synonym_and_id_equation_facts.h
@@ -27,15 +27,28 @@ namespace spvtools {
 namespace fuzz {
 namespace fact_manager {
 
+// Forward reference to the IrrelevantValueFacts class.
+class IrrelevantValueFacts;
+
 // The purpose of this class is to group the fields and data used to represent
 // facts about data synonyms and id equations.
 class DataSynonymAndIdEquationFacts {
  public:
   // See method in FactManager which delegates to this method.
-  void AddFact(const protobufs::FactDataSynonym& fact, opt::IRContext* context);
+  // |irrelevant_value_facts| is passed for consistency checks.
+  void AddFact(const protobufs::FactDataSynonym& fact,
+               const IrrelevantValueFacts& irrelevant_value_facts,
+               opt::IRContext* context);
+
+  // See method in FactManager which delegates to this method.
+  // |irrelevant_value_facts| is passed for consistency checks.
+  void AddFact(const protobufs::FactIdEquation& fact,
+               const IrrelevantValueFacts& irrelevant_value_facts,
+               opt::IRContext* context);
 
   // See method in FactManager which delegates to this method.
-  void AddFact(const protobufs::FactIdEquation& fact, opt::IRContext* context);
+  std::vector<const protobufs::DataDescriptor*> GetSynonymsForId(
+      uint32_t id) const;
 
   // See method in FactManager which delegates to this method.
   std::vector<const protobufs::DataDescriptor*> GetSynonymsForDataDescriptor(

diff --git a/source/fuzz/fact_manager/fact_manager.cpp b/source/fuzz/fact_manager/fact_manager.cpp
@@ -105,8 +105,8 @@ bool FactManager::AddFact(const fuzz::protobufs::Fact& fact,
       return constant_uniform_facts_.AddFact(fact.constant_uniform_fact(),
                                              context);
     case protobufs::Fact::kDataSynonymFact:
-      data_synonym_and_id_equation_facts_.AddFact(fact.data_synonym_fact(),
-                                                  context);
+      data_synonym_and_id_equation_facts_.AddFact(
+          fact.data_synonym_fact(), irrelevant_value_facts_, context);
       return true;
     case protobufs::Fact::kBlockIsDeadFact:
       dead_block_facts_.AddFact(fact.block_is_dead_fact());
@@ -126,7 +126,8 @@ void FactManager::AddFactDataSynonym(const protobufs::DataDescriptor& data1,
   protobufs::FactDataSynonym fact;
   *fact.mutable_data1() = data1;
   *fact.mutable_data2() = data2;
-  data_synonym_and_id_equation_facts_.AddFact(fact, context);
+  data_synonym_and_id_equation_facts_.AddFact(fact, irrelevant_value_facts_,
+                                              context);
 }
 
 std::vector<uint32_t> FactManager::GetConstantsAvailableFromUniformsForType(
@@ -172,7 +173,7 @@ FactManager::GetSynonymsForDataDescriptor(
 
 std::vector<const protobufs::DataDescriptor*> FactManager::GetSynonymsForId(
     uint32_t id) const {
-  return GetSynonymsForDataDescriptor(MakeDataDescriptor(id, {}));
+  return data_synonym_and_id_equation_facts_.GetSynonymsForId(id);
 }
 
 bool FactManager::IsSynonymous(
@@ -214,16 +215,20 @@ const std::unordered_set<uint32_t>& FactManager::GetIrrelevantIds() const {
   return irrelevant_value_facts_.GetIrrelevantIds();
 }
 
-void FactManager::AddFactValueOfPointeeIsIrrelevant(uint32_t pointer_id) {
+void FactManager::AddFactValueOfPointeeIsIrrelevant(uint32_t pointer_id,
+                                                    opt::IRContext* context) {
   protobufs::FactPointeeValueIsIrrelevant fact;
   fact.set_pointer_id(pointer_id);
-  irrelevant_value_facts_.AddFact(fact);
+  irrelevant_value_facts_.AddFact(fact, data_synonym_and_id_equation_facts_,
+                                  context);
 }
 
-void FactManager::AddFactIdIsIrrelevant(uint32_t result_id) {
+void FactManager::AddFactIdIsIrrelevant(uint32_t result_id,
+                                        opt::IRContext* context) {
   protobufs::FactIdIsIrrelevant fact;
   fact.set_result_id(result_id);
-  irrelevant_value_facts_.AddFact(fact);
+  irrelevant_value_facts_.AddFact(fact, data_synonym_and_id_equation_facts_,
+                                  context);
 }
 
 void FactManager::AddFactIdEquation(uint32_t lhs_id, SpvOp opcode,
@@ -235,7 +240,8 @@ void FactManager::AddFactIdEquation(uint32_t lhs_id, SpvOp opcode,
   for (auto an_rhs_id : rhs_id) {
     fact.add_rhs_id(an_rhs_id);
   }
-  data_synonym_and_id_equation_facts_.AddFact(fact, context);
+  data_synonym_and_id_equation_facts_.AddFact(fact, irrelevant_value_facts_,
+                                              context);
 }
 
 void FactManager::ComputeClosureOfFacts(

diff --git a/source/fuzz/fact_manager/fact_manager.h b/source/fuzz/fact_manager/fact_manager.h
@@ -66,11 +66,14 @@ class FactManager {
 
   // Records the fact that the value of the pointee associated with |pointer_id|
   // is irrelevant: it does not affect the observable behaviour of the module.
-  void AddFactValueOfPointeeIsIrrelevant(uint32_t pointer_id);
+  // |pointer_id| must exist in the module and actually be a pointer.
+  void AddFactValueOfPointeeIsIrrelevant(uint32_t pointer_id,
+                                         opt::IRContext* context);
 
   // Records a fact that the |result_id| is irrelevant (i.e. it doesn't affect
-  // the semantics of the module)
-  void AddFactIdIsIrrelevant(uint32_t result_id);
+  // the semantics of the module).
+  // |result_id| must exist in the module and actually be a pointer.
+  void AddFactIdIsIrrelevant(uint32_t result_id, opt::IRContext* context);
 
   // Records the fact that |lhs_id| is defined by the equation:
   //

diff --git a/source/fuzz/fact_manager/irrelevant_value_facts.cpp b/source/fuzz/fact_manager/irrelevant_value_facts.cpp
@@ -15,25 +15,43 @@
 #include "source/fuzz/fact_manager/irrelevant_value_facts.h"
 
 #include "source/fuzz/data_descriptor.h"
-#include "source/fuzz/fuzzer_util.h"
+#include "source/fuzz/fact_manager/data_synonym_and_id_equation_facts.h"
+#include "source/opt/ir_context.h"
 
 namespace spvtools {
 namespace fuzz {
 namespace fact_manager {
 
 void IrrelevantValueFacts::AddFact(
-    const protobufs::FactPointeeValueIsIrrelevant& fact) {
-  // TODO(https://github.com/KhronosGroup/SPIRV-Tools/issues/3550)
-  //  Assert that the id does not participate in DataSynonym facts and is a
-  //  pointer.
+    const protobufs::FactPointeeValueIsIrrelevant& fact,
+    const DataSynonymAndIdEquationFacts& data_synonym_and_id_equation_facts,
+    opt::IRContext* context) {
+  (void)data_synonym_and_id_equation_facts;  // Keep release compilers happy.
+  assert(data_synonym_and_id_equation_facts.GetSynonymsForId(fact.pointer_id())
+             .empty() &&
+         "The id cannot participate in DataSynonym facts.");
+  auto pointer_def = context->get_def_use_mgr()->GetDef(fact.pointer_id());
+  assert(pointer_def && "The id must exist in the module.");
+  auto type = context->get_type_mgr()->GetType(pointer_def->type_id());
+  (void)type;  // Keep release compilers happy.
+  assert(type && type->AsPointer() && "The id must be a pointer.");
 
   pointers_to_irrelevant_pointees_ids_.insert(fact.pointer_id());
 }
 
-void IrrelevantValueFacts::AddFact(const protobufs::FactIdIsIrrelevant& fact) {
-  // TODO(https://github.com/KhronosGroup/SPIRV-Tools/issues/3550)
-  //  Assert that the id does not participate in DataSynonym facts and is not a
-  //  pointer.
+void IrrelevantValueFacts::AddFact(
+    const protobufs::FactIdIsIrrelevant& fact,
+    const DataSynonymAndIdEquationFacts& data_synonym_and_id_equation_facts,
+    opt::IRContext* context) {
+  (void)data_synonym_and_id_equation_facts;  // Keep release compilers happy.
+  assert(data_synonym_and_id_equation_facts.GetSynonymsForId(fact.result_id())
+             .empty() &&
+         "The id cannot participate in DataSynonym facts.");
+  auto pointer_def = context->get_def_use_mgr()->GetDef(fact.result_id());
+  assert(pointer_def && "The id must exist in the module.");
+  auto type = context->get_type_mgr()->GetType(pointer_def->type_id());
+  (void)type;  // Keep release compilers happy.
+  assert(type && !type->AsPointer() && "The id must not be a pointer.");
 
   irrelevant_ids_.insert(fact.result_id());
 }

diff --git a/source/fuzz/fact_manager/irrelevant_value_facts.h b/source/fuzz/fact_manager/irrelevant_value_facts.h
@@ -24,15 +24,28 @@ namespace spvtools {
 namespace fuzz {
 namespace fact_manager {
 
+// Forward reference to the DataSynonymAndIdEquationFacts class.
+class DataSynonymAndIdEquationFacts;
+
 // The purpose of this class is to group the fields and data used to represent
 // facts about various irrelevant values in the module.
 class IrrelevantValueFacts {
  public:
   // See method in FactManager which delegates to this method.
-  void AddFact(const protobufs::FactPointeeValueIsIrrelevant& fact);
+  // |data_synonym_and_id_equation_facts| and |context| are passed for
+  // consistency checks.
+  void AddFact(
+      const protobufs::FactPointeeValueIsIrrelevant& fact,
+      const DataSynonymAndIdEquationFacts& data_synonym_and_id_equation_facts,
+      opt::IRContext* context);
 
   // See method in FactManager which delegates to this method.
-  void AddFact(const protobufs::FactIdIsIrrelevant& fact);
+  // |data_synonym_and_id_equation_facts| and |context| are passed for
+  // consistency checks.
+  void AddFact(
+      const protobufs::FactIdIsIrrelevant& fact,
+      const DataSynonymAndIdEquationFacts& data_synonym_and_id_equation_facts,
+      opt::IRContext* context);
 
   // See method in FactManager which delegates to this method.
   bool PointeeValueIsIrrelevant(uint32_t pointer_id) const;

diff --git a/source/fuzz/transformation_access_chain.cpp b/source/fuzz/transformation_access_chain.cpp
@@ -339,7 +339,7 @@ void TransformationAccessChain::Apply(
   if (transformation_context->GetFactManager()->PointeeValueIsIrrelevant(
           message_.pointer_id())) {
     transformation_context->GetFactManager()->AddFactValueOfPointeeIsIrrelevant(
-        message_.fresh_id());
+        message_.fresh_id(), ir_context);
   }
 }
 

diff --git a/source/fuzz/transformation_add_constant_boolean.cpp b/source/fuzz/transformation_add_constant_boolean.cpp
@@ -53,7 +53,7 @@ void TransformationAddConstantBoolean::Apply(
 
   if (message_.is_irrelevant()) {
     transformation_context->GetFactManager()->AddFactIdIsIrrelevant(
-        message_.fresh_id());
+        message_.fresh_id(), ir_context);
   }
 }
 

diff --git a/source/fuzz/transformation_add_constant_composite.cpp b/source/fuzz/transformation_add_constant_composite.cpp
@@ -122,7 +122,7 @@ void TransformationAddConstantComposite::Apply(
 
   if (message_.is_irrelevant()) {
     transformation_context->GetFactManager()->AddFactIdIsIrrelevant(
-        message_.fresh_id());
+        message_.fresh_id(), ir_context);
   }
 }
 

diff --git a/source/fuzz/transformation_add_constant_scalar.cpp b/source/fuzz/transformation_add_constant_scalar.cpp
@@ -80,7 +80,7 @@ void TransformationAddConstantScalar::Apply(
 
   if (message_.is_irrelevant()) {
     transformation_context->GetFactManager()->AddFactIdIsIrrelevant(
-        message_.fresh_id());
+        message_.fresh_id(), ir_context);
   }
 }
 

diff --git a/source/fuzz/transformation_add_copy_memory.cpp b/source/fuzz/transformation_add_copy_memory.cpp
@@ -132,18 +132,18 @@ void TransformationAddCopyMemory::Apply(
 
   fuzzerutil::UpdateModuleIdBound(ir_context, message_.fresh_id());
 
+  // Make sure our changes are analyzed
+  ir_context->InvalidateAnalysesExceptFor(
+      opt::IRContext::Analysis::kAnalysisNone);
+
   // Even though the copy memory instruction will - at least temporarily - lead
   // to the destination and source pointers referring to identical values, this
   // fact is not guaranteed to hold throughout execution of the SPIR-V code
   // since the source pointer could be over-written. We thus assume nothing
   // about the destination pointer, and record this fact so that the destination
   // pointer can be used freely by other fuzzer passes.
   transformation_context->GetFactManager()->AddFactValueOfPointeeIsIrrelevant(
-      message_.fresh_id());
-
-  // Make sure our changes are analyzed
-  ir_context->InvalidateAnalysesExceptFor(
-      opt::IRContext::Analysis::kAnalysisNone);
+      message_.fresh_id(), ir_context);
 }
 
 protobufs::Transformation TransformationAddCopyMemory::ToMessage() const {

diff --git a/source/fuzz/transformation_add_function.cpp b/source/fuzz/transformation_add_function.cpp
@@ -168,30 +168,6 @@ void TransformationAddFunction::Apply(
   (void)(success);  // Keep release builds happy (otherwise they may complain
                     // that |success| is not used).
 
-  // Record the fact that all pointer parameters and variables declared in the
-  // function should be regarded as having irrelevant values.  This allows other
-  // passes to store arbitrarily to such variables, and to pass them freely as
-  // parameters to other functions knowing that it is OK if they get
-  // over-written.
-  for (auto& instruction : message_.instruction()) {
-    switch (instruction.opcode()) {
-      case SpvOpFunctionParameter:
-        if (ir_context->get_def_use_mgr()
-                ->GetDef(instruction.result_type_id())
-                ->opcode() == SpvOpTypePointer) {
-          transformation_context->GetFactManager()
-              ->AddFactValueOfPointeeIsIrrelevant(instruction.result_id());
-        }
-        break;
-      case SpvOpVariable:
-        transformation_context->GetFactManager()
-            ->AddFactValueOfPointeeIsIrrelevant(instruction.result_id());
-        break;
-      default:
-        break;
-    }
-  }
-
   if (message_.is_livesafe()) {
     // Make the function livesafe, which also should succeed.
     success = TryToMakeFunctionLivesafe(ir_context, *transformation_context);
@@ -214,6 +190,32 @@ void TransformationAddFunction::Apply(
     }
   }
   ir_context->InvalidateAnalysesExceptFor(opt::IRContext::kAnalysisNone);
+
+  // Record the fact that all pointer parameters and variables declared in the
+  // function should be regarded as having irrelevant values.  This allows other
+  // passes to store arbitrarily to such variables, and to pass them freely as
+  // parameters to other functions knowing that it is OK if they get
+  // over-written.
+  for (auto& instruction : message_.instruction()) {
+    switch (instruction.opcode()) {
+      case SpvOpFunctionParameter:
+        if (ir_context->get_def_use_mgr()
+                ->GetDef(instruction.result_type_id())
+                ->opcode() == SpvOpTypePointer) {
+          transformation_context->GetFactManager()
+              ->AddFactValueOfPointeeIsIrrelevant(instruction.result_id(),
+                                                  ir_context);
+        }
+        break;
+      case SpvOpVariable:
+        transformation_context->GetFactManager()
+            ->AddFactValueOfPointeeIsIrrelevant(instruction.result_id(),
+                                                ir_context);
+        break;
+      default:
+        break;
+    }
+  }
 }
 
 protobufs::Transformation TransformationAddFunction::ToMessage() const {

diff --git a/source/fuzz/transformation_add_global_variable.cpp b/source/fuzz/transformation_add_global_variable.cpp
@@ -98,15 +98,15 @@ void TransformationAddGlobalVariable::Apply(
       static_cast<SpvStorageClass>(message_.storage_class()),
       message_.initializer_id());
 
-  if (message_.value_is_irrelevant()) {
-    transformation_context->GetFactManager()->AddFactValueOfPointeeIsIrrelevant(
-        message_.fresh_id());
-  }
-
   // We have added an instruction to the module, so need to be careful about the
   // validity of existing analyses.
   ir_context->InvalidateAnalysesExceptFor(
       opt::IRContext::Analysis::kAnalysisNone);
+
+  if (message_.value_is_irrelevant()) {
+    transformation_context->GetFactManager()->AddFactValueOfPointeeIsIrrelevant(
+        message_.fresh_id(), ir_context);
+  }
 }
 
 protobufs::Transformation TransformationAddGlobalVariable::ToMessage() const {