Skip to content

Commit

Permalink
PUBLIC: [P4-Constraints] Remove usages of DebugString.
Browse files Browse the repository at this point in the history
LSC: C++ DebugString to AbslStringify

In this LSC, we replace calls to C++ Protobuf DebugString APIs with implicit AbslStringify conversions or absl::StrCat. This makes debug information incompatible with TextFormat parsers (go/explicit-debug-string), redacts Datapol-annotated SPII values (go/redact-debug-string), and introduces per-process-randomized syntax.

Background:
DebugString callers should not rely on DebugString output (go/no-more-debugstring). proto2::Message DebugString APIs will be deprecated by Protobuf's AbslStringify in order to support SPII redaction. We are incrementally migrating DebugString calls to the new API whenever we are confident it is safe to do so. If this CL causes breakage, please roll back and notify us (orrery-debug-string@).

LSC proposal: go/cpp-debug-string-to-absl-stringify
ISE LSC checklist: go/cpp-debugstring-to-stringify-checklist

Tested:
    TAP --sample ran all affected tests and none failed
    http://test/OCL:598320964:BASE:598306044:1705214817104:33c3a758
PiperOrigin-RevId: 599891641
  • Loading branch information
PINS Team authored and jonathan-dilorenzo committed Jan 26, 2024
1 parent 2407548 commit 7a61f2d
Show file tree
Hide file tree
Showing 7 changed files with 44 additions and 44 deletions.
3 changes: 3 additions & 0 deletions .github/workflows/ci-format.yml
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,7 @@ jobs:
echo "${CHANGED_FILES}"
echo ""
echo "Please run format.sh to apply the changes."
echo "------------------------------------------"
echo "Diff below"
git diff HEAD --
exit 1
12 changes: 6 additions & 6 deletions p4_constraints/backend/constraint_info.cc
Original file line number Diff line number Diff line change
Expand Up @@ -198,12 +198,12 @@ absl::StatusOr<TableInfo> ParseTableInfo(const Table& table) {
if (!keys_by_id.insert({key_info.id, key_info}).second) {
return gutils::InvalidArgumentErrorBuilder(GUTILS_LOC)
<< "table " << table.preamble().name()
<< " has duplicate key: " << key.DebugString();
<< " has duplicate key: " << absl::StrCat(key);
}
if (!keys_by_name.insert({key_info.name, key_info}).second) {
return gutils::InvalidArgumentErrorBuilder(GUTILS_LOC)
<< "table " << table.preamble().name()
<< " has duplicate key: " << key.DebugString();
<< " has duplicate key: " << absl::StrCat(key);
}
}

Expand Down Expand Up @@ -249,12 +249,12 @@ absl::StatusOr<ActionInfo> ParseActionInfo(const Action& action) {
if (!params_by_id.insert({param_info.id, param_info}).second) {
return gutils::InvalidArgumentErrorBuilder(GUTILS_LOC)
<< "action " << action.preamble().name()
<< " has duplicate param: " << param.DebugString();
<< " has duplicate param: " << absl::StrCat(param);
}
if (!params_by_name.insert({param_info.name, param_info}).second) {
return gutils::InvalidArgumentErrorBuilder(GUTILS_LOC)
<< "action " << action.preamble().name()
<< " has duplicate param: " << param.DebugString();
<< " has duplicate param: " << absl::StrCat(param);
}
}
ASSIGN_OR_RETURN(
Expand Down Expand Up @@ -326,7 +326,7 @@ absl::StatusOr<ConstraintInfo> P4ToConstraintInfo(
} else if (!table_info_by_id.insert({table.preamble().id(), *table_info})
.second) {
errors.push_back(gutils::InvalidArgumentErrorBuilder(GUTILS_LOC)
<< "duplicate table: " << table.DebugString());
<< "duplicate table: " << absl::StrCat(table));
}
}

Expand All @@ -337,7 +337,7 @@ absl::StatusOr<ConstraintInfo> P4ToConstraintInfo(
} else if (!action_info_by_id.insert({action.preamble().id(), *action_info})
.second) {
errors.push_back(gutils::InvalidArgumentErrorBuilder(GUTILS_LOC)
<< "duplicate action: " << action.DebugString());
<< "duplicate action: " << absl::StrCat(action));
}
}

Expand Down
4 changes: 2 additions & 2 deletions p4_constraints/backend/interpreter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ absl::StatusOr<EvaluationContext> ParseTableEntry(
return gutils::InternalErrorBuilder(GUTILS_LOC)
<< "Key '" << key_info.name
<< "' of invalid match type detected at runtime: "
<< key_info.type.DebugString();
<< absl::StrCat(key_info.type);
}

