From 3fbb0aaf271bffdc343a11af3765ef5c592c00ae Mon Sep 17 00:00:00 2001 From: Angela Zhang Date: Wed, 25 Oct 2023 14:16:03 -0700 Subject: [PATCH] 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 --- p4_constraints/backend/BUILD.bazel | 1 - p4_constraints/backend/constraint_info.cc | 20 ++++++++--- p4_constraints/backend/constraint_info.h | 19 ++++++++-- p4_constraints/backend/interpreter.cc | 36 +++++++++---------- .../backend/interpreter_golden_test_runner.cc | 21 ++++++++--- p4_constraints/backend/interpreter_test.cc | 10 ++++-- .../backend/symbolic_interpreter_test.cc | 32 ++++++++++++++--- 7 files changed, 100 insertions(+), 39 deletions(-) diff --git a/p4_constraints/backend/BUILD.bazel b/p4_constraints/backend/BUILD.bazel index 30ab98a..302cac7 100644 --- 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 index fbe739a..20bbed2 100644 --- a/p4_constraints/backend/constraint_info.cc +++ b/p4_constraints/backend/constraint_info.cc @@ -193,27 +193,37 @@ std::optional 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 P4ToConstraintInfo( const p4::config::v1::P4Info& p4info) { // Allocate output. - absl::flat_hash_map info; + // TODO: b/293655979 - Populate the action_info_by_id map. + absl::flat_hash_map action_info_by_id; + absl::flat_hash_map table_info_by_id; + std::vector errors; for (const Table& table : p4info.tables()) { absl::StatusOr 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 index fe36ce5..24b48b4 100644 --- a/p4_constraints/backend/constraint_info.h +++ b/p4_constraints/backend/constraint_info.h @@ -71,9 +71,24 @@ struct TableInfo { absl::flat_hash_map 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 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; +struct ConstraintInfo { + // Maps from action IDs to ActionInfo. + absl::flat_hash_map action_info_by_id; + // Maps from table IDs to TableInfo. + absl::flat_hash_map table_info_by_id; +}; // Translates P4Info to ConstraintInfo. // diff --git a/p4_constraints/backend/interpreter.cc b/p4_constraints/backend/interpreter.cc index a44b9ad..332f18d 100644 --- a/p4_constraints/backend/interpreter.cc +++ b/p4_constraints/backend/interpreter.cc @@ -761,30 +761,29 @@ absl::StatusOr 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 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 index e084a1f..be49952 100644 --- a/p4_constraints/backend/interpreter_golden_test_runner.cc +++ b/p4_constraints/backend/interpreter_golden_test_runner.cc @@ -89,12 +89,18 @@ absl::StatusOr 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 TestCases() { std::vector 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() ? "\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 index 5dd39f1..a194a04 100644 --- a/p4_constraints/backend/interpreter_test.cc +++ b/p4_constraints/backend/interpreter_test.cc @@ -19,7 +19,6 @@ #include #include -#include #include #include #include @@ -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 index 2a1d270..b0204fe 100644 --- 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")) {