Fix for issue #420

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<IR::ParserState>();
     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<IR::Declaration>();
     // 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);
 

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) {

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"

testdata/p4_16_samples/issue420.p4

@@ -0,0 +1,87 @@
+#include <core.p4>
+#include <v1model.p4>
+
+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<Parsed_packet, mystruct1>(parserI(),
+                                   vc(),
+                                   cIngress(),
+                                   cEgress(),
+                                   uc(),
+                                   DeparserI()) main;

testdata/p4_16_samples_outputs/issue420-first.p4

@@ -0,0 +1,71 @@
+#include <core.p4>
+#include <v1model.p4>
+
+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<Ethernet_h>(hdr.ethernet);
+    }
+}
+
+parser parserI(packet_in pkt, out Parsed_packet hdr, inout mystruct1 meta, inout standard_metadata_t stdmeta) {
+    state start {
+        pkt.extract<Ethernet_h>(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<Parsed_packet, mystruct1>(parserI(), vc(), cIngress(), cEgress(), uc(), DeparserI()) main;