Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
#
# SPDX-License-Identifier: Apache-2.0

FROM ubuntu:16.04
FROM ubuntu:24.04

ARG DEBIAN_FRONTEND=noninteractive

Expand Down
12 changes: 6 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -181,14 +181,14 @@ bazel test //... # Run tests in container.

The easiest way to experiment with p4-constraints is to write a
[golden test](https://ro-che.info/articles/2017-12-04-golden-tests).
We provide [Bazel rules](e2e_test/p4check.bzl) `run_p4check` and `diff_test` to
We provide [Bazel rules](e2e_tests/p4check.bzl) `run_p4check` and `diff_test` to
make this convenient.
See the [e2e_test/](e2e_test/) folder -- in particular
[e2e_test/BUILD.bazel](e2e_test/BUILD.bazel) -- for examples of how to use them.
See the [e2e_tests/](e2e_tests/) folder -- in particular
[e2e_tests/BUILD.bazel](e2e_tests/BUILD.bazel) -- for examples of how to use them.

To run all golden tests, execute
```sh
bazel test //e2e_test/...
bazel test //e2e_tests/...
```
[Recall](#building) that this will build p4c and requires
[Bison](https://en.wikipedia.org/wiki/GNU_Bison) and
Expand All @@ -204,7 +204,7 @@ bazel run //e2e_test:invalid_constraints_test

The `p4check` CLI allows invoking the p4-constraints library from the command
line. The most convenient way to run `p4check` is using the
[`run_p4check`-rule](e2e_test/p4check.bzl), as is done for
[`run_p4check`-rule](e2e_tests/p4check.bzl), as is done for
[golden testing](#golden-tests).

To learn how to invoke [p4check](p4_constraints/cli/p4check.cc) manually,
Expand All @@ -217,7 +217,7 @@ bazel run p4_constraints/cli:p4check -- --help

See [docs/language-specification.md](docs/language-specification.md) for a
documentation of the constraint languages, or look at some example constraints
in the .p4-files in the [e2e_test folder](e2e_test/).
in the .p4-files in the [e2e_tests folder](e2e_tests/).

## Contributing

Expand Down
3 changes: 2 additions & 1 deletion format.sh
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,5 @@ find . -not -path "./third_party/**" \
| egrep "\.(${CLANG_FORMAT_EXTENSIONS})\$" \
| xargs clang-format --verbose -style=google -i

bazel run -- @buildifier_prebuilt//:buildifier --lint=fix -r .
bazel run -- \
@buildifier_prebuilt//:buildifier --lint=fix -r $(bazel info workspace)
48 changes: 44 additions & 4 deletions p4_constraints/backend/type_checker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,33 @@ bool StrictlyAboveInCastabilityOrder(const Type& left, const Type& right) {
return false;
}
}
// Returns true if 'left' is exactly one level above 'right' in the castability
// order. This ensures that casts only move one level up the Hasse diagram.
bool OneLevelAboveInCastabilityOrder(const Type& left, const Type& right) {
// Check if left is strictly above right in the castability order
if (!StrictlyAboveInCastabilityOrder(left, right)) {
return false;
}

// Now check that there isn't an intermediate type between them
// For the specific Hasse diagram used in P4-Constraints:

// Case 1: right is arbitrary_int, left must be fixed_unsigned
if (right.type_case() == Type::kArbitraryInt) {
return left.type_case() == Type::kFixedUnsigned;
}

// Case 2: right is fixed_unsigned, left must be one of the match types
if (right.type_case() == Type::kFixedUnsigned) {
return left.type_case() == Type::kExact ||
left.type_case() == Type::kTernary ||
left.type_case() == Type::kLpm || left.type_case() == Type::kRange ||
left.type_case() == Type::kOptionalMatch;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be misunderstanding this, but in the case of casting bit<W> (kFixedUnsigned) to exact<W> | ternary<W> | lpm<W> | range<W> | optional<W>, do we need to check that the bit widths match?

e.g. in StrictlyAboveInCastabilityOrder:

switch (left.type_case()) {
    case Type::kExact:
    case Type::kTernary:
    case Type::kLpm:
    case Type::kRange:
    case Type::kOptionalMatch:
      switch (right.type_case()) {
        case Type::kFixedUnsigned:
          return TypeBitwidth(left) == TypeBitwidth(right); <------ bit width check
        case Type::kArbitraryInt:
          return true;
        default:
          return false;
      }

Copy link
Copy Markdown
Author

@Kaiwen-Guo Kaiwen-Guo Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's a great point. I now added additionalTypeBitwidth checks during case 2 return


// No other valid one-level jumps in the hierarchy
return false;
}
// Returns the least upper bound of the given types in the castability ordering.
//
// The least upper bound (join, supremum) is the lowest common ancestor of the
Expand Down Expand Up @@ -325,11 +351,25 @@ absl::Status InferAndCheckTypes(Expression* expr, const ActionInfo* action_info,
return absl::OkStatus();
}

case ast::Expression::kTypeCast:
return StaticTypeError(constraint_source, expr->start_location(),
expr->end_location())
<< "type casts should only be inserted by the type checker";
case ast::Expression::kTypeCast: {
// Type check the inner expression first
Expression* inner_expr = expr->mutable_type_cast();
RETURN_IF_ERROR(InferAndCheckTypes(inner_expr, action_info, table_info));

const Type& target_type = expr->type();
const Type& inner_type = inner_expr->type();

// Validate the cast is exactly one level in the castability order
if (!OneLevelAboveInCastabilityOrder(target_type, inner_type)) {
return StaticTypeError(constraint_source, expr->start_location(),
expr->end_location())
<< "invalid type cast from " << TypeName(inner_type) << " to "
<< TypeName(target_type)
<< " - must be a single-step cast in the type hierarchy";
}

return absl::OkStatus();
}
case Expression::kBinaryExpression: {
BinaryExpression* bin_expr = expr->mutable_binary_expression();
Expression* left = bin_expr->mutable_left();
Expand Down
24 changes: 22 additions & 2 deletions p4_constraints/backend/type_checker_test.cc
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, changing InferAndCheckTypes() to be idempotent should cause TypeCastNeverTypeChecks to fail because we are no longer erroring immediately upon encountering a type cast. Based on a quick look it seems to me that the test case Expression expr = ParseTextProtoOrDie<Expression>(R"pb(type_cast { integer_constant: "0" })pb"); is not a valid type cast expression since left is set to TYPE_NOT_SET and right is set to kArbitraryInt when OneLevelAboveInCastabilityOrder() is called, which instantly returns false because TYPE_NOT_SET isn't accounted for in the castability order.

i.e. I think that the test is currently passing because the function returns kInvalidArgument for the wrong reason.

I got the test to fail (In this case return Ok() when an error was expected) using the test case below - let me know if you can reproduce

TEST_F(InferAndCheckTypesTest, TypeCastNeverTypeChecks) {
  Expression expr = ParseTextProtoOrDie<Expression>(R"pb(
    binary_expression {
      binop: EQ
      left {
        type { exact { bitwidth: 32 } }
        key: "exact32"
      }
      right {
        type { exact { bitwidth: 32 } }
        type_cast {
          type { fixed_unsigned { bitwidth: 32 } }
          type_cast {
            type { arbitrary_int {} }
            integer_constant: "48"
          }
        }
      }
    }
  )pb");
  AddMockSourceLocations(expr);
  ASSERT_THAT(InferAndCheckTypes(&expr, kTableInfo),
              StatusIs(StatusCode::kInvalidArgument));
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really depends on whether nested type casts should be allowed. If it should not be allowed, then we can add an additional check after getting the inner expression to raise any corresponding errors:

      if (inner_expr->expression_case() == ast::Expression::kTypeCast) {
        return StaticTypeError(constraint_source, expr->start_location(),
                               expr->end_location())
               << "nested type casts are not allowed";
      }

Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,28 @@ TEST_F(InferAndCheckTypesTest, IllegalTypeCastEqualityComparisonFails) {
}
}
}
TEST_F(InferAndCheckTypesTest, IdempotentTypeCastHandling) {
const std::pair<std::string, std::string> castable_pairs[] = {
{"int", "bit16"}, {"int", "exact32"}, {"bit32", "exact32"}};
for (ast::BinaryOperator op : {ast::EQ, ast::NE}) {
for (std::pair<std::string, std::string> left_right : castable_pairs) {
Expression expr = ParseTextProtoOrDie<Expression>(
absl::Substitute(R"pb(
binary_expression {
binop: $0
left { key: "$1" }
right { key: "$2" }
})pb",
op, left_right.first, left_right.second));

ASSERT_THAT(InferAndCheckTypes(&expr, kTableInfo), IsOk())
<< "First type check failed: " << expr.DebugString();
// Idempotent type cast test.
ASSERT_THAT(InferAndCheckTypes(&expr, kTableInfo), IsOk())
<< "Idempotence test failed: " << expr.DebugString();
}
}
}
TEST_F(InferAndCheckTypesTest, BinaryBooleanOperators) {
for (ast::BinaryOperator op : {ast::AND, ast::OR, ast::IMPLIES}) {
// Positive tests.
Expand Down Expand Up @@ -561,5 +582,4 @@ TEST_F(InferAndCheckTypesTest, AttributeAccessTypeChecks) {
ASSERT_OK(InferAndCheckTypes(&expr, kTableInfo)) << expr.DebugString();
EXPECT_EQ(expr.type(), kArbitraryInt) << expr.DebugString();
}

} // namespace p4_constraints
} // namespace p4_constraints