Refactor ConstraintInfo into a struct containing a map from table_id …

…to table_info and a map from action_id to action_info. (#113) This is part of the work to support actions in P4-constraints by allowing users to add constraint annotations for actions. Co-authored-by: PINS Team <[email protected]>
p4lang · Oct 25, 2023 · 3fbb0aa · 3fbb0aa
1 parent c42adaa
commit 3fbb0aa
Show file tree

Hide file tree

Showing 7 changed files with 100 additions and 39 deletions.
diff --git a/p4_constraints/backend/BUILD.bazel b/p4_constraints/backend/BUILD.bazel
@@ -50,7 +50,6 @@ cc_test(
         ":constraint_info",
         ":interpreter",
         "//gutils:parse_text_proto",
-        "//gutils:status",
         "//gutils:status_matchers",
         "//p4_constraints:ast",
         "//p4_constraints:ast_cc_proto",

diff --git a/p4_constraints/backend/constraint_info.cc b/p4_constraints/backend/constraint_info.cc
@@ -193,27 +193,37 @@ std::optional<AttributeInfo> GetAttributeInfo(
 
 const TableInfo* GetTableInfoOrNull(const ConstraintInfo& constraint_info,
                                     uint32_t table_id) {
-  auto it = constraint_info.find(table_id);
-  if (it == constraint_info.end()) return nullptr;
+  auto it = constraint_info.table_info_by_id.find(table_id);
+  if (it == constraint_info.table_info_by_id.end()) return nullptr;
   return &it->second;
 }
 
 absl::StatusOr<ConstraintInfo> P4ToConstraintInfo(
     const p4::config::v1::P4Info& p4info) {
   // Allocate output.
-  absl::flat_hash_map<uint32_t, TableInfo> info;
+  // TODO: b/293655979 - Populate the action_info_by_id map.
+  absl::flat_hash_map<uint32_t, ActionInfo> action_info_by_id;
+  absl::flat_hash_map<uint32_t, TableInfo> table_info_by_id;
+
   std::vector<absl::Status> errors;
 
   for (const Table& table : p4info.tables()) {
     absl::StatusOr<TableInfo> table_info = ParseTableInfo(table);
     if (!table_info.ok()) {
       errors.push_back(table_info.status());
-    } else if (!info.insert({table.preamble().id(), *table_info}).second) {
+    } else if (!table_info_by_id.insert({table.preamble().id(), *table_info})
+                    .second) {
       errors.push_back(gutils::InvalidArgumentErrorBuilder(GUTILS_LOC)
                        << "duplicate table: " << table.DebugString());
     }
   }
-  if (errors.empty()) return info;
+  if (errors.empty()) {
+    ConstraintInfo info{
+        .action_info_by_id = std::move(action_info_by_id),
+        .table_info_by_id = std::move(table_info_by_id),
+    };
+    return info;
+  }
   return gutils::InvalidArgumentErrorBuilder(GUTILS_LOC)
          << "P4Info to constraint info translation failed with the following "
             "errors:\n- "

diff --git a/p4_constraints/backend/constraint_info.h b/p4_constraints/backend/constraint_info.h
@@ -71,9 +71,24 @@ struct TableInfo {
   absl::flat_hash_map<std::string, KeyInfo> keys_by_name;
 };
 
+struct ActionInfo {
+  uint32_t id;       // Same as Action.preamble.id in p4info.proto.
+  std::string name;  // Same as Action.preamble.name in p4info.proto.
+
+  // An optional constraint (aka action_restriction) on actions.
+  absl::optional<ast::Expression> constraint;
+  // If member `constraint` is present, this captures its source. Arbitrary
+  // otherwise.
+  ConstraintSource constraint_source;
+};
+
 // Contains all information required for constraint checking.
-// Technically, a map from table IDs to TableInfo.
-using ConstraintInfo = absl::flat_hash_map<uint32_t, TableInfo>;
+struct ConstraintInfo {
+  // Maps from action IDs to ActionInfo.
+  absl::flat_hash_map<uint32_t, ActionInfo> action_info_by_id;
+  // Maps from table IDs to TableInfo.
+  absl::flat_hash_map<uint32_t, TableInfo> table_info_by_id;
+};
 
 // Translates P4Info to ConstraintInfo.
 //

diff --git a/p4_constraints/backend/interpreter.cc b/p4_constraints/backend/interpreter.cc
@@ -761,30 +761,29 @@ absl::StatusOr<bool> EntryMeetsConstraint(const p4::v1::TableEntry& entry,
   using ::p4_constraints::internal_interpreter::TableEntry;
 
   // Find table associated with entry.
-  auto it = context.find(entry.table_id());
-  if (it == context.end()) {
+  auto* table_info = GetTableInfoOrNull(context, entry.table_id());
+  if (table_info == nullptr) {
     return gutils::InvalidArgumentErrorBuilder(GUTILS_LOC)
            << "table entry with unknown table ID "
            << P4IDToString(entry.table_id());
   }
-  const TableInfo& table_info = it->second;
 
   // Check if entry satisfies table constraint (if present).
-  if (!table_info.constraint.has_value()) return "";
-  const Expression& constraint = table_info.constraint.value();
+  if (!table_info->constraint.has_value()) return "";
+  const Expression& constraint = table_info->constraint.value();
   if (constraint.type().type_case() != Type::kBoolean) {
     return gutils::InvalidArgumentErrorBuilder(GUTILS_LOC)
-           << "table " << table_info.name
+           << "table " << table_info->name
            << " has non-boolean constraint: " << constraint.DebugString();
   }
 
   // Parse entry and check constraint.
-  ASSIGN_OR_RETURN(TableEntry parsed_entry, ParseEntry(entry, table_info),
+  ASSIGN_OR_RETURN(TableEntry parsed_entry, ParseEntry(entry, *table_info),
                    _ << " while parsing P4RT table entry for table '"
-                     << table_info.name << "':");
+                     << table_info->name << "':");
   EvaluationContext eval_context{
       .entry = parsed_entry,
-      .source = table_info.constraint_source,
+      .source = table_info->constraint_source,
   };
   // No explanation is returned so no cache is provided.
   return EvalToBool(constraint, eval_context, /*eval_cache=*/nullptr);
@@ -802,30 +801,29 @@ absl::StatusOr<std::string> ReasonEntryViolatesConstraint(
   using ::p4_constraints::internal_interpreter::TableEntry;
 
   // Find table associated with entry.
-  const auto it = context.find(entry.table_id());
-  if (it == context.end()) {
+  auto* table_info = GetTableInfoOrNull(context, entry.table_id());
+  if (table_info == nullptr) {
     return gutils::InvalidArgumentErrorBuilder(GUTILS_LOC)
            << "table entry with unknown table ID "
            << P4IDToString(entry.table_id());
   }
-  const TableInfo& table_info = it->second;
-
   // Check if entry satisfies table constraint (if present).
-  if (!table_info.constraint.has_value()) return "";
-  const Expression& constraint = table_info.constraint.value();
+  if (!table_info->constraint.has_value()) return "";
+  const Expression& constraint = table_info->constraint.value();
   if (constraint.type().type_case() != Type::kBoolean) {
     return gutils::InvalidArgumentErrorBuilder(GUTILS_LOC)
-           << "table " << table_info.name
+           << "table " << table_info->name
            << " has non-boolean constraint: " << constraint.DebugString();
   }
 
   // Parse entry and check constraint.
-  ASSIGN_OR_RETURN(const TableEntry parsed_entry, ParseEntry(entry, table_info),
+  ASSIGN_OR_RETURN(const TableEntry parsed_entry,
+                   ParseEntry(entry, *table_info),
                    _ << " while parsing P4RT table entry for table '"
-                     << table_info.name << "':");
+                     << table_info->name << "':");
   EvaluationContext eval_context{
       .entry = parsed_entry,
-      .source = table_info.constraint_source,
+      .source = table_info->constraint_source,
   };
   EvaluationCache eval_cache;
   ASSIGN_OR_RETURN(bool entry_satisfies_constraint,

diff --git a/p4_constraints/backend/interpreter_golden_test_runner.cc b/p4_constraints/backend/interpreter_golden_test_runner.cc
@@ -89,12 +89,18 @@ absl::StatusOr<ConstraintInfo> MakeConstraintInfo(TestCase test_case) {
                    ParseConstraint(ConstraintKind::kTableConstraint, source));
   CHECK_OK(InferAndCheckTypes(&(expression), kTableInfo));
   table_info.constraint = expression;
-  return ConstraintInfo{{table_info.id, table_info}};
+  return ConstraintInfo{
+      .action_info_by_id = {},
+      .table_info_by_id{{
+          table_info.id,
+          table_info,
+      }},
+  };
 }
 
-// NOTE: Test cases only define the "exact" field for brevity but kTableInfo is
-// composed of several keys. These undeclared keys are implicitly set to hold
-// "catchall" values (i.e. range=[min,max], ternary=*).
+// NOTE: Test cases only define the "exact" field for brevity but kTableInfo
+// is composed of several keys. These undeclared keys are implicitly set to
+// hold "catchall" values (i.e. range=[min,max], ternary=*).
 std::vector<TestCase> TestCases() {
   std::vector<TestCase> test_cases;
   test_cases.push_back(TestCase{
@@ -239,8 +245,12 @@ absl::Status main() {
   for (const TestCase& test_case : TestCases()) {
     ASSIGN_OR_RETURN(ConstraintInfo constraint_info,
                      MakeConstraintInfo(test_case));
+    auto* table_info = GetTableInfoOrNull(constraint_info, 1);
+    if (table_info == nullptr) {
+      return absl::InvalidArgumentError("No table info");
+    }
     ASSIGN_OR_RETURN(TableEntry input,
-                     ParseEntry(test_case.table_entry, constraint_info.at(1)));
+                     ParseEntry(test_case.table_entry, *table_info));
     std::cout << "### ReasonEntryViolatestConstraint Test ###################\n"
               << "=== INPUT ===\n"
               << "--- Constraint ---\n"
@@ -255,6 +265,7 @@ absl::Status main() {
                 << (result->empty() ? "<empty>\n" : *result) << "\n";
     } else {
       std::cout << "=== ERROR ===\n" << result.status();
+      return absl::InvalidArgumentError(result.status().ToString());
     }
   }
   return absl::OkStatus();

diff --git a/p4_constraints/backend/interpreter_test.cc b/p4_constraints/backend/interpreter_test.cc
@@ -19,7 +19,6 @@
 #include <gtest/gtest.h>
 #include <stdint.h>
 
-#include <optional>
 #include <string>
 #include <utility>
 #include <variant>
@@ -32,7 +31,6 @@
 #include "absl/strings/string_view.h"
 #include "absl/strings/substitute.h"
 #include "gutils/parse_text_proto.h"
-#include "gutils/status_macros.h"
 #include "gutils/status_matchers.h"
 #include "p4/v1/p4runtime.pb.h"
 #include "p4_constraints/ast.h"
@@ -129,7 +127,13 @@ class EntryMeetsConstraintTest : public ::testing::Test {
     table_info.constraint = expr;
     table_info.constraint_source.constraint_location.set_table_name(
         table_info.name);
-    return {{table_info.id, table_info}};
+    return {
+        .action_info_by_id = {},
+        .table_info_by_id = {{
+            table_info.id,
+            table_info,
+        }},
+    };
   }
 
   static Expression ExpressionWithType(const Type& type,

diff --git a/p4_constraints/backend/symbolic_interpreter_test.cc b/p4_constraints/backend/symbolic_interpreter_test.cc
@@ -628,7 +628,13 @@ TEST_P(ConstraintTest, ConcretizeEntryGivesEntrySatisfyingConstraints) {
     }
   }
 
-  ConstraintInfo context{{table_info.id, table_info}};
+  ConstraintInfo context{
+      .action_info_by_id = {},
+      .table_info_by_id = {{
+          table_info.id,
+          table_info,
+      }},
+  };
   // The empty string signifies that the entry doesn't violate the constraint.
   EXPECT_THAT(ReasonEntryViolatesConstraint(concretized_entry, context),
               IsOkAndHolds(""))
@@ -707,7 +713,13 @@ TEST_P(ConstraintTest, EncodeValidTableEntryInZ3AndConcretizeEntry) {
       ConcretizeEntry(solver.get_model(), table_info, environment));
 
   // The empty string signifies that the entry doesn't violate the constraint.
-  ConstraintInfo context{{table_info.id, table_info}};
+  ConstraintInfo context{
+      .action_info_by_id = {},
+      .table_info_by_id = {{
+          table_info.id,
+          table_info,
+      }},
+  };
   EXPECT_THAT(ReasonEntryViolatesConstraint(concretized_entry, context),
               IsOkAndHolds(""))
       << "\nFor entry:\n"
@@ -837,7 +849,13 @@ TEST_P(FullySpecifiedConstraintTest, ConcretizeEntryGivesExactEntry) {
 
   TableInfo table_info =
       GetTableInfoWithConstraint(GetParam().constraint_string);
-  ConstraintInfo context{{table_info.id, table_info}};
+  ConstraintInfo context{
+      .action_info_by_id = {},
+      .table_info_by_id = {{
+          table_info.id,
+          table_info,
+      }},
+  };
   // TODO(b/297400616): p4-constraints currently does not correctly translate
   // the bytestring representing 1280 to 1280 (instead turning it into 5).
   if (!absl::StrContains(GetParam().constraint_string, "1280")) {
@@ -890,7 +908,13 @@ TEST_P(FullySpecifiedConstraintTest,
 
   TableInfo table_info =
       GetTableInfoWithConstraint(GetParam().constraint_string);
-  ConstraintInfo context{{table_info.id, table_info}};
+  ConstraintInfo context{
+      .action_info_by_id = {},
+      .table_info_by_id = {{
+          table_info.id,
+          table_info,
+      }},
+  };
   // TODO(b/297400616): p4-constraints currently does not correctly translate
   // the bytestring representing 1280 to 1280 (instead turning it into 5).
   if (!absl::StrContains(GetParam().constraint_string, "1280")) {