TableEntry table_entry{
Expand Down Expand Up @@ -935,7 +935,7 @@ absl::StatusOr<std::string> ReasonEntryViolatesConstraint(
return gutils::InvalidArgumentErrorBuilder(GUTILS_LOC)
<< "action restrictions not supported for entries with the given "
"kind of action: "
<< entry.DebugString();
<< absl::StrCat(entry);
case p4::v1::TableAction::kActionProfileActionSet: {
return internal_interpreter::ReasonEntryViolatesConstraint(
entry.action().action_profile_action_set(), constraint_info);
Expand Down
4 changes: 2 additions & 2 deletions p4_constraints/backend/interpreter_golden_test_runner.cc
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,7 @@ absl::Status main() {
return gutils::InvalidArgumentErrorBuilder(GUTILS_LOC)
<< "action restrictions not supported for entries with the "
"given kind of action: "
<< test_case.table_entry.DebugString();
<< absl::StrCat(test_case.table_entry);
case p4::v1::TableAction::kActionProfileActionSet: {
ASSIGN_OR_RETURN(
std::string action_string,
Expand All @@ -527,7 +527,7 @@ absl::Status main() {
case p4::v1::TableAction::TYPE_NOT_SET:
return gutils::InvalidArgumentErrorBuilder(GUTILS_LOC)
<< "unknown action type "
<< test_case.table_entry.DebugString();
<< absl::StrCat(test_case.table_entry);
}
}

Expand Down
16 changes: 8 additions & 8 deletions p4_constraints/backend/symbolic_interpreter_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -639,9 +639,9 @@ TEST_P(ConstraintTest, ConcretizeEntryGivesEntrySatisfyingConstraints) {
EXPECT_THAT(ReasonEntryViolatesConstraint(concretized_entry, context),
IsOkAndHolds(""))
<< "\nFor entry:\n"
<< concretized_entry.DebugString()
<< concretized_entry
<< "\nConstraint string: " << GetParam().constraint_string
<< "\nConstraint: " << table_info.constraint->DebugString()
<< "\nConstraint: " << *table_info.constraint
<< "\nAnd solver state: " << solver.to_smt2();
}

Expand Down Expand Up @@ -723,9 +723,9 @@ TEST_P(ConstraintTest, EncodeValidTableEntryInZ3AndConcretizeEntry) {
EXPECT_THAT(ReasonEntryViolatesConstraint(concretized_entry, context),
IsOkAndHolds(""))
<< "\nFor entry:\n"
<< concretized_entry.DebugString()
<< concretized_entry
<< "\nConstraint string: " << GetParam().constraint_string
<< "\nConstraint: " << table_info.constraint->DebugString()
<< "\nConstraint: " << *table_info.constraint
<< "\nAnd solver state: " << solver.to_smt2();
}

Expand Down Expand Up @@ -865,9 +865,9 @@ TEST_P(FullySpecifiedConstraintTest, ConcretizeEntryGivesExactEntry) {
GetParam().expected_concretized_entry, context),
IsOkAndHolds(""))
<< "\nFor entry:\n"
<< GetParam().expected_concretized_entry.DebugString()
<< GetParam().expected_concretized_entry
<< "\nConstraint string: " << GetParam().constraint_string
<< "\nConstraint: " << table_info.constraint->DebugString()
<< "\nConstraint: " << *table_info.constraint
<< "\nAnd solver state: " << solver.to_smt2();
}

Expand Down Expand Up @@ -924,9 +924,9 @@ TEST_P(FullySpecifiedConstraintTest,
GetParam().expected_concretized_entry, context),
IsOkAndHolds(""))
<< "\nFor entry:\n"
<< GetParam().expected_concretized_entry.DebugString()
<< GetParam().expected_concretized_entry
<< "\nConstraint string: " << GetParam().constraint_string
<< "\nConstraint: " << table_info.constraint->DebugString()
<< "\nConstraint: " << *table_info.constraint
<< "\nAnd solver state: " << solver.to_smt2();
}

Expand Down
32 changes: 14 additions & 18 deletions p4_constraints/backend/type_checker_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -349,16 +349,14 @@ TEST_F(InferAndCheckTypesTest, LegalTypeCastEqualityComparisonTypeChecks) {
right { key: "$2" }
})pb",
op, left_right.first, left_right.second));
ASSERT_THAT(InferAndCheckTypes(&expr, kTableInfo), IsOk())
<< expr.DebugString();
EXPECT_TRUE(expr.binary_expression().left().has_type_cast())
<< expr.DebugString();
ASSERT_THAT(InferAndCheckTypes(&expr, kTableInfo), IsOk()) << expr;
EXPECT_TRUE(expr.binary_expression().left().has_type_cast()) << expr;
ASSERT_EQ(expr.binary_expression().left().type().type_case(),
expr.binary_expression().right().type().type_case())
<< expr.DebugString();
<< expr;
EXPECT_TRUE(expr.binary_expression().left().type() ==
expr.binary_expression().right().type())
<< expr.DebugString();
<< expr;
}
}
}
Expand All @@ -383,7 +381,7 @@ TEST_F(InferAndCheckTypesTest, IllegalTypeCastEqualityComparisonFails) {
AddMockSourceLocations(expr);
EXPECT_THAT(InferAndCheckTypes(&expr, kTableInfo),
StatusIs(StatusCode::kInvalidArgument))
<< expr.DebugString();
<< expr;
}
}
}
Expand Down Expand Up @@ -450,7 +448,7 @@ TEST_F(InferAndCheckTypesTest, OrderedComparisonOperatorsFails) {
AddMockSourceLocations(expr);
ASSERT_THAT(InferAndCheckTypes(&expr, kTableInfo),
StatusIs(StatusCode::kInvalidArgument))
<< expr.DebugString();
<< expr;
}
}
}
Expand All @@ -466,9 +464,8 @@ TEST_F(InferAndCheckTypesTest, OrderedComparisonOperatorsTypeChecks) {
right { key: "$1" }
})pb",
op, key));
ASSERT_THAT(InferAndCheckTypes(&expr, kTableInfo), IsOk())
<< expr.DebugString();
EXPECT_TRUE(expr.type().has_boolean()) << expr.DebugString();
ASSERT_THAT(InferAndCheckTypes(&expr, kTableInfo), IsOk()) << expr;
EXPECT_TRUE(expr.type().has_boolean()) << expr;
}
}
}
Expand All @@ -495,9 +492,8 @@ TEST_F(InferAndCheckTypesTest, FieldAccessTypeChecks) {
expr { key: "$1" }
})pb",
field, key));
ASSERT_THAT(InferAndCheckTypes(&expr, kTableInfo), IsOk())
<< expr.DebugString();
EXPECT_EQ(expr.type(), field_type) << expr.DebugString();
ASSERT_THAT(InferAndCheckTypes(&expr, kTableInfo), IsOk()) << expr;
EXPECT_EQ(expr.type(), field_type) << expr;
}
}
}
Expand All @@ -524,7 +520,7 @@ TEST_F(InferAndCheckTypesTest, FieldAccess_AccessNonExistingField) {
AddMockSourceLocations(expr);
EXPECT_THAT(InferAndCheckTypes(&expr, kTableInfo),
StatusIs(StatusCode::kInvalidArgument))
<< expr.DebugString();
<< expr;
}
}
}
Expand All @@ -546,7 +542,7 @@ TEST_F(InferAndCheckTypesTest, FieldAccess_AccessFieldOfScalarExpression) {
AddMockSourceLocations(expr);
EXPECT_THAT(InferAndCheckTypes(&expr, kTableInfo),
StatusIs(StatusCode::kInvalidArgument))
<< expr.DebugString();
<< expr;
}
}
}
Expand All @@ -555,8 +551,8 @@ TEST_F(InferAndCheckTypesTest, AttributeAccessTypeChecks) {
Expression expr = ParseTextProtoOrDie<Expression>(R"pb(
attribute_access { attribute_name: "priority" }
)pb");
ASSERT_OK(InferAndCheckTypes(&expr, kTableInfo)) << expr.DebugString();
EXPECT_EQ(expr.type(), kArbitraryInt) << expr.DebugString();
ASSERT_OK(InferAndCheckTypes(&expr, kTableInfo)) << expr;
EXPECT_EQ(expr.type(), kArbitraryInt) << expr;
}

} // namespace p4_constraints
17 changes: 9 additions & 8 deletions p4_constraints_deps.bzl
Original file line number Diff line number Diff line change
@@ -1,28 +1,29 @@
"""Sets up 3rd party workspaces needed to compile p4_constraints."""

