[p4-constraints] Add support for optional match kind. (#58)

Also improved code in the process: - Use exhaustive match where possible. - Use explicit constructors for absl::nullopt and absl::OkStatus() - Improved error messages in a few places. - Fixed bug in interpreter: ternary and similar match keys may be absent in p4runtime proto, and must be default initialized; exact match keys must be present. PiperOrigin-RevId: 359737008 Co-authored-by: PINS Team <[email protected]>
p4lang · Mar 1, 2021 · 920b6ab · 920b6ab
1 parent 1662d96
commit 920b6ab
Show file tree

Hide file tree

Showing 16 changed files with 351 additions and 156 deletions.
diff --git a/.github/workflows/ci-native.yml b/.github/workflows/ci-native.yml
@@ -25,10 +25,10 @@ jobs:
         # See https://docs.bazel.build/versions/master/output_directories.html
         path: "~/.cache/bazel"
         # See https://docs.github.com/en/actions/guides/caching-dependencies-to-speed-up-workflows
-        key: ${{ runner.os }}-build-${{ hashFiles('**/*.bzl', '**/*.bazel') }}
+        key: new-${{ runner.os }}-build-${{ hashFiles('**/*.bzl', '**/*.bazel') }}
         restore-keys: |
-          ${{ runner.os }}-build-
-          ${{ runner.os }}-
+          new-${{ runner.os }}-build-
+          new-${{ runner.os }}-
 
     - name: Install p4c system dependencies (Flex, Bison, GMP)
       run: sudo apt-get update && sudo apt-get install bison flex libfl-dev libgmp-dev
@@ -55,10 +55,10 @@ jobs:
         # See https://docs.bazel.build/versions/master/output_directories.html
         path: "~/.cache/bazel"
         # See https://docs.github.com/en/actions/guides/caching-dependencies-to-speed-up-workflows
-        key: ${{ runner.os }}-test-${{ hashFiles('**/*.bzl', '**/*.bazel') }}
+        key: new-${{ runner.os }}-test-${{ hashFiles('**/*.bzl', '**/*.bazel') }}
         restore-keys: |
-          ${{ runner.os }}-test-
-          ${{ runner.os }}-
+          new-${{ runner.os }}-test-
+          new-${{ runner.os }}-
 
     - name: Install p4c system dependencies (Flex, Bison, GMP)
       run: sudo apt-get update && sudo apt-get install bison flex libfl-dev libgmp-dev

diff --git a/docs/language-specification.md b/docs/language-specification.md
@@ -73,11 +73,19 @@ table:
 | exact        | k::value         | bit\<W\>   |
 | ternary      | k::value         | bit\<W\>   |
 |              | k::mask          | bit\<W\>   |
+| optional     | k::value         | bit\<W\>   |
+|              | k::mask          | bit\<W\>   |
 | lpm          | k::value         | bit\<W\>   |
 |              | k::prefix_length | int        |
 | range        | k::low           | bit\<W\>   |
 |              | k::high          | bit\<W\>   |
 
+Note that an `optional` match is just a restricted kind of `ternary` match whose mask always satisfies the following constraint:
+```
+// Exact match or wildcard match.
+optional_match_key::mask == 0 || optional_match_key::mask == -1
+```
+
 When `k` is of type `bool`, everything behaves precisely as if `k` was of type
 `bit<1>`, with the boolean constant `true` and `false` being mapped to `1` and
 `0`, respectively.

diff --git a/e2e_tests/invalid_constraints.expected.output b/e2e_tests/invalid_constraints.expected.output
@@ -61,3 +61,9 @@ Type error: expected type int, got bool
 84 |     !false -> -8 == -0b1000 || !1
    |                                 ^
 Type error: expected type bool, got int
+
+- e2e_tests/invalid_constraints.p4:92:5-21:
+   |   @entry_restriction("
+92 |     optional_key > 10;
+   |     ^^^^^^^^^^^^^^^^^
+Type error: operand type optional<32> does not support ordered comparison
diff --git a/e2e_tests/invalid_constraints.p4 b/e2e_tests/invalid_constraints.p4
@@ -85,6 +85,17 @@ control invalid_constraints(inout headers_t headers,
   ")
   table boolean_negation_of_integer { actions = {} key = {} }
 
+
+  @file(__FILE__)
+  @line(__LINE__)
+  @entry_restriction("
+    optional_key > 10;
+  ")
+  table optional_does_not_support_ordered_comparison {
+    key = { headers.ipv4.dst_addr : optional  @name("optional_key"); }
+    actions = {}
+  }
+
   apply {
     forgot_quotes.apply();
     forgot_quotes_with_srcloc.apply();
@@ -99,6 +110,7 @@ control invalid_constraints(inout headers_t headers,
     scalar_has_no_field.apply();
     arithmetic_negation_of_boolean.apply();
     boolean_negation_of_integer.apply();
+    optional_does_not_support_ordered_comparison.apply();
    }
  }
 

diff --git a/e2e_tests/table_entries/optional_match_table_invalid_1.pb.txt b/e2e_tests/table_entries/optional_match_table_invalid_1.pb.txt
@@ -0,0 +1,8 @@
+table_id: 4
+match {
+  field_id: 1  # hdr.ipv4.dst_addr
+  optional {
+    # Illegal: constraint only allows wildcard match.
+    value: "123"
+  }
+}
diff --git a/e2e_tests/table_entries/optional_match_table_valid_1.pb.txt b/e2e_tests/table_entries/optional_match_table_valid_1.pb.txt
@@ -0,0 +1,2 @@
+table_id: 4
+# No matches at all is valid, since it corresponds to "don't care".
diff --git a/e2e_tests/valid_constraints.expected.output b/e2e_tests/valid_constraints.expected.output
@@ -2,6 +2,10 @@ e2e_tests/table_entries/accept_all_entries_1.pb.txt: constraint satisfied
 
 e2e_tests/table_entries/acl_table_1.pb.txt: constraint violated
 
+e2e_tests/table_entries/optional_match_table_invalid_1.pb.txt: constraint violated
+
+e2e_tests/table_entries/optional_match_table_valid_1.pb.txt: constraint satisfied
+
 e2e_tests/table_entries/reject_all_entries_1.pb.txt: constraint violated
 
 e2e_tests/table_entries/unknown_table_entry.pb.txt: Error: INVALID_ARGUMENT: table entry with unknown table ID 0 (full ID: 33554432 (0x02000000))

diff --git a/e2e_tests/valid_constraints.p4 b/e2e_tests/valid_constraints.p4
@@ -3,15 +3,20 @@
 control valid_constraints(inout headers_t hdr,
                           inout local_metadata_t local_metadata,
                           inout standard_metadata_t standard_metadata) {
-
+  @file(__FILE__)
+  @line(__LINE__)
   @entry_restriction("true")
   @id(1)
   table accept_all_entries { key = {} actions = {} }
 
+  @file(__FILE__)
+  @line(__LINE__)
   @entry_restriction("false")
   @id(2)
   table reject_all_entries { key = {} actions = {} }
 
+  @file(__FILE__)
+  @line(__LINE__)
   @entry_restriction("
     // Either wildcard or exact match (i.e., "optional" match).
     hdr.ipv4.dst_addr::mask == 0 || hdr.ipv4.dst_addr::mask == -1;
@@ -38,17 +43,35 @@ control valid_constraints(inout headers_t hdr,
       hdr.ipv4.dst_addr : ternary;
       standard_metadata.ingress_port: ternary;
       hdr.ipv6.dst_addr : ternary;
-      hdr.ipv4.src_addr : ternary;
+      hdr.ipv4.src_addr : optional;
       local_metadata.dscp : ternary;
       local_metadata.is_ip_packet : ternary;
     }
     actions = { }
   }
 
+  @file(__FILE__)
+  @line(__LINE__)
+  @entry_restriction("
+    // Vacuously true, just to test syntax and implicit conversions.
+    hdr.ipv4.dst_addr::mask == 0 || hdr.ipv4.dst_addr::mask == -1;
+    hdr.ipv4.dst_addr::value == 10 || hdr.ipv4.dst_addr::value != 10;
+    // Same as above, but using implicit conversion.
+    hdr.ipv4.dst_addr == 10 || hdr.ipv4.dst_addr != 10;
+    // A real constraint: only wildcard match is okay.
+    hdr.ipv4.dst_addr::mask == 0;
+  ")
+  @id(4)
+  table optional_match_table {
+    key = { hdr.ipv4.dst_addr : optional; }
+    actions = {}
+  }
+
   apply {
     accept_all_entries.apply();
     reject_all_entries.apply();
     vrf_classifier_table.apply();
+    optional_match_table.apply();
   }
 }
 

diff --git a/p4_constraints/ast.cc b/p4_constraints/ast.cc
@@ -64,9 +64,13 @@ std::string TypeName(const Type& type) {
       return absl::StrCat("bit<", type.lpm().bitwidth(), ">");
     case Type::kRange:
       return absl::StrCat("range<", type.range().bitwidth(), ">");
-    default:
-      return "???";
+    case Type::kOptionalMatch:
+      return absl::StrCat("optional<", type.optional_match().bitwidth(), ">");
+    case Type::TYPE_NOT_SET:
+      break;
   }
+  LOG(DFATAL) << "invalid type: " << type.DebugString();
+  return "???";
 }
 
 std::ostream& operator<<(std::ostream& os, const Type& type) {
@@ -97,8 +101,10 @@ absl::optional<int> TypeBitwidth(const Type& type) {
       return type.lpm().bitwidth();
     case Type::kRange:
       return type.range().bitwidth();
+    case Type::kOptionalMatch:
+      return type.optional_match().bitwidth();
     default:
-      return {};
+      return absl::nullopt;
   }
 }
 
@@ -119,6 +125,9 @@ bool SetTypeBitwidth(Type* type, int bitwidth) {
     case Type::kRange:
       type->mutable_range()->set_bitwidth(bitwidth);
       return true;
+    case Type::kOptionalMatch:
+      type->mutable_optional_match()->set_bitwidth(bitwidth);
+      return true;
     default:
       return false;
   }
@@ -129,35 +138,38 @@ Type TypeCaseToType(Type::TypeCase type_case) {
   switch (type_case) {
     case Type::kUnknown:
       type.mutable_unknown();
-      break;
+      return type;
     case Type::kUnsupported:
       type.mutable_unsupported();
-      break;
+      return type;
     case Type::kBoolean:
       type.mutable_boolean();
-      break;
+      return type;
     case Type::kArbitraryInt:
       type.mutable_arbitrary_int();
-      break;
+      return type;
     case Type::kFixedUnsigned:
       type.mutable_fixed_unsigned();
-      break;
+      return type;
     case Type::kExact:
       type.mutable_exact();
-      break;
+      return type;
     case Type::kTernary:
       type.mutable_ternary();
-      break;
+      return type;
     case Type::kLpm:
       type.mutable_lpm();
-      break;
+      return type;
     case Type::kRange:
       type.mutable_range();
+      return type;
+    case Type::kOptionalMatch:
+      type.mutable_optional_match();
+      return type;
+    case Type::TYPE_NOT_SET:
       break;
-    default:
-      LOG(DFATAL) << "unknown type case: " << type_case;
   }
-  DCHECK_EQ(type.type_case(), type_case);
+  LOG(DFATAL) << "invalid type case: " << type_case;
   return type;
 }
 

diff --git a/p4_constraints/ast.proto b/p4_constraints/ast.proto
@@ -101,6 +101,8 @@ message Type {
     Ternary ternary = 7;
     Lpm lpm = 8;
     Range range = 9;
+    // `optional` is a reserved name, so we use `optional_match` instead.
+    Optional optional_match = 10;
   }
 
   // Before type-checking, types may be unknown.
@@ -141,6 +143,11 @@ message Type {
   message Range {
     int32 bitwidth = 1;  // required
   }
+
+  // Optional match, aka "Optional<W>".
+  message Optional {
+    int32 bitwidth = 1;  // required
+  }
 }
 
 // Represents the location of a character relative to a source file or table.

diff --git a/p4_constraints/backend/constraint_info.cc b/p4_constraints/backend/constraint_info.cc
@@ -110,6 +110,9 @@ absl::StatusOr<ast::Type> ParseKeyType(const MatchField& key) {
         case MatchField::RANGE:
           type.mutable_range()->set_bitwidth(key.bitwidth());
           return type;
+        case MatchField::OPTIONAL:
+          type.mutable_optional_match()->set_bitwidth(key.bitwidth());
+          return type;
         default:
           return gutils::InvalidArgumentErrorBuilder(GUTILS_LOC)
                  << "match key of invalid MatchType: "
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		table_id: 4
		# No matches at all is valid, since it corresponds to "don't care".