diff --git a/e2e_tests/invalid_constraints.expected.output b/e2e_tests/invalid_constraints.expected.output index 1c7170b..d04ef59 100644 --- a/e2e_tests/invalid_constraints.expected.output +++ b/e2e_tests/invalid_constraints.expected.output @@ -74,3 +74,15 @@ Type error: operand type optional<32> does not support ordered comparison 102 | ::unknown > 10; | ^^^^^^^^^ Type error: unknown attribute 'unknown' + +- e2e_tests/invalid_constraints.p4:113:1-9: + | headers.ethernet.ether_type == 0x86DD; +113 | ::unknown > 10; + | ^^^^^^^^^ +Type error: unknown attribute 'unknown' + +- e2e_tests/invalid_constraints.p4:157:5-13: + | // An invalid constraint to ensure error messages are sensible. +157 | ::unknown > 10; + | ^^^^^^^^^ +Type error: unknown attribute 'unknown' diff --git a/e2e_tests/invalid_constraints.p4 b/e2e_tests/invalid_constraints.p4 index b47d698..f6d1085 100644 --- a/e2e_tests/invalid_constraints.p4 +++ b/e2e_tests/invalid_constraints.p4 @@ -103,6 +103,77 @@ control invalid_constraints(inout headers_t headers, ") table unknown_metadata { actions = {} key = {} } + @file(__FILE__) + @line(__LINE__) + @entry_restriction( + // Either wildcard or exact match (i.e., "optional" match). + "headers.ipv4.dst_addr::mask == 0 || headers.ipv4.dst_addr::mask == -1;" + + // Only match on IPv4 addresses of IPv4 packets. + "headers.ipv4.dst_addr::mask != 0 -> " + // Macros are not usable within a single-line string. + " headers.ethernet.ether_type == 0x0800;" + + // Only match on IPv6 addresses of IPv6 packets. + "headers.ipv6.dst_addr::mask != 0 -> + headers.ethernet.ether_type == IPv6_ETHER_TYPE;" + + // An invalid constraint to ensure error messages are sensible. + "::unknown > 10;" + + // More valid stuff. + "local_metadata.dscp::mask != 0 -> ( + headers.ethernet.ether_type == IPv4_ETHER_TYPE || + headers.ethernet.ether_type == IPv6_ETHER_TYPE || + local_metadata.is_ip_packet == 1 + ); + ") + table invalid_vrf_classifier_table_with_multiline_strings { + key = { + headers.ethernet.ether_type : ternary; + headers.ipv4.dst_addr : ternary; + headers.ipv6.dst_addr : ternary; + local_metadata.dscp : ternary; + local_metadata.is_ip_packet : ternary; + } + actions = { } + } + + @file(__FILE__) + @line(__LINE__) + @entry_restriction(" + // Either wildcard or exact match (i.e., 'optional' match). + headers.ipv4.dst_addr::mask == 0 || headers.ipv4.dst_addr::mask == -1; + + // Only match on IPv4 addresses of IPv4 packets. + headers.ipv4.dst_addr::mask != 0 -> + headers.ethernet.ether_type == 0x0800; + + // Only match on IPv6 addresses of IPv6 packets. + headers.ipv6.dst_addr::mask != 0 -> + headers.ethernet.ether_type == IPv6_ETHER_TYPE; + + // An invalid constraint to ensure error messages are sensible. + ::unknown > 10; + + // More valid stuff. + local_metadata.dscp::mask != 0 -> ( + headers.ethernet.ether_type == IPv4_ETHER_TYPE || + headers.ethernet.ether_type == IPv6_ETHER_TYPE || + local_metadata.is_ip_packet == 1 + ); + ") + table invalid_vrf_classifier_table_without_multiline_strings { + key = { + headers.ethernet.ether_type : ternary; + headers.ipv4.dst_addr : ternary; + headers.ipv6.dst_addr : ternary; + local_metadata.dscp : ternary; + local_metadata.is_ip_packet : ternary; + } + actions = { } + } + apply { forgot_quotes.apply(); forgot_quotes_with_srcloc.apply(); @@ -119,6 +190,8 @@ control invalid_constraints(inout headers_t headers, boolean_negation_of_integer.apply(); optional_does_not_support_ordered_comparison.apply(); unknown_metadata.apply(); + invalid_vrf_classifier_table_with_multiline_strings.apply(); + invalid_vrf_classifier_table_without_multiline_strings.apply(); } } diff --git a/e2e_tests/valid_constraints.expected.output b/e2e_tests/valid_constraints.expected.output index 3aea7ea..f812464 100644 --- a/e2e_tests/valid_constraints.expected.output +++ b/e2e_tests/valid_constraints.expected.output @@ -11,7 +11,7 @@ e2e_tests/table_entries/acl_table_1.pb.txt All entries must satisfy: e2e_tests/valid_constraints.p4:22:5-65: - | // Either wildcard or exact match (i.e., " optional " match). + | // Either wildcard or exact match (i.e., 'optional' match). 22 | hdr.ipv4.dst_addr::mask == 0 || hdr.ipv4.dst_addr::mask == -1; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -84,10 +84,10 @@ e2e_tests/table_entries/optional_match_table_invalid_1.pb.txt === Output === All entries must satisfy: -e2e_tests/valid_constraints.p4:62:5-32: - | // A real constraint: only wildcard match is okay. -62 | hdr.ipv4.dst_addr::mask == 0; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +e2e_tests/valid_constraints.p4:100:5-32: + | // A real constraint: only wildcard match is okay. +100 | hdr.ipv4.dst_addr::mask == 0; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ But your entry does not. >>>Relevant Entry Info<<< @@ -107,10 +107,10 @@ e2e_tests/table_entries/optional_match_table_valid_max_priority.pb.txt === Output === All entries must satisfy: -e2e_tests/valid_constraints.p4:64:5-27: - | // A constraint on metadata. -64 | ::priority < 0x7fffffff; - | ^^^^^^^^^^^^^^^^^^^^^^^ +e2e_tests/valid_constraints.p4:102:5-27: + | // A constraint on metadata. +102 | ::priority < 0x7fffffff; + | ^^^^^^^^^^^^^^^^^^^^^^^ But your entry does not. >>>Relevant Entry Info<<< diff --git a/e2e_tests/valid_constraints.p4 b/e2e_tests/valid_constraints.p4 index de424db..014d7fd 100644 --- a/e2e_tests/valid_constraints.p4 +++ b/e2e_tests/valid_constraints.p4 @@ -18,7 +18,7 @@ control valid_constraints(inout headers_t hdr, @file(__FILE__) @line(__LINE__) @entry_restriction(" - // Either wildcard or exact match (i.e., "optional" match). + // Either wildcard or exact match (i.e., 'optional' match). hdr.ipv4.dst_addr::mask == 0 || hdr.ipv4.dst_addr::mask == -1; // Only match on IPv4 addresses of IPv4 packets. @@ -50,6 +50,44 @@ control valid_constraints(inout headers_t hdr, actions = { } } + + @file(__FILE__) + @line(__LINE__) + // Tests that multiline strings also work. + @entry_restriction( + // Either wildcard or exact match (i.e., "optional" match). + "hdr.ipv4.dst_addr::mask == 0 || hdr.ipv4.dst_addr::mask == -1;" + + // Only match on IPv4 addresses of IPv4 packets. + "hdr.ipv4.dst_addr::mask != 0 -> " + // Macros are not usable within a single-line string. + " hdr.ethernet.ether_type == 0x0800;" + + // Only match on IPv6 addresses of IPv6 packets. + "hdr.ipv6.dst_addr::mask != 0 -> + hdr.ethernet.ether_type == IPv6_ETHER_TYPE; + + local_metadata.dscp::mask != 0 -> ( + hdr.ethernet.ether_type == IPv4_ETHER_TYPE || + hdr.ethernet.ether_type == IPv6_ETHER_TYPE || + local_metadata.is_ip_packet == 1 + ); + ") + @id(6) + table vrf_classifier_table_with_multiline_strings { + key = { + hdr.ethernet.ether_type : ternary; + hdr.ethernet.src_addr : ternary; + hdr.ipv4.dst_addr : ternary; + standard_metadata.ingress_port: ternary; + hdr.ipv6.dst_addr : ternary; + hdr.ipv4.src_addr : optional; + local_metadata.dscp : ternary; + local_metadata.is_ip_packet : ternary; + } + actions = { } + } + @file(__FILE__) @line(__LINE__) @entry_restriction(" @@ -90,6 +128,7 @@ control valid_constraints(inout headers_t hdr, vrf_classifier_table.apply(); optional_match_table.apply(); network_address_table.apply(); + vrf_classifier_table_with_multiline_strings.apply(); } } diff --git a/p4_constraints/backend/constraint_info.cc b/p4_constraints/backend/constraint_info.cc index 388c4f9..fa7d3a4 100644 --- a/p4_constraints/backend/constraint_info.cc +++ b/p4_constraints/backend/constraint_info.cc @@ -24,11 +24,14 @@ #include "absl/log/log.h" #include "absl/status/status.h" #include "absl/status/statusor.h" +#include "absl/strings/match.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_join.h" #include "absl/strings/string_view.h" #include "absl/strings/strip.h" #include "absl/types/optional.h" +#include "gutils/source_location.h" +#include "gutils/status_builder.h" #include "gutils/status_macros.h" #include "p4/config/v1/p4info.pb.h" #include "p4/config/v1/p4types.pb.h" @@ -135,8 +138,45 @@ absl::StatusOr> ExtractConstraint( << "Syntax error: @" << (is_table ? "entry" : "action") << "_restriction must be enclosed in '(\"' and '\")'"; } + + // Consume intermediate, matched double-quotes (") to allow constraints with + // the following shape (and similar for actions): + // ```p4 + // @entry_restriction( + // "" + // "" + // ... + // ) + // table foo { ... } + // ``` + std::string constraint_string_without_quotes = std::string(constraint_string); + // Currently, the p4c frontend collapses all white space in annotations + // (like `@entry_restriction`) into a single space. See + // https://github.com/p4lang/p4c/issues/2333 for more information. + // Thus, rather than preserve the white space, we add a newline as an + // imperfect workaround to improve readability. The idea is that this form + // would most often be used for multi-line input. + RE2::GlobalReplace(&constraint_string_without_quotes, "\" \"", "\n"); + // Ensure there are no further quotation marks in + // `constraint_string_without_quotes`, since those would always be incorrect. + if (absl::StrContains(constraint_string_without_quotes, "\"")) { + return gutils::InvalidArgumentErrorBuilder(GUTILS_LOC) + << "In " + << (constraint_kind == ConstraintKind::kTableConstraint ? "table " + : "action ") + << preamble.name() << ":\n" + << "Syntax error: @" + << (constraint_kind == ConstraintKind::kTableConstraint ? "entry" + : "action") + << "_restriction was parsed with unexpected quotation marks (\"). " + "Ensure you aren't using any quotation marks in a comment. " + "Otherwise, this suggests a parser assumption was violated, " + "likely due to a p4c frontend change, or " + "`@p4runtime_translation` support being added."; + } + return ConstraintSource{ - .constraint_string = std::string(constraint_string), + .constraint_string = std::move(constraint_string_without_quotes), .constraint_location = constraint_location, }; }