diff --git a/frontends/p4/def_use.cpp b/frontends/p4/def_use.cpp index 723cd8d7bd9..4b19093b094 100644 --- a/frontends/p4/def_use.cpp +++ b/frontends/p4/def_use.cpp @@ -390,7 +390,7 @@ void ComputeWriteSet::enterScope(const IR::ParameterList* parameters, } definitions->set(entryPoint, defs); currentDefinitions = defs; - LOG1("Definitions at " << entryPoint << ":" << currentDefinitions); + LOG3("Definitions at " << entryPoint << ":" << currentDefinitions); } void ComputeWriteSet::exitScope(const IR::ParameterList* parameters, @@ -601,7 +601,7 @@ bool ComputeWriteSet::preorder(const IR::Operation_Unary* expression) { } bool ComputeWriteSet::preorder(const IR::MethodCallExpression* expression) { - LOG2("CWS Visiting " << dbp(expression)); + LOG3("CWS Visiting " << dbp(expression)); bool save = lhs; lhs = true; // The method call may modify the object, which is part of the method @@ -647,7 +647,7 @@ bool ComputeWriteSet::preorder(const IR::MethodCallExpression* expression) { } if (callee != nullptr) { - LOG1("Analyzing " << dbp(callee)); + LOG3("Analyzing " << dbp(callee)); ProgramPoint pt(callingContext, expression); ComputeWriteSet cw(this, pt, currentDefinitions); (void)callee->apply(cw); @@ -687,7 +687,7 @@ bool ComputeWriteSet::preorder(const IR::ParserState* state) { // Symbolic execution of the parser bool ComputeWriteSet::preorder(const IR::P4Parser* parser) { - LOG1("CWS Visiting " << dbp(parser)); + LOG3("CWS Visiting " << dbp(parser)); auto startState = parser->getDeclByName(IR::ParserState::start)->to(); auto startPoint = ProgramPoint(startState); enterScope(parser->type->applyParams, parser->parserLocals, startPoint); @@ -706,7 +706,7 @@ bool ComputeWriteSet::preorder(const IR::P4Parser* parser) { while (!toRun.empty()) { auto state = *toRun.begin(); toRun.erase(state); - LOG1("Traversing " << dbp(state)); + LOG3("Traversing " << dbp(state)); // We need a new visitor to visit the state, // but we use the same data structures @@ -733,7 +733,7 @@ bool ComputeWriteSet::preorder(const IR::P4Parser* parser) { } bool ComputeWriteSet::preorder(const IR::P4Control* control) { - LOG1("CWS Visiting " << dbp(control)); + LOG3("CWS Visiting " << dbp(control)); auto startPoint = ProgramPoint(control); enterScope(control->type->applyParams, control->controlLocals, startPoint); exitDefinitions = new Definitions(); @@ -787,7 +787,7 @@ bool ComputeWriteSet::preorder(const IR::EmptyStatement*) { } bool ComputeWriteSet::preorder(const IR::AssignmentStatement* statement) { - LOG1("CWS Visiting " << dbp(statement) << " " << statement); + LOG3("CWS Visiting " << dbp(statement) << " " << statement); lhs = true; visit(statement->left); lhs = false; @@ -801,7 +801,7 @@ bool ComputeWriteSet::preorder(const IR::AssignmentStatement* statement) { } bool ComputeWriteSet::preorder(const IR::SwitchStatement* statement) { - LOG1("CWS Visiting " << dbp(statement)); + LOG3("CWS Visiting " << dbp(statement)); visit(statement->expression); auto locs = get(statement->expression); auto defs = currentDefinitions->writes(getProgramPoint(), locs); @@ -828,7 +828,10 @@ bool ComputeWriteSet::preorder(const IR::SwitchStatement* statement) { } bool ComputeWriteSet::preorder(const IR::P4Action* action) { - LOG1("CWS Visiting " << dbp(action)); + LOG3("CWS Visiting " << dbp(action)); + auto saveReturned = returnedDefinitions; + returnedDefinitions = new Definitions(); + auto decls = new IR::IndexedVector(); // We assume that there are no declarations in inner scopes // inside the action body. @@ -840,6 +843,8 @@ bool ComputeWriteSet::preorder(const IR::P4Action* action) { enterScope(action->parameters, decls, pt, false); visit(action->body); exitScope(action->parameters, decls); + currentDefinitions = currentDefinitions->join(returnedDefinitions); + returnedDefinitions = saveReturned; return false; } @@ -860,7 +865,7 @@ class GetDeclarations : public Inspector { } // namespace bool ComputeWriteSet::preorder(const IR::Function* function) { - LOG1("CWS Visiting " << dbp(function)); + LOG3("CWS Visiting " << dbp(function)); auto point = ProgramPoint(function); auto locals = GetDeclarations::get(function->body); auto saveReturned = returnedDefinitions; @@ -873,12 +878,12 @@ bool ComputeWriteSet::preorder(const IR::Function* function) { exitScope(function->type->parameters, locals); returnedDefinitions = saveReturned; - LOG1("Done " << dbp(function)); + LOG3("Done " << dbp(function)); return false; } bool ComputeWriteSet::preorder(const IR::P4Table* table) { - LOG1("CWS Visiting " << dbp(table)); + LOG3("CWS Visiting " << dbp(table)); ProgramPoint pt(callingContext, table); enterScope(nullptr, nullptr, pt, false); diff --git a/midend/removeReturns.cpp b/midend/removeReturns.cpp index 5d3e60f66b6..dbf0f9a1d2b 100644 --- a/midend/removeReturns.cpp +++ b/midend/removeReturns.cpp @@ -20,6 +20,14 @@ limitations under the License. namespace P4 { namespace { +/** +This inspector detects whether an IR tree contains +'return' or 'exit' statements. +It sets a boolean flag for each of them. + +It treats exceptionally Functions - it claims that +returns do not exist in Functions. +*/ class HasExits : public Inspector { public: bool hasExits; @@ -43,6 +51,7 @@ const IR::Node* DoRemoveReturns::preorder(IR::P4Action* action) { prune(); return action; } + LOG3("Processing " << dbp(action)); cstring var = refMap->newName(variableName); returnVar = IR::ID(var, nullptr); auto f = new IR::BoolLiteral(Util::SourceInfo(), false); @@ -65,6 +74,8 @@ const IR::Node* DoRemoveReturns::preorder(IR::P4Action* action) { } const IR::Node* DoRemoveReturns::preorder(IR::P4Control* control) { + visit(control->controlLocals); + HasExits he; (void)control->body->apply(he); if (!he.hasReturns) { diff --git a/midend/removeReturns.h b/midend/removeReturns.h index 39a18c39cb2..37f9c6b45af 100644 --- a/midend/removeReturns.h +++ b/midend/removeReturns.h @@ -23,21 +23,23 @@ limitations under the License. namespace P4 { -// This replaces 'returns' by ifs: -// e.g. -// control c(inout bit x) { apply { -// if (x) return; -// x = !x; -// }} -// becomes: -// control c(inout bit x) { -// bool hasReturned; -// apply { -// hasReturned = false; -// if (x) hasReturned = true; -// if (!hasReturned) -// x = !x; -// }} +/** +This visitor replaces 'returns' by ifs: +e.g. +control c(inout bit x) { apply { + if (x) return; + x = !x; +}} +becomes: +control c(inout bit x) { + bool hasReturned; + apply { + hasReturned = false; + if (x) hasReturned = true; + if (!hasReturned) + x = !x; +}} +*/ class DoRemoveReturns : public Transform { protected: P4::ReferenceMap* refMap; @@ -75,13 +77,16 @@ class DoRemoveReturns : public Transform { { prune(); return parser; } }; -// This removes "exit" calls. It is significantly more involved than return removal, -// since an exit in an action causes the calling control to terminate. -// This pass assumes that each statement in a control block can -// exit only once - so it should be run after a pass that enforces this. -// (E.g., it does not handle: -// if (t1.apply().hit && t2.apply().hit) { ... } -// It also assumes that there are no global actions and that action calls have been inlined. +/** +This visitor removes "exit" calls. It is significantly more involved than return removal, +since an exit in an action causes the calling control to terminate. +This pass assumes that each statement in a control block can +exit only once -- so it should be run after a pass that enforces this, +e.g., SideEffectOrdering. +(E.g., it does not handle: +if (t1.apply().hit && t2.apply().hit) { ... } +It also assumes that there are no global actions and that action calls have been inlined. +*/ class DoRemoveExits : public DoRemoveReturns { TypeMap* typeMap; // In this class "Return" (inherited from RemoveReturns) should be read as "Exit" diff --git a/testdata/p4_16_samples/issue420.p4 b/testdata/p4_16_samples/issue420.p4 new file mode 100644 index 00000000000..7d1016fd111 --- /dev/null +++ b/testdata/p4_16_samples/issue420.p4 @@ -0,0 +1,87 @@ +#include +#include + +typedef bit<48> EthernetAddress; + +header Ethernet_h { + EthernetAddress dstAddr; + EthernetAddress srcAddr; + bit<16> etherType; +} + +struct Parsed_packet { + Ethernet_h ethernet; +} + +struct mystruct1 { + bit<4> a; + bit<4> b; +} + +control DeparserI(packet_out packet, + in Parsed_packet hdr) { + apply { packet.emit(hdr.ethernet); } +} + +parser parserI(packet_in pkt, + out Parsed_packet hdr, + inout mystruct1 meta, + inout standard_metadata_t stdmeta) { + state start { + pkt.extract(hdr.ethernet); + transition accept; + } +} + +control cIngress(inout Parsed_packet hdr, + inout mystruct1 meta, + inout standard_metadata_t stdmeta) { + action foo (bit<16> bar) { + if (bar == 16w0xf00d) { + hdr.ethernet.srcAddr = 48w0xdeadbeeff00d; + // tested with 2017-Mar-30 p4test and p4c-bm2-ss + + // With following line, there is crash. + + // Commented out, there is no crash. There is a warning + // from p4c-bm2-ss that it is removing unused action + // parameter 'bar' for compatibility reasons, but that + // seems wrong, given that the value of bar must be used + // to implement the action's behavior correctly. + return; + } + hdr.ethernet.srcAddr = ~48w0xdeadbeeff00d; + } + table tbl1 { + key = { } + actions = { foo; NoAction; } + default_action = NoAction; + } + apply { + tbl1.apply(); + return; + } +} + +control cEgress(inout Parsed_packet hdr, + inout mystruct1 meta, + inout standard_metadata_t stdmeta) { + apply { } +} + +control vc(in Parsed_packet hdr, + inout mystruct1 meta) { + apply { } +} + +control uc(inout Parsed_packet hdr, + inout mystruct1 meta) { + apply { } +} + +V1Switch(parserI(), + vc(), + cIngress(), + cEgress(), + uc(), + DeparserI()) main; diff --git a/testdata/p4_16_samples_outputs/issue420-first.p4 b/testdata/p4_16_samples_outputs/issue420-first.p4 new file mode 100644 index 00000000000..e29b7679265 --- /dev/null +++ b/testdata/p4_16_samples_outputs/issue420-first.p4 @@ -0,0 +1,71 @@ +#include +#include + +typedef bit<48> EthernetAddress; +header Ethernet_h { + EthernetAddress dstAddr; + EthernetAddress srcAddr; + bit<16> etherType; +} + +struct Parsed_packet { + Ethernet_h ethernet; +} + +struct mystruct1 { + bit<4> a; + bit<4> b; +} + +control DeparserI(packet_out packet, in Parsed_packet hdr) { + apply { + packet.emit(hdr.ethernet); + } +} + +parser parserI(packet_in pkt, out Parsed_packet hdr, inout mystruct1 meta, inout standard_metadata_t stdmeta) { + state start { + pkt.extract(hdr.ethernet); + transition accept; + } +} + +control cIngress(inout Parsed_packet hdr, inout mystruct1 meta, inout standard_metadata_t stdmeta) { + action foo(bit<16> bar) { + if (bar == 16w0xf00d) { + hdr.ethernet.srcAddr = 48w0xdeadbeeff00d; + return; + } + hdr.ethernet.srcAddr = 48w0x215241100ff2; + } + table tbl1 { + key = { + } + actions = { + foo(); + NoAction(); + } + default_action = NoAction(); + } + apply { + tbl1.apply(); + return; + } +} + +control cEgress(inout Parsed_packet hdr, inout mystruct1 meta, inout standard_metadata_t stdmeta) { + apply { + } +} + +control vc(in Parsed_packet hdr, inout mystruct1 meta) { + apply { + } +} + +control uc(inout Parsed_packet hdr, inout mystruct1 meta) { + apply { + } +} + +V1Switch(parserI(), vc(), cIngress(), cEgress(), uc(), DeparserI()) main; diff --git a/testdata/p4_16_samples_outputs/issue420-frontend.p4 b/testdata/p4_16_samples_outputs/issue420-frontend.p4 new file mode 100644 index 00000000000..99b460adc3c --- /dev/null +++ b/testdata/p4_16_samples_outputs/issue420-frontend.p4 @@ -0,0 +1,71 @@ +#include +#include + +typedef bit<48> EthernetAddress; +header Ethernet_h { + EthernetAddress dstAddr; + EthernetAddress srcAddr; + bit<16> etherType; +} + +struct Parsed_packet { + Ethernet_h ethernet; +} + +struct mystruct1 { + bit<4> a; + bit<4> b; +} + +control DeparserI(packet_out packet, in Parsed_packet hdr) { + apply { + packet.emit(hdr.ethernet); + } +} + +parser parserI(packet_in pkt, out Parsed_packet hdr, inout mystruct1 meta, inout standard_metadata_t stdmeta) { + state start { + pkt.extract(hdr.ethernet); + transition accept; + } +} + +control cIngress(inout Parsed_packet hdr, inout mystruct1 meta, inout standard_metadata_t stdmeta) { + @name("foo") action foo_0(bit<16> bar) { + if (bar == 16w0xf00d) { + hdr.ethernet.srcAddr = 48w0xdeadbeeff00d; + return; + } + hdr.ethernet.srcAddr = 48w0x215241100ff2; + } + @name("tbl1") table tbl1_0 { + key = { + } + actions = { + foo_0(); + NoAction(); + } + default_action = NoAction(); + } + apply { + tbl1_0.apply(); + return; + } +} + +control cEgress(inout Parsed_packet hdr, inout mystruct1 meta, inout standard_metadata_t stdmeta) { + apply { + } +} + +control vc(in Parsed_packet hdr, inout mystruct1 meta) { + apply { + } +} + +control uc(inout Parsed_packet hdr, inout mystruct1 meta) { + apply { + } +} + +V1Switch(parserI(), vc(), cIngress(), cEgress(), uc(), DeparserI()) main; diff --git a/testdata/p4_16_samples_outputs/issue420-midend.p4 b/testdata/p4_16_samples_outputs/issue420-midend.p4 new file mode 100644 index 00000000000..20b5bf2d249 --- /dev/null +++ b/testdata/p4_16_samples_outputs/issue420-midend.p4 @@ -0,0 +1,69 @@ +#include +#include + +typedef bit<48> EthernetAddress; +header Ethernet_h { + EthernetAddress dstAddr; + EthernetAddress srcAddr; + bit<16> etherType; +} + +struct Parsed_packet { + Ethernet_h ethernet; +} + +struct mystruct1 { + bit<4> a; + bit<4> b; +} + +control DeparserI(packet_out packet, in Parsed_packet hdr) { + apply { + packet.emit(hdr.ethernet); + } +} + +parser parserI(packet_in pkt, out Parsed_packet hdr, inout mystruct1 meta, inout standard_metadata_t stdmeta) { + state start { + pkt.extract(hdr.ethernet); + transition accept; + } +} + +control cIngress(inout Parsed_packet hdr, inout mystruct1 meta, inout standard_metadata_t stdmeta) { + @name("NoAction") action NoAction_0() { + } + @name("foo") action foo_0(bit<16> bar) { + hdr.ethernet.srcAddr = (bar == 16w0xf00d ? 48w0xdeadbeeff00d : hdr.ethernet.srcAddr); + hdr.ethernet.srcAddr = (!(bar == 16w0xf00d ? true : false) ? 48w0x215241100ff2 : hdr.ethernet.srcAddr); + } + @name("tbl1") table tbl1 { + key = { + } + actions = { + foo_0(); + NoAction_0(); + } + default_action = NoAction_0(); + } + apply { + tbl1.apply(); + } +} + +control cEgress(inout Parsed_packet hdr, inout mystruct1 meta, inout standard_metadata_t stdmeta) { + apply { + } +} + +control vc(in Parsed_packet hdr, inout mystruct1 meta) { + apply { + } +} + +control uc(inout Parsed_packet hdr, inout mystruct1 meta) { + apply { + } +} + +V1Switch(parserI(), vc(), cIngress(), cEgress(), uc(), DeparserI()) main; diff --git a/testdata/p4_16_samples_outputs/issue420.p4 b/testdata/p4_16_samples_outputs/issue420.p4 new file mode 100644 index 00000000000..9ec5e51cdfd --- /dev/null +++ b/testdata/p4_16_samples_outputs/issue420.p4 @@ -0,0 +1,71 @@ +#include +#include + +typedef bit<48> EthernetAddress; +header Ethernet_h { + EthernetAddress dstAddr; + EthernetAddress srcAddr; + bit<16> etherType; +} + +struct Parsed_packet { + Ethernet_h ethernet; +} + +struct mystruct1 { + bit<4> a; + bit<4> b; +} + +control DeparserI(packet_out packet, in Parsed_packet hdr) { + apply { + packet.emit(hdr.ethernet); + } +} + +parser parserI(packet_in pkt, out Parsed_packet hdr, inout mystruct1 meta, inout standard_metadata_t stdmeta) { + state start { + pkt.extract(hdr.ethernet); + transition accept; + } +} + +control cIngress(inout Parsed_packet hdr, inout mystruct1 meta, inout standard_metadata_t stdmeta) { + action foo(bit<16> bar) { + if (bar == 16w0xf00d) { + hdr.ethernet.srcAddr = 48w0xdeadbeeff00d; + return; + } + hdr.ethernet.srcAddr = ~48w0xdeadbeeff00d; + } + table tbl1 { + key = { + } + actions = { + foo; + NoAction; + } + default_action = NoAction; + } + apply { + tbl1.apply(); + return; + } +} + +control cEgress(inout Parsed_packet hdr, inout mystruct1 meta, inout standard_metadata_t stdmeta) { + apply { + } +} + +control vc(in Parsed_packet hdr, inout mystruct1 meta) { + apply { + } +} + +control uc(inout Parsed_packet hdr, inout mystruct1 meta) { + apply { + } +} + +V1Switch(parserI(), vc(), cIngress(), cEgress(), uc(), DeparserI()) main; diff --git a/testdata/p4_16_samples_outputs/issue420.p4-stderr b/testdata/p4_16_samples_outputs/issue420.p4-stderr new file mode 100644 index 00000000000..e69de29bb2d