From 4ea69b575edd8a6639044b263c184022663d4953 Mon Sep 17 00:00:00 2001 From: fruffy Date: Mon, 5 Aug 2024 08:49:29 +0200 Subject: [PATCH 1/3] Set other headers also invalid when calling setInvalid on a union header. Signed-off-by: fruffy --- .../core/small_step/abstract_stepper.cpp | 52 +++++++----- .../targets/bmv2/test/BMV2PTFXfail.cmake | 9 +++ .../targets/bmv2/test/BMV2STFXfail.cmake | 9 +++ .../test/p4-programs/bmv2_invalid_hu_1.p4 | 76 ++++++++++++++++++ .../test/p4-programs/bmv2_invalid_hu_2.p4 | 80 +++++++++++++++++++ 5 files changed, 205 insertions(+), 21 deletions(-) create mode 100644 backends/p4tools/modules/testgen/targets/bmv2/test/p4-programs/bmv2_invalid_hu_1.p4 create mode 100644 backends/p4tools/modules/testgen/targets/bmv2/test/p4-programs/bmv2_invalid_hu_2.p4 diff --git a/backends/p4tools/modules/testgen/core/small_step/abstract_stepper.cpp b/backends/p4tools/modules/testgen/core/small_step/abstract_stepper.cpp index 7121564f77b..5ca5b9188d8 100644 --- a/backends/p4tools/modules/testgen/core/small_step/abstract_stepper.cpp +++ b/backends/p4tools/modules/testgen/core/small_step/abstract_stepper.cpp @@ -210,38 +210,48 @@ bool AbstractStepper::stepGetHeaderValidity(const IR::StateVariable &headerRef) return false; } -void AbstractStepper::setHeaderValidity(const IR::StateVariable &headerRef, bool validity, - ExecutionState &nextState) { +namespace { + +void setHeaderValidityHelper(const IR::StateVariable &headerRef, bool validity, + const ProgramInfo &programInfo, ExecutionState &nextState) { const auto &headerRefValidity = ToolsVariables::getHeaderValidity(headerRef); nextState.set(headerRefValidity, IR::BoolLiteral::get(validity)); - // In some cases, the header may be `part of a union. - if (validity) { - const auto *headerBaseMember = headerRef.ref->to(); - if (headerBaseMember == nullptr) { - return; + // The header is going to be invalid. Set all fields to taint constants. + // TODO: Should we make this target specific? Some targets set the header fields to 0... + if (!validity) { + std::vector validityVector; + auto fieldsVector = nextState.getFlatFields(headerRef, &validityVector); + for (const auto &field : validityVector) { + nextState.set(field, IR::BoolLiteral::get(false)); + } + for (const auto &field : fieldsVector) { + nextState.set(field, programInfo.createTargetUninitialized(field->type, true)); } - const auto *headerBase = headerBaseMember->expr; + } +} + +} // namespace + +void AbstractStepper::setHeaderValidity(const IR::StateVariable &headerRef, bool validity, + ExecutionState &nextState) { + setHeaderValidityHelper(headerRef, validity, programInfo, nextState); + + // In some cases, the header may be nested. + if (const auto *headerBase = headerRef.ref->to()) { // In the case of header unions, we need to set all other union members invalid. - if (const auto *hdrUnion = headerBase->type->to()) { + CHECK_NULL(headerBase->expr->type); + if (const auto *hdrUnion = headerBase->expr->type->to()) { for (const auto *field : hdrUnion->fields) { - auto *member = new IR::Member(field->type, headerBase, field->name); - // Ignore the member we are setting to valid. - if (headerRef->equiv(*member)) { + // Ignore the member we are setting. + if (headerBase->member == field->name) { continue; } // Set all other members to invalid. - setHeaderValidity(member, false, nextState); + setHeaderValidityHelper(new IR::Member(field->type, headerBase->expr, field->name), + false, programInfo, nextState); } } - return; - } - std::vector validityVector; - auto fieldsVector = nextState.getFlatFields(headerRef, &validityVector); - // The header is going to be invalid. Set all fields to taint constants. - // TODO: Should we make this target specific? Some targets set the header fields to 0. - for (const auto &field : fieldsVector) { - nextState.set(field, programInfo.createTargetUninitialized(field->type, true)); } } diff --git a/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2PTFXfail.cmake b/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2PTFXfail.cmake index 8fa55d5e903..4f3366fd573 100644 --- a/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2PTFXfail.cmake +++ b/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2PTFXfail.cmake @@ -77,6 +77,15 @@ p4tools_add_xfail_reason( v1model-digest-custom-type.p4 ) +p4tools_add_xfail_reason( + "testgen-p4c-bmv2-ptf" + "Expected packet was not received on device" + # BMv2 has the incorrect HU invalidation implementation. + bmv2_invalid_hu_1.p4 + bmv2_invalid_hu_2.p4 + union4-bmv2.p4 +) + #################################################################################################### # 2. P4Testgen Issues # These are failures in P4Testgen that need to be fixed. diff --git a/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2STFXfail.cmake b/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2STFXfail.cmake index 4bd7e1d4abc..ea06588cd6c 100644 --- a/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2STFXfail.cmake +++ b/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2STFXfail.cmake @@ -62,6 +62,15 @@ p4tools_add_xfail_reason( extract_for_header_union.p4 ) +p4tools_add_xfail_reason( + "testgen-p4c-bmv2-stf" + "differs|Expected ([0-9]+) packets on port ([0-9]+) got ([0-9]+)" + # BMv2 has the incorrect HU invalidation implementation. + bmv2_invalid_hu_1.p4 + bmv2_invalid_hu_2.p4 + union4-bmv2.p4 +) + #################################################################################################### # 2. P4Testgen Issues # These are failures in P4Testgen that need to be fixed. diff --git a/backends/p4tools/modules/testgen/targets/bmv2/test/p4-programs/bmv2_invalid_hu_1.p4 b/backends/p4tools/modules/testgen/targets/bmv2/test/p4-programs/bmv2_invalid_hu_1.p4 new file mode 100644 index 00000000000..94ce65a9a87 --- /dev/null +++ b/backends/p4tools/modules/testgen/targets/bmv2/test/p4-programs/bmv2_invalid_hu_1.p4 @@ -0,0 +1,76 @@ +#include + +header opt_t { + bit<8> opt; +} + +header ethernet_t { + bit<48> dst_addr; + bit<48> src_addr; + bit<16> eth_type; +} + +header H { + bit<8> a; + bit<8> a1; + bit<8> b; +} + +header_union U { + ethernet_t e; + H h; +} + +struct Headers { + opt_t opt; + U u; +} + +struct Meta {} + +parser p(packet_in pkt, out Headers h, inout Meta meta, inout standard_metadata_t stdmeta) { + state start { + pkt.extract(h.opt); + transition select(h.opt.opt) { + 0x0: parse_e; + 0x1: parse_h; + default: accept; + } + } + + state parse_e { + pkt.extract(h.u.e); + transition accept; + } + + state parse_h { + pkt.extract(h.u.h); + transition accept; + } +} + +control vrfy(inout Headers h, inout Meta meta) { + apply { } +} + +control ingress(inout Headers h, inout Meta m, inout standard_metadata_t s) { + apply { + h.u.e.setInvalid(); + } +} + +control egress(inout Headers h, inout Meta m, inout standard_metadata_t s) { + apply { } +} + +control update(inout Headers h, inout Meta m) { + apply { } +} + +control deparser(packet_out pkt, in Headers h) { + apply { + pkt.emit(h); + } +} + +V1Switch(p(), vrfy(), ingress(), egress(), update(), deparser()) main; diff --git a/backends/p4tools/modules/testgen/targets/bmv2/test/p4-programs/bmv2_invalid_hu_2.p4 b/backends/p4tools/modules/testgen/targets/bmv2/test/p4-programs/bmv2_invalid_hu_2.p4 new file mode 100644 index 00000000000..6e55348812f --- /dev/null +++ b/backends/p4tools/modules/testgen/targets/bmv2/test/p4-programs/bmv2_invalid_hu_2.p4 @@ -0,0 +1,80 @@ +#include + +header opt_t { + bit<8> opt; +} + +header ethernet_t { + bit<48> dst_addr; + bit<48> src_addr; + bit<16> eth_type; +} + +header H { + bit<8> a; + bit<8> a1; + bit<8> b; +} + +header_union U { + ethernet_t e; + H h; +} + +struct Headers { + opt_t opt; + U u; +} + +struct Meta {} + +parser p(packet_in pkt, out Headers h, inout Meta meta, inout standard_metadata_t stdmeta) { + state start { + pkt.extract(h.opt); + transition select(h.opt.opt) { + 0x0: parse_e; + 0x1: parse_h; + default: accept; + } + } + + state parse_e { + pkt.extract(h.u.e); + transition invalidate; + } + + state parse_h { + pkt.extract(h.u.h); + transition invalidate; + } + + state invalidate { + h.u.e.setInvalid(); + transition accept; + } +} + +control vrfy(inout Headers h, inout Meta meta) { + apply { } +} + +control ingress(inout Headers h, inout Meta m, inout standard_metadata_t s) { + apply { + } +} + +control egress(inout Headers h, inout Meta m, inout standard_metadata_t s) { + apply { } +} + +control update(inout Headers h, inout Meta m) { + apply { } +} + +control deparser(packet_out pkt, in Headers h) { + apply { + pkt.emit(h); + } +} + +V1Switch(p(), vrfy(), ingress(), egress(), update(), deparser()) main; From 17df75bc9c83aeb34d9a1883c1765c5eea899df1 Mon Sep 17 00:00:00 2001 From: fruffy Date: Wed, 7 Aug 2024 09:19:35 +0200 Subject: [PATCH 2/3] Add an option to copy structures to skip expanding header unions. Signed-off-by: fruffy --- backends/bmv2/pna_nic/midend.cpp | 2 +- backends/bmv2/psa_switch/midend.cpp | 2 +- backends/bmv2/simple_switch/midend.cpp | 2 +- backends/dpdk/midend.cpp | 6 +++- backends/p4test/midend.cpp | 6 +++- backends/p4tools/common/compiler/midend.cpp | 8 ++++- backends/ubpf/midend.cpp | 2 +- midend/copyStructures.cpp | 9 +++--- midend/copyStructures.h | 35 ++++++++++++++------- test/gtest/midend_pass.cpp | 2 +- 10 files changed, 50 insertions(+), 24 deletions(-) diff --git a/backends/bmv2/pna_nic/midend.cpp b/backends/bmv2/pna_nic/midend.cpp index 95bf1f9b3ae..a776d61c71c 100644 --- a/backends/bmv2/pna_nic/midend.cpp +++ b/backends/bmv2/pna_nic/midend.cpp @@ -130,7 +130,7 @@ PnaNicMidEnd::PnaNicMidEnd(CompilerOptions &options, std::ostream *outStream) new P4::StrengthReduction(&typeMap), new P4::EliminateTuples(&typeMap), new P4::SimplifyComparisons(&typeMap), - new P4::CopyStructures(&typeMap), + new P4::CopyStructures(&typeMap, P4::CopyStructuresConfig()), new P4::NestedStructs(&typeMap), new P4::SimplifySelectList(&typeMap), new P4::RemoveSelectBooleans(&typeMap), diff --git a/backends/bmv2/psa_switch/midend.cpp b/backends/bmv2/psa_switch/midend.cpp index c92adf5eb4d..5c5840392a1 100644 --- a/backends/bmv2/psa_switch/midend.cpp +++ b/backends/bmv2/psa_switch/midend.cpp @@ -131,7 +131,7 @@ PsaSwitchMidEnd::PsaSwitchMidEnd(CompilerOptions &options, std::ostream *outStre new P4::StrengthReduction(&typeMap), new P4::EliminateTuples(&typeMap), new P4::SimplifyComparisons(&typeMap), - new P4::CopyStructures(&typeMap), + new P4::CopyStructures(&typeMap, P4::CopyStructuresConfig()), new P4::NestedStructs(&typeMap), new P4::SimplifySelectList(&typeMap), new P4::RemoveSelectBooleans(&typeMap), diff --git a/backends/bmv2/simple_switch/midend.cpp b/backends/bmv2/simple_switch/midend.cpp index 402d351021f..7620c6af245 100644 --- a/backends/bmv2/simple_switch/midend.cpp +++ b/backends/bmv2/simple_switch/midend.cpp @@ -98,7 +98,7 @@ SimpleSwitchMidEnd::SimpleSwitchMidEnd(CompilerOptions &options, std::ostream *o new P4::StrengthReduction(&typeMap), new P4::EliminateTuples(&typeMap), new P4::SimplifyComparisons(&typeMap), - new P4::CopyStructures(&typeMap), + new P4::CopyStructures(&typeMap, P4::CopyStructuresConfig()), new P4::NestedStructs(&typeMap), new P4::SimplifySelectList(&typeMap), new P4::RemoveSelectBooleans(&typeMap), diff --git a/backends/dpdk/midend.cpp b/backends/dpdk/midend.cpp index 89cfd232f48..9662baf8189 100644 --- a/backends/dpdk/midend.cpp +++ b/backends/dpdk/midend.cpp @@ -194,7 +194,11 @@ DpdkMidEnd::DpdkMidEnd(CompilerOptions &options, std::ostream *outStream) { new P4::StrengthReduction(&typeMap), new P4::EliminateTuples(&typeMap), new P4::SimplifyComparisons(&typeMap), - new P4::CopyStructures(&typeMap, false /* errorOnMethodCall */), + new P4::CopyStructures(&typeMap, P4::CopyStructuresConfig{ + /*errorOnMethodCall*/ false, + /*copyHeaders*/ false, + /*expandUnions*/ true, + }), new P4::NestedStructs(&typeMap), new P4::SimplifySelectList(&typeMap), new P4::RemoveSelectBooleans(&typeMap), diff --git a/backends/p4test/midend.cpp b/backends/p4test/midend.cpp index 1ab5238f0e3..57978aaa6ae 100644 --- a/backends/p4test/midend.cpp +++ b/backends/p4test/midend.cpp @@ -102,7 +102,11 @@ MidEnd::MidEnd(CompilerOptions &options, std::ostream *outStream) { new P4::StrengthReduction(&typeMap), new P4::EliminateTuples(&typeMap), new P4::SimplifyComparisons(&typeMap), - new P4::CopyStructures(&typeMap, false), + new P4::CopyStructures(&typeMap, P4::CopyStructuresConfig{ + /*errorOnMethodCall*/ false, + /*copyHeaders*/ false, + /*expandUnions*/ true, + }), new P4::NestedStructs(&typeMap), new P4::StrengthReduction(&typeMap), new P4::SimplifySelectList(&typeMap), diff --git a/backends/p4tools/common/compiler/midend.cpp b/backends/p4tools/common/compiler/midend.cpp index f356d62119a..19a46f460ab 100644 --- a/backends/p4tools/common/compiler/midend.cpp +++ b/backends/p4tools/common/compiler/midend.cpp @@ -121,7 +121,13 @@ void MidEnd::addDefaultPasses() { new P4::SimplifyComparisons(&typeMap), // Expand header and struct assignments into sequences of field assignments. new PassRepeated({ - new P4::CopyStructures(&typeMap, false, true, nullptr), + new P4::CopyStructures(&typeMap, + { + /*errorOnMethodCall*/ false, + /*copyHeaders*/ true, + /*expandUnions*/ false, + }, + nullptr), }), new P4::RemoveParserControlFlow(&typeMap), // Flatten nested list expressions. diff --git a/backends/ubpf/midend.cpp b/backends/ubpf/midend.cpp index ec7e3605351..2f66525be05 100644 --- a/backends/ubpf/midend.cpp +++ b/backends/ubpf/midend.cpp @@ -93,7 +93,7 @@ const IR::ToplevelBlock *MidEnd::run(EbpfOptions &options, const IR::P4Program * new P4::StrengthReduction(&typeMap), }), new P4::SimplifyComparisons(&typeMap), - new P4::CopyStructures(&typeMap), + new P4::CopyStructures(&typeMap, P4::CopyStructuresConfig()), new P4::LocalCopyPropagation(&typeMap), new P4::SimplifySelectList(&typeMap), new P4::MoveDeclarations(), // more may have been introduced diff --git a/midend/copyStructures.cpp b/midend/copyStructures.cpp index 14bbb7a9b81..4940f84fc86 100644 --- a/midend/copyStructures.cpp +++ b/midend/copyStructures.cpp @@ -73,13 +73,14 @@ const IR::Node *DoCopyStructures::postorder(IR::AssignmentStatement *statement) FIXME: this is not correct for header unions and should be fixed. The fix bellow, commented-out, causes problems elsewhere. https://github.com/p4lang/p4c/issues/3842 - if (ltype->is()) - return statement; */ + if (!_config.expandUnions && ltype->is()) { + return statement; + } // Do not copy structures for method calls. if (statement->right->is()) { - if (errorOnMethodCall) { + if (_config.errorOnMethodCall) { ::P4::error(ErrorType::ERR_UNSUPPORTED_ON_TARGET, "%1%: functions or methods returning structures " "are not supported on this target", @@ -98,7 +99,7 @@ const IR::Node *DoCopyStructures::postorder(IR::AssignmentStatement *statement) retval->push_back( new IR::AssignmentStatement(statement->srcInfo, left, right->expression)); } - } else if (copyHeaders && ltype->is()) { + } else if (_config.copyHeaders && ltype->is()) { const auto *header = ltype->checkedTo(); // Build a "src.isValid()" call. const auto *isSrcValidCall = new IR::MethodCallExpression( diff --git a/midend/copyStructures.h b/midend/copyStructures.h index 78fcdba6999..67b17e81a70 100644 --- a/midend/copyStructures.h +++ b/midend/copyStructures.h @@ -22,6 +22,21 @@ limitations under the License. namespace P4 { +struct CopyStructuresConfig { + /// Specific targets may allow functions or methods to return structs. + /// Such methods will not be converted in this pass. Setting the + /// errorOnMethodCall flag will produce an error message if such a + /// method is encountered. + bool errorOnMethodCall = true; + + /// Do not only copy normal structures but also perform copy assignments for headers. + bool copyHeaders = false; + /// Also expand header union assignments. + /// TODO: This is only necessary because the copy structure pass does not correctly expand + /// header unions for some back ends. + bool expandUnions = true; +}; + /** * Convert assignments between structures to assignments between fields * @@ -53,19 +68,15 @@ namespace P4 { * */ class DoCopyStructures : public Transform { + /// The type map. TypeMap *typeMap; - /// Specific targets may allow functions or methods to return structs. - /// Such methods will not be converted in this pass. Setting the - /// errorOnMethodCall flag will produce an error message if such a - /// method is encountered. - bool errorOnMethodCall; - /// Do not only copy normal structures but also perform copy assignments for headers. - bool copyHeaders; + /// Configuration options. + CopyStructuresConfig _config; public: - explicit DoCopyStructures(TypeMap *typeMap, bool errorOnMethodCall, bool copyHeaders = false) - : typeMap(typeMap), errorOnMethodCall(errorOnMethodCall), copyHeaders(copyHeaders) { + explicit DoCopyStructures(TypeMap *typeMap, CopyStructuresConfig config) + : typeMap(typeMap), _config(config) { CHECK_NULL(typeMap); setName("DoCopyStructures"); } @@ -116,15 +127,15 @@ class RemoveAliases : public Transform { class CopyStructures : public PassRepeated { public: - explicit CopyStructures(TypeMap *typeMap, bool errorOnMethodCall = true, - bool copyHeaders = false, TypeChecking *typeChecking = nullptr) { + explicit CopyStructures(TypeMap *typeMap, CopyStructuresConfig config, + TypeChecking *typeChecking = nullptr) { CHECK_NULL(typeMap); setName("CopyStructures"); if (typeChecking == nullptr) typeChecking = new TypeChecking(nullptr, typeMap); passes.emplace_back(typeChecking); passes.emplace_back(new RemoveAliases(typeMap)); passes.emplace_back(typeChecking); - passes.emplace_back(new DoCopyStructures(typeMap, errorOnMethodCall, copyHeaders)); + passes.emplace_back(new DoCopyStructures(typeMap, config)); } }; diff --git a/test/gtest/midend_pass.cpp b/test/gtest/midend_pass.cpp index d6752ee7ae0..319bb140978 100644 --- a/test/gtest/midend_pass.cpp +++ b/test/gtest/midend_pass.cpp @@ -83,7 +83,7 @@ MidEnd::MidEnd(CompilerOptions &options, std::ostream *outStream) { new P4::StrengthReduction(&typeMap), new P4::EliminateTuples(&typeMap), new P4::SimplifyComparisons(&typeMap), - new P4::CopyStructures(&typeMap), + new P4::CopyStructures(&typeMap, P4::CopyStructuresConfig()), new P4::NestedStructs(&typeMap), new P4::StrengthReduction(&typeMap), new P4::SimplifySelectList(&typeMap), From afcadeca586d0940b470b8270877b431275b896a Mon Sep 17 00:00:00 2001 From: fruffy Date: Mon, 5 Aug 2024 21:25:42 +0200 Subject: [PATCH 3/3] Implement header union invaidation in the BMv2 back end. Signed-off-by: fruffy --- backends/bmv2/common/action.cpp | 28 +++++++ backends/bmv2/common/parser.cpp | 83 ++++++++++++++----- backends/bmv2/common/parser.h | 2 +- .../targets/bmv2/test/BMV2PTFXfail.cmake | 4 - .../targets/bmv2/test/BMV2STFXfail.cmake | 4 - 5 files changed, 92 insertions(+), 29 deletions(-) diff --git a/backends/bmv2/common/action.cpp b/backends/bmv2/common/action.cpp index 62bdb5ddf14..116a6479d14 100644 --- a/backends/bmv2/common/action.cpp +++ b/backends/bmv2/common/action.cpp @@ -36,6 +36,29 @@ cstring ActionConverter::jsonAssignment(const IR::Type *type) { return "assign"_cs; } +namespace { +/// Invalidates all other headers in a header union except the provided source header. +/// Has no effect if the parent structure is not a header union. +void invalidateOtherHeaderUnionHeaders(const IR::Member *sourceHeader, + const ConversionContext &ctxt, Util::JsonArray *result, + const IR::StatOrDecl *sourceStatement) { + const auto *type = ctxt.typeMap->getType(sourceHeader->expr, true); + if (const auto *headerUnionType = type->to()) { + for (const auto *field : headerUnionType->fields) { + // Do not set the source member invalid. + if (sourceHeader->member == field->name) { + continue; + } + auto *member = new IR::Member(field->type, sourceHeader->expr, field->name); + ctxt.typeMap->setType(member, field->type); + auto *primitive = mkPrimitive("remove_header"_cs, result); + primitive->emplace_non_null("source_info"_cs, sourceStatement->sourceInfoJsonObj()); + primitive->emplace("parameters", new Util::JsonArray({ctxt.conv->convert(member)})); + } + } +} +} // namespace + void ActionConverter::convertActionBody(const IR::Vector *body, Util::JsonArray *result) { for (auto s : *body) { @@ -146,6 +169,11 @@ void ActionConverter::convertActionBody(const IR::Vector *body, prim = "add_header"_cs; } else if (builtin->name == IR::Type_Header::setInvalid) { prim = "remove_header"_cs; + // If setInvalid is called on any header in a header union, we need to + // invalidate all other headers in the union. + if (const auto *parentStructure = builtin->appliedTo->to()) { + invalidateOtherHeaderUnionHeaders(parentStructure, *ctxt, result, s); + } } else if (builtin->name == IR::Type_Stack::push_front) { BUG_CHECK(mc->arguments->size() == 1, "Expected 1 argument for %1%", mc); auto arg = ctxt->conv->convert(mc->arguments->at(0)->expression); diff --git a/backends/bmv2/common/parser.cpp b/backends/bmv2/common/parser.cpp index 815eeda4849..9c85b1458a3 100644 --- a/backends/bmv2/common/parser.cpp +++ b/backends/bmv2/common/parser.cpp @@ -22,9 +22,39 @@ limitations under the License. #include "frontends/p4-14/fromv1.0/v1model.h" #include "frontends/p4/coreLibrary.h" #include "lib/algorithm.h" +#include "lib/json.h" namespace P4::BMV2 { +namespace { +/// Invalidates all other headers in a header union except the provided source header. +/// Has no effect if the parent structure is not a header union. +void invalidateOtherHeaderUnionHeaders(const IR::Member *sourceHeader, + const ConversionContext &ctxt, Util::JsonArray *result) { + const auto *type = ctxt.typeMap->getType(sourceHeader->expr, true); + if (const auto *headerUnionType = type->to()) { + for (const auto *field : headerUnionType->fields) { + // Do not set the source member invalid. + if (sourceHeader->member == field->name) { + continue; + } + auto *member = new IR::Member(field->type, sourceHeader->expr, field->name); + ctxt.typeMap->setType(member, field->type); + auto *obj = new Util::JsonObject(); + obj->emplace("op", "primitive"); + auto *params = mkArrayField(obj, "parameters"_cs); + auto *paramsValue = new Util::JsonObject(); + params->append(paramsValue); + auto *pp = mkArrayField(paramsValue, "parameters"_cs); + auto *biObj = ctxt.conv->convert(member); + pp->append(biObj); + paramsValue->emplace("op", "remove_header"); + result->append(obj); + } + } +} +} // namespace + cstring ParserConverter::jsonAssignment(const IR::Type *type) { if (type->is()) return "assign_union"_cs; if (type->is() || type->is()) return "assign_header"_cs; @@ -40,9 +70,10 @@ cstring ParserConverter::jsonAssignment(const IR::Type *type) { return "set"_cs; } -Util::IJson *ParserConverter::convertParserStatement(const IR::StatOrDecl *stat) { - auto result = new Util::JsonObject(); - auto params = mkArrayField(result, "parameters"_cs); +Util::JsonArray *ParserConverter::convertParserStatement(const IR::StatOrDecl *stat) { + auto *result = new Util::JsonArray(); + auto *obj = new Util::JsonObject(); + auto *params = mkArrayField(obj, "parameters"_cs); auto isR = false; IR::MethodCallExpression *mce2 = nullptr; if (stat->is()) { @@ -104,7 +135,7 @@ Util::IJson *ParserConverter::convertParserStatement(const IR::StatOrDecl *stat) auto assign = stat->to(); auto type = ctxt->typeMap->getType(assign->left, true); cstring operation = jsonAssignment(type); - result->emplace("op", operation); + obj->emplace("op", operation); auto l = ctxt->conv->convertLeftValue(assign->left); bool convertBool = type->is(); auto r = ctxt->conv->convert(assign->right, true, true, convertBool); @@ -116,10 +147,11 @@ Util::IJson *ParserConverter::convertParserStatement(const IR::StatOrDecl *stat) auto wrap = new Util::JsonObject(); wrap->emplace("op", "primitive"); auto params = mkParameters(wrap); - params->append(result); - result = wrap; + params->append(obj); + obj = wrap; } + result->push_back(obj); return result; } else if (stat->is()) { auto mce = stat->to()->methodCall; @@ -135,7 +167,7 @@ Util::IJson *ParserConverter::convertParserStatement(const IR::StatOrDecl *stat) } cstring ename = argCount == 1 ? "extract"_cs : "extract_VL"_cs; - result->emplace("op", ename); + obj->emplace("op", ename); auto arg = mce->arguments->at(0); auto argtype = ctxt->typeMap->getType(arg->expression, true); if (!argtype->is()) { @@ -206,6 +238,7 @@ Util::IJson *ParserConverter::convertParserStatement(const IR::StatOrDecl *stat) rwrap->emplace("value", jexpr); params->append(rwrap); } + result->push_back(obj); return result; } else if (extmeth->method->name.name == corelib.packetIn.lookahead.name) { // bare lookahead call -- should flag an error if there's not enough @@ -218,8 +251,9 @@ Util::IJson *ParserConverter::convertParserStatement(const IR::StatOrDecl *stat) } auto arg = mce->arguments->at(0); auto jexpr = ctxt->conv->convert(arg->expression, true, false); - result->emplace("op", "advance"); + obj->emplace("op", "advance"); params->append(jexpr); + result->push_back(obj); return result; } else if ((extmeth->originalExternType->name == "InternetChecksum" && (extmeth->method->name.name == "clear" || @@ -236,16 +270,17 @@ Util::IJson *ParserConverter::convertParserStatement(const IR::StatOrDecl *stat) json = ExternConverter::cvtExternObject(ctxt, extmeth, mce, stat, true); } if (json) { - result->emplace("op", "primitive"); + obj->emplace("op", "primitive"); params->append(json); } + result->push_back(obj); return result; } } else if (minst->is()) { auto extfn = minst->to(); auto extFuncName = extfn->method->name.name; if (extFuncName == IR::ParserState::verify) { - result->emplace("op", "verify"); + obj->emplace("op", "verify"); BUG_CHECK(mce->arguments->size() == 2, "%1%: Expected 2 arguments", mce); { auto cond = mce->arguments->at(0); @@ -261,10 +296,9 @@ Util::IJson *ParserConverter::convertParserStatement(const IR::StatOrDecl *stat) auto jexpr = ctxt->conv->convert(error->expression, true, false); params->append(jexpr); } - return result; } else if (extFuncName == "assert" || extFuncName == "assume") { BUG_CHECK(mce->arguments->size() == 1, "%1%: Expected 1 argument ", mce); - result->emplace("op", "primitive"); + obj->emplace("op", "primitive"); auto paramValue = new Util::JsonObject(); params->append(paramValue); auto paramsArray = mkArrayField(paramValue, "parameters"_cs); @@ -276,12 +310,13 @@ Util::IJson *ParserConverter::convertParserStatement(const IR::StatOrDecl *stat) } else if (extFuncName == P4V1::V1Model::instance.log_msg.name) { BUG_CHECK(mce->arguments->size() == 2 || mce->arguments->size() == 1, "%1%: Expected 1 or 2 arguments", mce); - result->emplace("op", "primitive"); + obj->emplace("op", "primitive"); auto ef = minst->to(); auto ijson = ExternConverter::cvtExternFunction(ctxt, ef, mce, stat, false); params->append(ijson); - return result; } + result->push_back(obj); + return result; } else if (minst->is()) { /* example result: { @@ -293,7 +328,7 @@ Util::IJson *ParserConverter::convertParserStatement(const IR::StatOrDecl *stat) ], "op" : "primitive" } */ - result->emplace("op", "primitive"); + obj->emplace("op", "primitive"); auto bi = minst->to(); cstring primitive; @@ -301,13 +336,18 @@ Util::IJson *ParserConverter::convertParserStatement(const IR::StatOrDecl *stat) params->append(paramsValue); auto pp = mkArrayField(paramsValue, "parameters"_cs); - auto obj = ctxt->conv->convert(bi->appliedTo); - pp->append(obj); + auto biObj = ctxt->conv->convert(bi->appliedTo); + pp->append(biObj); if (bi->name == IR::Type_Header::setValid) { primitive = "add_header"_cs; } else if (bi->name == IR::Type_Header::setInvalid) { primitive = "remove_header"_cs; + // If setInvalid is called on any header in a header union, we need to + // invalidate all other headers in the union. + if (const auto *parentStructure = bi->appliedTo->to()) { + invalidateOtherHeaderUnionHeaders(parentStructure, *ctxt, result); + } } else if (bi->name == IR::Type_Stack::push_front || bi->name == IR::Type_Stack::pop_front) { if (bi->name == IR::Type_Stack::push_front) @@ -323,6 +363,7 @@ Util::IJson *ParserConverter::convertParserStatement(const IR::StatOrDecl *stat) } paramsValue->emplace("op", primitive); + result->push_back(obj); return result; } } @@ -560,9 +601,11 @@ bool ParserConverter::preorder(const IR::P4Parser *parser) { // For the state we use the internal name, not the control-plane name auto state_id = ctxt->json->add_parser_state(parser_id, state->name); // convert statements - for (auto s : state->components) { - auto op = convertParserStatement(s); - if (op) ctxt->json->add_parser_op(state_id, op); + for (const auto *s : state->components) { + auto *op = convertParserStatement(s); + for (auto *o : *op) { + ctxt->json->add_parser_op(state_id, o); + } } // convert transitions if (state->selectExpression != nullptr) { diff --git a/backends/bmv2/common/parser.h b/backends/bmv2/common/parser.h index b900ec056f4..af41f1377ef 100644 --- a/backends/bmv2/common/parser.h +++ b/backends/bmv2/common/parser.h @@ -38,7 +38,7 @@ class ParserConverter : public Inspector { unsigned combine(const IR::Expression *keySet, const IR::ListExpression *select, big_int &value, big_int &mask, bool &is_vset, cstring &vset_name) const; Util::IJson *stateName(IR::ID state); - Util::IJson *convertParserStatement(const IR::StatOrDecl *stat); + Util::JsonArray *convertParserStatement(const IR::StatOrDecl *stat); Util::IJson *convertSelectKey(const IR::SelectExpression *expr); Util::IJson *convertPathExpression(const IR::PathExpression *expr); Util::IJson *createDefaultTransition(); diff --git a/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2PTFXfail.cmake b/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2PTFXfail.cmake index 4f3366fd573..67f2643a32e 100644 --- a/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2PTFXfail.cmake +++ b/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2PTFXfail.cmake @@ -80,10 +80,6 @@ p4tools_add_xfail_reason( p4tools_add_xfail_reason( "testgen-p4c-bmv2-ptf" "Expected packet was not received on device" - # BMv2 has the incorrect HU invalidation implementation. - bmv2_invalid_hu_1.p4 - bmv2_invalid_hu_2.p4 - union4-bmv2.p4 ) #################################################################################################### diff --git a/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2STFXfail.cmake b/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2STFXfail.cmake index ea06588cd6c..6f1af64acea 100644 --- a/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2STFXfail.cmake +++ b/backends/p4tools/modules/testgen/targets/bmv2/test/BMV2STFXfail.cmake @@ -65,10 +65,6 @@ p4tools_add_xfail_reason( p4tools_add_xfail_reason( "testgen-p4c-bmv2-stf" "differs|Expected ([0-9]+) packets on port ([0-9]+) got ([0-9]+)" - # BMv2 has the incorrect HU invalidation implementation. - bmv2_invalid_hu_1.p4 - bmv2_invalid_hu_2.p4 - union4-bmv2.p4 ) ####################################################################################################