load("@bazel_tools//tools/build_defs/repo:git.bzl", "git_repository")
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

def p4_constraints_deps():
"""Sets up 3rd party workspaces needed to compile p4_constraints."""
if not native.existing_rule("com_google_absl"):
git_repository(
http_archive(
name = "com_google_absl",
remote = "https://github.com/abseil/abseil-cpp",
commit = "78be63686ba732b25052be15f8d6dee891c05749", # Abseil LTS 20230125
url = "https://github.com/abseil/abseil-cpp/releases/download/20240116.0/abseil-cpp-20240116.0.tar.gz",
strip_prefix = "abseil-cpp-20240116.0",
sha256 = "338420448b140f0dfd1a1ea3c3ce71b3bc172071f24f4d9a57d59b45037da440",
)
if not native.existing_rule("com_google_googletest"):
http_archive(
name = "com_google_googletest",
urls = ["https://github.com/google/googletest/archive/refs/tags/v1.13.0.tar.gz"],
url = "https://github.com/google/googletest/archive/refs/tags/v1.13.0.tar.gz",
strip_prefix = "googletest-1.13.0",
sha256 = "ad7fdba11ea011c1d925b3289cf4af2c66a352e18d4c7264392fead75e919363",
)
if not native.existing_rule("com_google_protobuf"):
http_archive(
name = "com_google_protobuf",
url = "https://github.com/protocolbuffers/protobuf/releases/download/v22.2/protobuf-22.2.tar.gz",
strip_prefix = "protobuf-22.2",
sha256 = "1ff680568f8e537bb4be9813bac0c1d87848d5be9d000ebe30f0bc2d7aabe045",
url = "https://github.com/protocolbuffers/protobuf/releases/download/v25.2/protobuf-25.2.tar.gz",
strip_prefix = "protobuf-25.2",
sha256 = "8ff511a64fc46ee792d3fe49a5a1bcad6f7dc50dfbba5a28b0e5b979c17f9871",
)
if not native.existing_rule("com_googlesource_code_re2"):
http_archive(
Expand Down

0 comments on commit 7a61f2d

Please sign in to comment.