Skip to content

Commit

Permalink
[p4-constraints] Allow C++ compliant multi-line strings.
Browse files Browse the repository at this point in the history
I.e. strings like:
```
  "This is the start of my string "
  "and this is the end."
```

This will help solve a C++ preprocessor issue we're having where it will randomly insert preprocessor directives into the `@entry_restriction` annotation string when using a single multi-line string.

PiperOrigin-RevId: 628166543
  • Loading branch information
PINS Team authored and jonathan-dilorenzo committed May 3, 2024
1 parent d26400c commit b136fd0
Show file tree
Hide file tree
Showing 5 changed files with 175 additions and 11 deletions.
12 changes: 12 additions & 0 deletions e2e_tests/invalid_constraints.expected.output
Original file line number Diff line number Diff line change
Expand Up @@ -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'
73 changes: 73 additions & 0 deletions e2e_tests/invalid_constraints.p4
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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();
}
}

Expand Down
18 changes: 9 additions & 9 deletions e2e_tests/valid_constraints.expected.output
Original file line number Diff line number Diff line change
Expand Up @@ -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;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Expand Down Expand Up @@ -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<<<
Expand All @@ -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<<<
Expand Down
41 changes: 40 additions & 1 deletion e2e_tests/valid_constraints.p4
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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("
Expand Down Expand Up @@ -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();
}
}

Expand Down
42 changes: 41 additions & 1 deletion p4_constraints/backend/constraint_info.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -135,8 +138,45 @@ absl::StatusOr<absl::optional<ConstraintSource>> 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(
// "<constraint1>"
// "<constraint2>"
// ...
// )
// 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,
};
}
Expand Down

0 comments on commit b136fd0

Please sign in to comment.