From f5d65fe1df25e262fc4b4d3b5238e8465994369f Mon Sep 17 00:00:00 2001 From: PINS Team Date: Fri, 10 Nov 2023 19:33:00 +0000 Subject: [PATCH] PUBLIC: [p4-constraints] Remove EntryMeetsConstraint and replace with ReasonEntryViolatesConstraint in tests. EntryMeetsConstraint and ReasonEntryViolatesConstraint share common code. It was decided that we should keep ReasonEntryViolatesConstraint as it provides output that explains why an entry violated a constraint, whereas EntryMeetsConstraint does not. Tests were changed accordingly to use ReasonEntryViolatesConstraint. Documentation for https://github.com/p4lang/p4-constraints has been updated in another CL. PiperOrigin-RevId: 581321018 --- p4_constraints/backend/interpreter.cc | 37 ------ p4_constraints/backend/interpreter.h | 8 -- p4_constraints/backend/interpreter_test.cc | 131 ++++++++++++--------- 3 files changed, 77 insertions(+), 99 deletions(-) diff --git a/p4_constraints/backend/interpreter.cc b/p4_constraints/backend/interpreter.cc index ed2d88d..51e177a 100644 --- a/p4_constraints/backend/interpreter.cc +++ b/p4_constraints/backend/interpreter.cc @@ -740,43 +740,6 @@ absl::StatusOr Eval(const Expression& expr, // -- Public interface --------------------------------------------------------- -absl::StatusOr EntryMeetsConstraint(const p4::v1::TableEntry& entry, - const ConstraintInfo& context) { - using ::p4_constraints::internal_interpreter::EvalToBool; - using ::p4_constraints::internal_interpreter::EvaluationContext; - using ::p4_constraints::internal_interpreter::P4IDToString; - using ::p4_constraints::internal_interpreter::ParseEntry; - using ::p4_constraints::internal_interpreter::TableEntry; - - // Find table associated with entry. - 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()); - } - - // Check if entry satisfies table constraint (if present). - 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 - << " has non-boolean constraint: " << constraint.DebugString(); - } - - // Parse entry and check constraint. - ASSIGN_OR_RETURN(TableEntry parsed_entry, ParseEntry(entry, *table_info), - _ << " while parsing P4RT table entry for table '" - << table_info->name << "':"); - EvaluationContext eval_context{ - .entry = parsed_entry, - .source = table_info->constraint_source, - }; - // No explanation is returned so no cache is provided. - return EvalToBool(constraint, eval_context, /*eval_cache=*/nullptr); -} - absl::StatusOr ReasonEntryViolatesConstraint( const p4::v1::TableEntry& entry, const ConstraintInfo& context) { using ::p4_constraints::ast::SizeCache; diff --git a/p4_constraints/backend/interpreter.h b/p4_constraints/backend/interpreter.h index f07ec91..178f5da 100644 --- a/p4_constraints/backend/interpreter.h +++ b/p4_constraints/backend/interpreter.h @@ -47,14 +47,6 @@ namespace p4_constraints { absl::StatusOr ReasonEntryViolatesConstraint( const p4::v1::TableEntry& entry, const ConstraintInfo& context); -// Checks if a given table entry satisfies the entry constraint attached to its -// associated table. Returns true if this is the case or if no constraint -// exists. Returns an InvalidArgument Status if the entry belongs to a table not -// present in ConstraintInfo, or if it is inconsistent with the table definition -// in ConstraintInfo. -absl::StatusOr EntryMeetsConstraint(const p4::v1::TableEntry& entry, - const ConstraintInfo& context); - // -- END OF PUBLIC INTERFACE -------------------------------------------------- // Exposed for testing only. diff --git a/p4_constraints/backend/interpreter_test.cc b/p4_constraints/backend/interpreter_test.cc index 0fcf96d..a0ae28c 100644 --- a/p4_constraints/backend/interpreter_test.cc +++ b/p4_constraints/backend/interpreter_test.cc @@ -50,10 +50,11 @@ using ::p4_constraints::ast::Expression; using ::p4_constraints::ast::Type; using ::testing::Contains; using ::testing::Eq; +using ::testing::Not; using ::testing::Pair; using ::testing::UnorderedElementsAre; -class EntryMeetsConstraintTest : public ::testing::Test { +class ReasonEntryViolatesConstraintTest : public ::testing::Test { public: const Type kUnknown = ParseTextProtoOrDie("unknown {}"); const Type kUnsupported = @@ -183,44 +184,54 @@ class EntryMeetsConstraintTest : public ::testing::Test { } }; -class EvalTest : public EntryMeetsConstraintTest {}; -class EvalToBoolCacheTest : public EntryMeetsConstraintTest {}; +class EvalTest : public ReasonEntryViolatesConstraintTest {}; +class EvalToBoolCacheTest : public ReasonEntryViolatesConstraintTest {}; -TEST_F(EntryMeetsConstraintTest, EmptyExpressionErrors) { +TEST_F(ReasonEntryViolatesConstraintTest, EmptyExpressionErrors) { const Expression kExpr; - EXPECT_THAT(EntryMeetsConstraint(kTableEntry, MakeConstraintInfo(kExpr)), - StatusIs(StatusCode::kInvalidArgument)); + EXPECT_THAT( + ReasonEntryViolatesConstraint(kTableEntry, MakeConstraintInfo(kExpr)), + StatusIs(StatusCode::kInvalidArgument)); } -TEST_F(EntryMeetsConstraintTest, BooleanConstants) { +TEST_F(ReasonEntryViolatesConstraintTest, BooleanConstants) { const Expression kConstTrue = ExpressionWithType(kBool, "boolean_constant: true"); - const Expression kConstFalse = - ExpressionWithType(kBool, "boolean_constant: false"); - EXPECT_THAT(EntryMeetsConstraint(kTableEntry, MakeConstraintInfo(kConstTrue)), - IsOkAndHolds(Eq(true))); - EXPECT_THAT( - EntryMeetsConstraint(kTableEntry, MakeConstraintInfo(kConstFalse)), - IsOkAndHolds(Eq(false))); + + // start_location and end_location provided for quoting. + Expression const_false = ParseTextProtoOrDie(R"pb( + start_location { table_name: "table" } + end_location { table_name: "table" } + type { boolean {} } + boolean_constant: false + )pb"); + + EXPECT_THAT(ReasonEntryViolatesConstraint(kTableEntry, + MakeConstraintInfo(kConstTrue)), + IsOkAndHolds(Eq(""))); + EXPECT_THAT(ReasonEntryViolatesConstraint(kTableEntry, + MakeConstraintInfo(const_false)), + IsOkAndHolds(Not(Eq("")))); } -TEST_F(EntryMeetsConstraintTest, NonBooleanConstraintsAreRejected) { +TEST_F(ReasonEntryViolatesConstraintTest, NonBooleanConstraintsAreRejected) { for (const Type& type : {kArbitraryInt, kFixedUnsigned16, kFixedUnsigned32}) { const Expression kExpr = ExpressionWithType(type, R"(integer_constant: "42")"); - EXPECT_THAT(EntryMeetsConstraint(kTableEntry, MakeConstraintInfo(kExpr)), - StatusIs(StatusCode::kInvalidArgument)); + EXPECT_THAT( + ReasonEntryViolatesConstraint(kTableEntry, MakeConstraintInfo(kExpr)), + StatusIs(StatusCode::kInvalidArgument)); } // Expressions evaluating to non-scalar values should also be rejected. for (std::string key : {"exact32", "ternary32", "lpm32", "range32"}) { - EXPECT_THAT( - EntryMeetsConstraint(kTableEntry, MakeConstraintInfo(KeyExpr(key))), - StatusIs(StatusCode::kInvalidArgument)); + EXPECT_THAT(ReasonEntryViolatesConstraint(kTableEntry, + MakeConstraintInfo(KeyExpr(key))), + StatusIs(StatusCode::kInvalidArgument)); } } -TEST_F(EntryMeetsConstraintTest, EntriesWithLeadingZeroesWork) { +TEST_F(ReasonEntryViolatesConstraintTest, EntriesWithLeadingZeroesWork) { const Expression exact_equals_num = ExpressionWithType(kBool, R"pb( binary_expression { binop: EQ @@ -241,21 +252,24 @@ TEST_F(EntryMeetsConstraintTest, EntriesWithLeadingZeroesWork) { } )pb"); // Sanity check that it holds with original entry. - ASSERT_THAT( - EntryMeetsConstraint(kTableEntry, MakeConstraintInfo(exact_equals_num)), - IsOkAndHolds(true)); + ASSERT_THAT(ReasonEntryViolatesConstraint( + kTableEntry, MakeConstraintInfo(exact_equals_num)), + IsOkAndHolds(Eq(""))); // Modify entry to have leading zeroes. p4::v1::TableEntry modified_entry = kTableEntry; modified_entry.mutable_match(0)->mutable_exact()->set_value( absl::StrCat(std::string{'\0'}, kTableEntry.match(0).exact().value())); - EXPECT_THAT(EntryMeetsConstraint(modified_entry, - MakeConstraintInfo(exact_equals_num)), - IsOkAndHolds(true)); + EXPECT_THAT(ReasonEntryViolatesConstraint( + modified_entry, MakeConstraintInfo(exact_equals_num)), + IsOkAndHolds(Eq(""))); } -TEST_F(EntryMeetsConstraintTest, EntriesWithOnlyZeroesWork) { - const Expression exact_equals_num = ExpressionWithType(kBool, R"pb( +TEST_F(ReasonEntryViolatesConstraintTest, EntriesWithOnlyZeroesWork) { + Expression exact_equals_num = ParseTextProtoOrDie(R"pb( + start_location { table_name: "table" } + end_location { table_name: "table" } + type { boolean {} } binary_expression { binop: EQ left { @@ -274,22 +288,26 @@ TEST_F(EntryMeetsConstraintTest, EntriesWithOnlyZeroesWork) { } } )pb"); + // Unmodified entry should not meet constraint since exact32 != 0. - ASSERT_THAT( - EntryMeetsConstraint(kTableEntry, MakeConstraintInfo(exact_equals_num)), - IsOkAndHolds(false)); + ASSERT_THAT(ReasonEntryViolatesConstraint( + kTableEntry, MakeConstraintInfo(exact_equals_num)), + IsOkAndHolds(Not(Eq("")))); // Modify entry to be zero. p4::v1::TableEntry modified_entry = kTableEntry; modified_entry.mutable_match(0)->mutable_exact()->set_value( std::string{'\0'}); - ASSERT_THAT(EntryMeetsConstraint(modified_entry, - MakeConstraintInfo(exact_equals_num)), - IsOkAndHolds(true)); + ASSERT_THAT(ReasonEntryViolatesConstraint( + modified_entry, MakeConstraintInfo(exact_equals_num)), + IsOkAndHolds(Eq(""))); } -TEST_F(EntryMeetsConstraintTest, EntriesWithZeroAsciiValueWorks) { - const Expression exact_equals_num = ExpressionWithType(kBool, R"pb( +TEST_F(ReasonEntryViolatesConstraintTest, EntriesWithZeroAsciiValueWorks) { + Expression exact_equals_num = ParseTextProtoOrDie(R"pb( + start_location { table_name: "table" } + end_location { table_name: "table" } + type { boolean {} } binary_expression { binop: EQ left { @@ -308,17 +326,18 @@ TEST_F(EntryMeetsConstraintTest, EntriesWithZeroAsciiValueWorks) { } } )pb"); + // Unmodified entry should not meet constraint since exact32 != 48. - ASSERT_THAT( - EntryMeetsConstraint(kTableEntry, MakeConstraintInfo(exact_equals_num)), - IsOkAndHolds(false)); + ASSERT_THAT(ReasonEntryViolatesConstraint( + kTableEntry, MakeConstraintInfo(exact_equals_num)), + IsOkAndHolds(Not(Eq("")))); // Modify entry to be zero character. p4::v1::TableEntry modified_entry = kTableEntry; modified_entry.mutable_match(0)->mutable_exact()->set_value("0"); - ASSERT_THAT(EntryMeetsConstraint(modified_entry, - MakeConstraintInfo(exact_equals_num)), - IsOkAndHolds(true)); + ASSERT_THAT(ReasonEntryViolatesConstraint( + modified_entry, MakeConstraintInfo(exact_equals_num)), + IsOkAndHolds(Eq(""))); } Expression GetPriorityEqualityConstraint(const int32_t priority) { @@ -341,14 +360,15 @@ Expression GetPriorityEqualityConstraint(const int32_t priority) { absl::Substitute(kPriorityEqualityConstraint, priority)); } -TEST_F(EntryMeetsConstraintTest, PriorityConstraintWorksWithDefaultPriority) { +TEST_F(ReasonEntryViolatesConstraintTest, + PriorityConstraintWorksWithDefaultPriority) { const Expression kExpr = GetPriorityEqualityConstraint(0); const auto constraint_check_result = - EntryMeetsConstraint(kTableEntry, MakeConstraintInfo(kExpr)); - ASSERT_THAT(constraint_check_result, IsOkAndHolds(Eq(true))); + ReasonEntryViolatesConstraint(kTableEntry, MakeConstraintInfo(kExpr)); + ASSERT_THAT(constraint_check_result, IsOkAndHolds(Eq(""))); } -TEST_F(EntryMeetsConstraintTest, +TEST_F(ReasonEntryViolatesConstraintTest, PriorityConstraintWorksWithNonDefaultPriority) { constexpr absl::string_view kTableEntryWithPriority = R"pb( table_id: 1 @@ -367,18 +387,21 @@ TEST_F(EntryMeetsConstraintTest, // Equality to a different priority. { - const Expression kExpr = GetPriorityEqualityConstraint(0); - const auto constraint_check_result = EntryMeetsConstraint( - table_entry_with_priority, MakeConstraintInfo(kExpr)); - ASSERT_THAT(constraint_check_result, IsOkAndHolds(Eq(false))); + Expression expr = GetPriorityEqualityConstraint(0); + expr.mutable_start_location()->set_table_name(kTableInfo.name); + expr.mutable_end_location()->set_table_name(kTableInfo.name); + + const auto constraint_check_result = ReasonEntryViolatesConstraint( + table_entry_with_priority, MakeConstraintInfo(expr)); + ASSERT_THAT(constraint_check_result, IsOkAndHolds(Not(Eq("")))); } // Equality to the same priority. { const Expression kExpr = GetPriorityEqualityConstraint(priority); - const auto constraint_check_result = EntryMeetsConstraint( + const auto constraint_check_result = ReasonEntryViolatesConstraint( table_entry_with_priority, MakeConstraintInfo(kExpr)); - ASSERT_THAT(constraint_check_result, IsOkAndHolds(Eq(true))); + ASSERT_THAT(constraint_check_result, IsOkAndHolds(Eq(""))); } } @@ -778,7 +801,7 @@ TEST_F(EvalToBoolCacheTest, CacheIsUsed) { } class MinimalSubexpressionLeadingToEvalResultTest - : public EntryMeetsConstraintTest { + : public ReasonEntryViolatesConstraintTest { public: absl::StatusOr MinimalSubexpressionLeadingToEvalResultHelper(const Expression& kConstraint) {