From 14b30a00ecd50c8806d061a017bffa7d984c5b4d Mon Sep 17 00:00:00 2001 From: aeron-gh Date: Wed, 17 Jun 2026 01:59:20 +0530 Subject: [PATCH] Reject duplicate match_kind declarations Per P4-16 spec section 7.1.3, all match_kind identifiers are inserted into the top-level namespace and it is an error to declare the same one more than once. p4c already enforced this within a single match_kind block but silently accepted duplicates spread across separate blocks, including the common case of a program redeclaring a match_kind that core.p4 already defines. Handle match_kind the same way the frontend already handles error: the parser driver merges every match_kind declaration into a single node, so IndexedVector emits the duplicate-declaration error, and ToP4 prints only the user-defined members so the merged node is not hidden behind the system include. pipe.p4 redeclared ternary and exact from core.p4, so its redundant block is removed. Other samples that add genuinely new match_kinds now print that block next to the include, mirroring how error blocks are emitted. Fixes #5085 Signed-off-by: aeron-gh --- frontends/p4/toP4/toP4.cpp | 19 ++++++++++++++++--- frontends/parsers/p4/p4parser.ypp | 5 +++-- frontends/parsers/parserDriver.cpp | 9 +++++++++ frontends/parsers/parserDriver.h | 10 ++++++++++ ...plicate-fields-in-separate-declarations.p4 | 12 ++++++++++++ ...-fields-in-separate-declarations.p4-stderr | 7 +++++++ testdata/p4_16_samples/pipe.p4 | 5 ----- .../dash/dash-pipeline-pna-dpdk-first.p4 | 10 +++++----- .../dash/dash-pipeline-pna-dpdk-frontend.p4 | 10 +++++----- .../dash/dash-pipeline-pna-dpdk-midend.p4 | 10 +++++----- .../dash/dash-pipeline-pna-dpdk.p4 | 10 +++++----- .../dash/dash-pipeline-v1model-bmv2-first.p4 | 10 +++++----- .../dash-pipeline-v1model-bmv2-frontend.p4 | 10 +++++----- .../dash/dash-pipeline-v1model-bmv2-midend.p4 | 10 +++++----- .../dash/dash-pipeline-v1model-bmv2.p4 | 10 +++++----- .../p4_16_samples_outputs/issue982-first.p4 | 10 +++++----- .../issue982-frontend.p4 | 10 +++++----- .../p4_16_samples_outputs/issue982-midend.p4 | 10 +++++----- testdata/p4_16_samples_outputs/issue982.p4 | 10 +++++----- testdata/p4_16_samples_outputs/pipe-first.p4 | 4 ---- .../p4_16_samples_outputs/pipe-frontend.p4 | 4 ---- testdata/p4_16_samples_outputs/pipe-midend.p4 | 4 ---- testdata/p4_16_samples_outputs/pipe.p4 | 4 ---- testdata/p4_16_samples_outputs/pipe.p4-stderr | 2 +- 24 files changed, 118 insertions(+), 87 deletions(-) create mode 100644 testdata/p4_16_errors/decl-matchkind-with-duplicate-fields-in-separate-declarations.p4 create mode 100644 testdata/p4_16_errors_outputs/decl-matchkind-with-duplicate-fields-in-separate-declarations.p4-stderr diff --git a/frontends/p4/toP4/toP4.cpp b/frontends/p4/toP4/toP4.cpp index 9407dc0b92b..b71b02557bf 100644 --- a/frontends/p4/toP4/toP4.cpp +++ b/frontends/p4/toP4/toP4.cpp @@ -152,8 +152,12 @@ bool ToP4::preorder(const IR::P4Program *program) { for (auto a : program->objects) { // Check where this declaration originates auto sourceFileOpt = ifSystemFile(a); - // Errors can come from multiple files - if (!a->is() && sourceFileOpt.has_value()) { + // Errors and match_kinds can be declared across multiple files, so the + // merged node carries the source location of the first declaration (often + // a system file). Always visit them so any user-defined members are + // emitted instead of being hidden behind the #include. + if (!a->is() && !a->is() && + sourceFileOpt.has_value()) { /* FIXME -- when including a user header file (sourceFile != * mainFile), do we want to emit an #include of it or not? Probably * not when translating from P4-14, as that would create a P4-16 @@ -692,10 +696,19 @@ bool ToP4::preorder(const IR::Type_Error *d) { bool ToP4::preorder(const IR::Declaration_MatchKind *d) { dump(1); + + const auto userMatchKinds = d->getDeclarations() + ->where([this](const IR::IDeclaration *e) { + // only print if not from a system file + return !ifSystemFile(e->getNode()).has_value(); + }) + ->toVector(); + if (userMatchKinds.empty()) return false; + builder.append("match_kind "); builder.blockStart(); bool first = true; - for (auto a : *d->getDeclarations()) { + for (auto a : userMatchKinds) { if (!first) builder.append(",\n"); dump(1, a->getNode(), 1); first = false; diff --git a/frontends/parsers/p4/p4parser.ypp b/frontends/parsers/p4/p4parser.ypp index 74d4b2852cf..2a03b2a4e8a 100644 --- a/frontends/parsers/p4/p4parser.ypp +++ b/frontends/parsers/p4/p4parser.ypp @@ -315,8 +315,9 @@ using namespace P4; parserDeclaration controlDeclaration enumDeclaration typedefDeclaration packageTypeDeclaration typeDeclaration %type errorDeclaration +%type matchKindDeclaration %type fragment -%type declaration externDeclaration matchKindDeclaration +%type declaration externDeclaration %type parserTypeDeclaration %type controlTypeDeclaration %type> parserLocalElements controlLocalDeclarations @@ -493,7 +494,7 @@ declaration | controlDeclaration { $$ = $1; } | instantiation { $$ = $1; } | errorDeclaration { $$ = driver.onReadErrorDeclaration($1) ? $1 : nullptr; } - | matchKindDeclaration { $$ = $1; } + | matchKindDeclaration { $$ = driver.onReadMatchKindDeclaration($1) ? $1 : nullptr; } | functionDeclaration { $$ = $1; } ; diff --git a/frontends/parsers/parserDriver.cpp b/frontends/parsers/parserDriver.cpp index a1be68ec4f5..da03e2d1127 100644 --- a/frontends/parsers/parserDriver.cpp +++ b/frontends/parsers/parserDriver.cpp @@ -306,6 +306,15 @@ bool P4ParserDriver::onReadErrorDeclaration(IR::Type_Error *error) { return false; } +bool P4ParserDriver::onReadMatchKindDeclaration(IR::Declaration_MatchKind *matchKind) { + if (allMatchKinds == nullptr) { + allMatchKinds = matchKind; + return true; + } + allMatchKinds->members.append(matchKind->members); + return false; +} + } // namespace P4 namespace P4::V1 { diff --git a/frontends/parsers/parserDriver.h b/frontends/parsers/parserDriver.h index f35e16a7b71..61e5a96d1ef 100644 --- a/frontends/parsers/parserDriver.h +++ b/frontends/parsers/parserDriver.h @@ -183,6 +183,11 @@ class P4ParserDriver final : public AbstractParserDriver { // been combined into a previous one (and should be elided) bool onReadErrorDeclaration(IR::Type_Error *error); + /// Notify that the parser parsed a P4 `match_kind` declaration. + // @return true if this is the first match_kind declaration, false if it has + // been combined into a previous one (and should be elided) + bool onReadMatchKindDeclaration(IR::Declaration_MatchKind *matchKind); + //////////////////////////////////////////////////////////////////////////// // Shared state manipulated directly by the lexer and parser. //////////////////////////////////////////////////////////////////////////// @@ -217,6 +222,11 @@ class P4ParserDriver final : public AbstractParserDriver { /// lazily created the first time we see an `error` declaration. (This node /// is present in @declarations as well.) IR::Type_Error *allErrors = nullptr; + + /// All P4 `match_kind` declarations are merged together in the node, which + /// is lazily created the first time we see a `match_kind` declaration. (This + /// node is present in @declarations as well.) + IR::Declaration_MatchKind *allMatchKinds = nullptr; }; } // namespace P4 diff --git a/testdata/p4_16_errors/decl-matchkind-with-duplicate-fields-in-separate-declarations.p4 b/testdata/p4_16_errors/decl-matchkind-with-duplicate-fields-in-separate-declarations.p4 new file mode 100644 index 00000000000..32976133efc --- /dev/null +++ b/testdata/p4_16_errors/decl-matchkind-with-duplicate-fields-in-separate-declarations.p4 @@ -0,0 +1,12 @@ +// It is an error to declare the same match_kind identifier multiple times, +// even when the duplicates appear in separate match_kind declarations. +// See https://github.com/p4lang/p4c/issues/5085 + +match_kind { + foo, + bar +} + +match_kind { + foo +} diff --git a/testdata/p4_16_errors_outputs/decl-matchkind-with-duplicate-fields-in-separate-declarations.p4-stderr b/testdata/p4_16_errors_outputs/decl-matchkind-with-duplicate-fields-in-separate-declarations.p4-stderr new file mode 100644 index 00000000000..0459a47590b --- /dev/null +++ b/testdata/p4_16_errors_outputs/decl-matchkind-with-duplicate-fields-in-separate-declarations.p4-stderr @@ -0,0 +1,7 @@ +decl-matchkind-with-duplicate-fields-in-separate-declarations.p4(11): [--Werror=duplicate] error: foo: Duplicates declaration foo + foo + ^^^ +decl-matchkind-with-duplicate-fields-in-separate-declarations.p4(6) + foo, + ^^^ +[--Werror=overlimit] error: 1 errors encountered, aborting compilation diff --git a/testdata/p4_16_samples/pipe.p4 b/testdata/p4_16_samples/pipe.p4 index aa6f4b054f2..dca32456635 100644 --- a/testdata/p4_16_samples/pipe.p4 +++ b/testdata/p4_16_samples/pipe.p4 @@ -7,11 +7,6 @@ #include -match_kind { - ternary, - exact -} - typedef bit<9> BParamType; struct TArg1 { bit<9> field1; diff --git a/testdata/p4_16_samples_outputs/dash/dash-pipeline-pna-dpdk-first.p4 b/testdata/p4_16_samples_outputs/dash/dash-pipeline-pna-dpdk-first.p4 index 37ccca5dcbd..c40f3a3c3e0 100644 --- a/testdata/p4_16_samples_outputs/dash/dash-pipeline-pna-dpdk-first.p4 +++ b/testdata/p4_16_samples_outputs/dash/dash-pipeline-pna-dpdk-first.p4 @@ -4,6 +4,11 @@ error { InvalidIPv4Header } #include + +match_kind { + list, + range_list +} #include typedef bit<48> EthernetAddress; @@ -483,11 +488,6 @@ action tunnel_decap(inout headers_t hdr, inout metadata_t meta) { hdr.u0_udp.setInvalid(); meta.tunnel_pointer = 16w0; } -match_kind { - list, - range_list -} - control acl(inout headers_t hdr, inout metadata_t meta) { action permit() { } diff --git a/testdata/p4_16_samples_outputs/dash/dash-pipeline-pna-dpdk-frontend.p4 b/testdata/p4_16_samples_outputs/dash/dash-pipeline-pna-dpdk-frontend.p4 index 3c6efa0141b..bf90bc1bb99 100644 --- a/testdata/p4_16_samples_outputs/dash/dash-pipeline-pna-dpdk-frontend.p4 +++ b/testdata/p4_16_samples_outputs/dash/dash-pipeline-pna-dpdk-frontend.p4 @@ -4,6 +4,11 @@ error { InvalidIPv4Header } #include + +match_kind { + list, + range_list +} #include typedef bit<48> EthernetAddress; @@ -313,11 +318,6 @@ control dash_deparser(packet_out packet, in headers_t hdr, in metadata_t meta, i } } -match_kind { - list, - range_list -} - control dash_ingress(inout headers_t hdr, inout metadata_t meta, in pna_main_input_metadata_t istd, inout pna_main_output_metadata_t ostd) { @name("dash_ingress.meta") metadata_t meta_0; @name("dash_ingress.meta") metadata_t meta_5; diff --git a/testdata/p4_16_samples_outputs/dash/dash-pipeline-pna-dpdk-midend.p4 b/testdata/p4_16_samples_outputs/dash/dash-pipeline-pna-dpdk-midend.p4 index 1ddf240d6e3..0a13fe1e921 100644 --- a/testdata/p4_16_samples_outputs/dash/dash-pipeline-pna-dpdk-midend.p4 +++ b/testdata/p4_16_samples_outputs/dash/dash-pipeline-pna-dpdk-midend.p4 @@ -4,6 +4,11 @@ error { InvalidIPv4Header } #include + +match_kind { + list, + range_list +} #include header ethernet_t { @@ -312,11 +317,6 @@ control dash_deparser(packet_out packet, in headers_t hdr, in metadata_t meta, i } } -match_kind { - list, - range_list -} - control dash_ingress(inout headers_t hdr, inout metadata_t meta, in pna_main_input_metadata_t istd, inout pna_main_output_metadata_t ostd) { @name("dash_ingress.tmp_5") bit<32> tmp; @name("dash_ingress.tmp_6") bit<32> tmp_0; diff --git a/testdata/p4_16_samples_outputs/dash/dash-pipeline-pna-dpdk.p4 b/testdata/p4_16_samples_outputs/dash/dash-pipeline-pna-dpdk.p4 index 93c0db4ff6b..d99e4c73cba 100644 --- a/testdata/p4_16_samples_outputs/dash/dash-pipeline-pna-dpdk.p4 +++ b/testdata/p4_16_samples_outputs/dash/dash-pipeline-pna-dpdk.p4 @@ -4,6 +4,11 @@ error { InvalidIPv4Header } #include + +match_kind { + list, + range_list +} #include typedef bit<48> EthernetAddress; @@ -483,11 +488,6 @@ action tunnel_decap(inout headers_t hdr, inout metadata_t meta) { hdr.u0_udp.setInvalid(); meta.tunnel_pointer = 0; } -match_kind { - list, - range_list -} - control acl(inout headers_t hdr, inout metadata_t meta) { action permit() { } diff --git a/testdata/p4_16_samples_outputs/dash/dash-pipeline-v1model-bmv2-first.p4 b/testdata/p4_16_samples_outputs/dash/dash-pipeline-v1model-bmv2-first.p4 index dc7c0a10b51..884815a00a1 100644 --- a/testdata/p4_16_samples_outputs/dash/dash-pipeline-v1model-bmv2-first.p4 +++ b/testdata/p4_16_samples_outputs/dash/dash-pipeline-v1model-bmv2-first.p4 @@ -4,6 +4,11 @@ error { InvalidIPv4Header } #include + +match_kind { + list, + range_list +} #define V1MODEL_VERSION 20180101 #include @@ -456,11 +461,6 @@ action tunnel_decap(inout headers_t hdr, inout metadata_t meta) { hdr.u0_udp.setInvalid(); meta.tunnel_pointer = 16w0; } -match_kind { - list, - range_list -} - control acl(inout headers_t hdr, inout metadata_t meta) { action permit() { } diff --git a/testdata/p4_16_samples_outputs/dash/dash-pipeline-v1model-bmv2-frontend.p4 b/testdata/p4_16_samples_outputs/dash/dash-pipeline-v1model-bmv2-frontend.p4 index 017e771f85e..511ee08a193 100644 --- a/testdata/p4_16_samples_outputs/dash/dash-pipeline-v1model-bmv2-frontend.p4 +++ b/testdata/p4_16_samples_outputs/dash/dash-pipeline-v1model-bmv2-frontend.p4 @@ -4,6 +4,11 @@ error { InvalidIPv4Header } #include + +match_kind { + list, + range_list +} #define V1MODEL_VERSION 20180101 #include @@ -314,11 +319,6 @@ control dash_deparser(packet_out packet, in headers_t hdr) { } } -match_kind { - list, - range_list -} - control dash_ingress(inout headers_t hdr, inout metadata_t meta, inout standard_metadata_t standard_metadata) { @name("dash_ingress.meta") metadata_t meta_0; @name("dash_ingress.meta") metadata_t meta_5; diff --git a/testdata/p4_16_samples_outputs/dash/dash-pipeline-v1model-bmv2-midend.p4 b/testdata/p4_16_samples_outputs/dash/dash-pipeline-v1model-bmv2-midend.p4 index 0aa58195b74..e2edc70b695 100644 --- a/testdata/p4_16_samples_outputs/dash/dash-pipeline-v1model-bmv2-midend.p4 +++ b/testdata/p4_16_samples_outputs/dash/dash-pipeline-v1model-bmv2-midend.p4 @@ -4,6 +4,11 @@ error { InvalidIPv4Header } #include + +match_kind { + list, + range_list +} #define V1MODEL_VERSION 20180101 #include @@ -304,11 +309,6 @@ control dash_deparser(packet_out packet, in headers_t hdr) { } } -match_kind { - list, - range_list -} - control dash_ingress(inout headers_t hdr, inout metadata_t meta, inout standard_metadata_t standard_metadata) { @name("dash_ingress.tmp_7") bit<32> tmp; @name("dash_ingress.tmp_8") bit<32> tmp_0; diff --git a/testdata/p4_16_samples_outputs/dash/dash-pipeline-v1model-bmv2.p4 b/testdata/p4_16_samples_outputs/dash/dash-pipeline-v1model-bmv2.p4 index 4f50dd289db..27f2061d511 100644 --- a/testdata/p4_16_samples_outputs/dash/dash-pipeline-v1model-bmv2.p4 +++ b/testdata/p4_16_samples_outputs/dash/dash-pipeline-v1model-bmv2.p4 @@ -4,6 +4,11 @@ error { InvalidIPv4Header } #include + +match_kind { + list, + range_list +} #define V1MODEL_VERSION 20180101 #include @@ -456,11 +461,6 @@ action tunnel_decap(inout headers_t hdr, inout metadata_t meta) { hdr.u0_udp.setInvalid(); meta.tunnel_pointer = 0; } -match_kind { - list, - range_list -} - control acl(inout headers_t hdr, inout metadata_t meta) { action permit() { } diff --git a/testdata/p4_16_samples_outputs/issue982-first.p4 b/testdata/p4_16_samples_outputs/issue982-first.p4 index 1a45b4f7649..f9b2af66c2c 100644 --- a/testdata/p4_16_samples_outputs/issue982-first.p4 +++ b/testdata/p4_16_samples_outputs/issue982-first.p4 @@ -1,5 +1,10 @@ #include +match_kind { + range, + selector +} + header clone_0_t { bit<16> data; } @@ -95,11 +100,6 @@ struct psa_egress_output_metadata_t { PacketLength_t truncate_payload_bytes; } -match_kind { - range, - selector -} - action send_to_port(inout psa_ingress_output_metadata_t meta, in PortId_t egress_port) { meta.drop = false; meta.multicast_group = 10w0; diff --git a/testdata/p4_16_samples_outputs/issue982-frontend.p4 b/testdata/p4_16_samples_outputs/issue982-frontend.p4 index 1b195b25090..1f1de6d4971 100644 --- a/testdata/p4_16_samples_outputs/issue982-frontend.p4 +++ b/testdata/p4_16_samples_outputs/issue982-frontend.p4 @@ -1,5 +1,10 @@ #include +match_kind { + range, + selector +} + header clone_0_t { bit<16> data; } @@ -94,11 +99,6 @@ struct psa_egress_output_metadata_t { PacketLength_t truncate_payload_bytes; } -match_kind { - range, - selector -} - extern PacketReplicationEngine { } diff --git a/testdata/p4_16_samples_outputs/issue982-midend.p4 b/testdata/p4_16_samples_outputs/issue982-midend.p4 index 3e59c51752f..c5f2257e148 100644 --- a/testdata/p4_16_samples_outputs/issue982-midend.p4 +++ b/testdata/p4_16_samples_outputs/issue982-midend.p4 @@ -1,5 +1,10 @@ #include +match_kind { + range, + selector +} + header clone_0_t { bit<16> data; } @@ -82,11 +87,6 @@ struct psa_egress_output_metadata_t { bit<14> truncate_payload_bytes; } -match_kind { - range, - selector -} - extern PacketReplicationEngine { } diff --git a/testdata/p4_16_samples_outputs/issue982.p4 b/testdata/p4_16_samples_outputs/issue982.p4 index c892cbb5e2a..db68b735602 100644 --- a/testdata/p4_16_samples_outputs/issue982.p4 +++ b/testdata/p4_16_samples_outputs/issue982.p4 @@ -1,5 +1,10 @@ #include +match_kind { + range, + selector +} + header clone_0_t { bit<16> data; } @@ -95,11 +100,6 @@ struct psa_egress_output_metadata_t { PacketLength_t truncate_payload_bytes; } -match_kind { - range, - selector -} - action send_to_port(inout psa_ingress_output_metadata_t meta, in PortId_t egress_port) { meta.drop = false; meta.multicast_group = 0; diff --git a/testdata/p4_16_samples_outputs/pipe-first.p4 b/testdata/p4_16_samples_outputs/pipe-first.p4 index bf0bcd9fba4..663c15cbccf 100644 --- a/testdata/p4_16_samples_outputs/pipe-first.p4 +++ b/testdata/p4_16_samples_outputs/pipe-first.p4 @@ -1,9 +1,5 @@ #include -match_kind { - ternary, - exact -} typedef bit<9> BParamType; struct TArg1 { diff --git a/testdata/p4_16_samples_outputs/pipe-frontend.p4 b/testdata/p4_16_samples_outputs/pipe-frontend.p4 index f3ad80fed36..9bdaca620cb 100644 --- a/testdata/p4_16_samples_outputs/pipe-frontend.p4 +++ b/testdata/p4_16_samples_outputs/pipe-frontend.p4 @@ -1,9 +1,5 @@ #include -match_kind { - ternary, - exact -} typedef bit<9> BParamType; struct TArg1 { diff --git a/testdata/p4_16_samples_outputs/pipe-midend.p4 b/testdata/p4_16_samples_outputs/pipe-midend.p4 index fec9bde8b98..d0c03e51f21 100644 --- a/testdata/p4_16_samples_outputs/pipe-midend.p4 +++ b/testdata/p4_16_samples_outputs/pipe-midend.p4 @@ -1,9 +1,5 @@ #include -match_kind { - ternary, - exact -} struct TArg1 { bit<9> field1; diff --git a/testdata/p4_16_samples_outputs/pipe.p4 b/testdata/p4_16_samples_outputs/pipe.p4 index 5fa5028382f..23699ffe7f6 100644 --- a/testdata/p4_16_samples_outputs/pipe.p4 +++ b/testdata/p4_16_samples_outputs/pipe.p4 @@ -1,9 +1,5 @@ #include -match_kind { - ternary, - exact -} typedef bit<9> BParamType; struct TArg1 { diff --git a/testdata/p4_16_samples_outputs/pipe.p4-stderr b/testdata/p4_16_samples_outputs/pipe.p4-stderr index dad86c60de4..9511878720c 100644 --- a/testdata/p4_16_samples_outputs/pipe.p4-stderr +++ b/testdata/p4_16_samples_outputs/pipe.p4-stderr @@ -1,3 +1,3 @@ -pipe.p4(30): [--Wwarn=unused] warning: 'PArg2' is unused +pipe.p4(25): [--Wwarn=unused] warning: 'PArg2' is unused typedef bit<32> PArg2; ^^^^^