Fix for issue #435

frontends/p4/validateParsedProgram.cpp

@@ -18,6 +18,7 @@ limitations under the License.
 
 namespace P4 {
 
+/// Check that the type of a constant is either bit<>, int<> or int
 void ValidateParsedProgram::postorder(const IR::Constant* c) {
     if (c->type != nullptr &&
         !c->type->is<IR::Type_Unknown>() &&
@@ -26,44 +27,65 @@ void ValidateParsedProgram::postorder(const IR::Constant* c) {
         BUG("Invalid type %1% for constant %2%", c->type, c);
 }
 
+/// Check that underscore is not a method name
+/// Check that constructors do not have a return type
+/// Check that extern constructor names match the enclosing extern
 void ValidateParsedProgram::postorder(const IR::Method* m) {
     if (m->name.isDontCare())
         ::error("%1%: Illegal method/function name", m->name);
     if (auto ext = findContext<IR::Type_Extern>()) {
         if (m->name == ext->name && m->type->returnType != nullptr)
             ::error("%1%: Constructor cannot have a return type", m);
+        if (m->type->returnType == nullptr) {
+            if (m->name != ext->name) {
+                ::error("%1%: Method has no return type", m);
+                return;
+            }
+            for (auto p : *m->type->parameters)
+                if (p->direction != IR::Direction::None)
+                    ::error("%1%: constructor parameters cannot have a direction", p);
+        }
     }
 }
 
+/// Struct field names cannot be underscore
 void ValidateParsedProgram::postorder(const IR::StructField* f) {
     if (f->name.isDontCare())
         ::error("%1%: Illegal field name", f->name);
 }
 
+/// Unions must have at least one field
 void ValidateParsedProgram::postorder(const IR::Type_Union* type) {
     if (type->fields->size() == 0)
         ::error("%1%: empty union", type);
 }
 
+/// Width of a bit<> type is at least 0
+/// Width of an int<> type is at least 1
 void ValidateParsedProgram::postorder(const IR::Type_Bits* type) {
     if (type->size <= 0)
         ::error("%1%: Illegal bit size", type);
     if (type->size == 1 && type->isSigned)
         ::error("%1%: Signed types cannot be 1-bit wide", type);
 }
 
+/// The accept and reject states cannot be implemented
 void ValidateParsedProgram::postorder(const IR::ParserState* s) {
     if (s->name == IR::ParserState::accept ||
         s->name == IR::ParserState::reject)
         ::error("%1%: parser state should not be implemented, it is built-in", s->name);
 }
 
+/// All parameters of a constructor must be directionless.
+/// This only checks controls, parsers and packages
 void ValidateParsedProgram::container(const IR::IContainer* type) {
     for (auto p : *type->getConstructorParameters()->parameters)
         if (p->direction != IR::Direction::None)
             ::error("%1%: constructor parameters cannot have a direction", p);
 }
 
+/// Tables must have an 'actions' and a 'default_action' properties.
+/// The latter is just a warning.
 void ValidateParsedProgram::postorder(const IR::P4Table* t) {
     auto ac = t->getActionList();
     if (ac == nullptr)
@@ -75,6 +97,8 @@ void ValidateParsedProgram::postorder(const IR::P4Table* t) {
                 t->name, IR::TableProperties::defaultActionPropertyName);
 }
 
+/// Checks that the names of the three parameter lists for some constructs
+/// are all distinct (type parameters, apply parameters, constructor parameters)
 void ValidateParsedProgram::distinctParameters(
     const IR::TypeParameters* typeParams,
     const IR::ParameterList* apply,
@@ -99,17 +123,21 @@ void ValidateParsedProgram::distinctParameters(
     }
 }
 
+/// Cannot invoke constructors in actions
 void ValidateParsedProgram::postorder(const IR::ConstructorCallExpression* expression) {
     auto inAction = findContext<IR::P4Action>();
     if (inAction != nullptr)
         ::error("%1%: Constructor calls not allowed in actions", expression);
 }
 
+/// Variable names cannot be underscore
 void ValidateParsedProgram::postorder(const IR::Declaration_Variable* decl) {
     if (decl->name.isDontCare())
         ::error("%1%: illegal variable name", decl);
 }
 
+/// Instance names cannot be don't care
+/// Do not declare instances in apply {} blocks, parser states or actions
 void ValidateParsedProgram::postorder(const IR::Declaration_Instance* decl) {
     if (decl->name.isDontCare())
         ::error("%1%: illegal instance name", decl);
@@ -125,10 +153,12 @@ void ValidateParsedProgram::postorder(const IR::Declaration_Instance* decl) {
         ::error("%1%: Instantiations not allowed in actions", decl);
 }
 
+/// Constant names cannot be underscore
 void ValidateParsedProgram::postorder(const IR::Declaration_Constant* decl) {
     if (decl->name.isDontCare())
         ::error("%1%: illegal constant name", decl);
 }
+
 /**
   * check that an entries list is declared as constant
   * The current specification supports only const entries, and we chose
@@ -145,19 +175,21 @@ void ValidateParsedProgram::postorder(const IR::EntriesList* l) {
         ::error("%1%: table initializers must be constant", l);
 }
 
-
+/// Switch statements are not allowed in actions
 void ValidateParsedProgram::postorder(const IR::SwitchStatement* statement) {
     auto inAction = findContext<IR::P4Action>();
     if (inAction != nullptr)
         ::error("%1%: switch statements not allowed in actions", statement);
 }
 
+/// Return statements are not allowed in parsers
 void ValidateParsedProgram::postorder(const IR::ReturnStatement* statement) {
     auto inParser = findContext<IR::P4Parser>();
     if (inParser != nullptr)
         ::error("%1%: return statements not allowed in parsers", statement);
 }
 
+/// Exit statements are not allowed in parsers
 void ValidateParsedProgram::postorder(const IR::ExitStatement* statement) {
     auto inParser = findContext<IR::P4Parser>();
     if (inParser != nullptr)

frontends/p4/validateParsedProgram.h

@@ -21,9 +21,14 @@ limitations under the License.
 
 namespace P4 {
 
-/*
-   Run immediately after parsing. There is no type information at this
-   point, so we do only simple checks.
+/**
+   This pass performs some simple semantic checks on the program;
+   since the grammar accepts many programs that are actually illegal,
+   this pass does some additional validation.
+
+   This pass is run immediately after parsing. There is no type
+   information at this point, so it only does simple checks.
+
    - integer constants have valid types
    - don't care _ is not used as a name for methods, fields, variables, instances
    - unions have at least one field
@@ -32,11 +37,11 @@ namespace P4 {
    - no parser state is named 'accept' or 'reject'
    - constructor parameters are direction-less
    - tables have an actions and a default_action properties
-   - table parameters are never directionless
    - instantiations appear at the top-level only
    - switch statements do not occur in actions
    - instantiations do not occur in actions
    - constructors are not invoked in actions
+   - extern constructors have the same name as the enclosing extern
  */
 class ValidateParsedProgram final : public Inspector {
     bool isv1;

testdata/p4_16_errors/issue435.p4

@@ -0,0 +1,20 @@
+struct mystruct1 {
+    bit<4>  a;
+}
+
+extern myExtern1 {
+    myExtern1(bit x);
+    mystruct1(in bit<8> a, out bit<16> b);
+}
+
+control c() {
+    myExtern1(1) m;
+    apply {}
+}
+
+#if 0
+control ct();
+package top(ct c);
+
+top(c()) main;
+#endif

testdata/p4_16_errors_outputs/issue435.p4

@@ -0,0 +1,15 @@
+struct mystruct1 {
+    bit<4> a;
+}
+
+extern myExtern1 {
+    myExtern1(bit<1> x);
+    mystruct1(in bit<8> a, out bit<16> b);
+}
+
+control c() {
+    myExtern1(1) m;
+    apply {
+    }
+}
+

testdata/p4_16_errors_outputs/issue435.p4-stderr

@@ -0,0 +1,3 @@
+../testdata/p4_16_errors/issue435.p4(7): error: mystruct1: Method has no return type
+    mystruct1(in bit<8> a, out bit<16> b);
+    ^^^^^^^^^

testdata/p4_16_samples/interface.p4

@@ -18,7 +18,7 @@ limitations under the License.
 
 extern Crc16 <T> {
         Crc16();
-        Crc16(in int<32> x);
+        Crc16(int<32> x);
         void initialize<U>(in U input_data);
         T value();
         T make_index(

testdata/p4_16_samples_outputs/interface-first.p4

@@ -2,7 +2,7 @@
 
 extern Crc16<T> {
     Crc16();
-    Crc16(in int<32> x);
+    Crc16(int<32> x);
     void initialize<U>(in U input_data);
     T value();
     T make_index(in T size, in T base);

testdata/p4_16_samples_outputs/interface-frontend.p4

@@ -2,7 +2,7 @@
 
 extern Crc16<T> {
     Crc16();
-    Crc16(in int<32> x);
+    Crc16(int<32> x);
     void initialize<U>(in U input_data);
     T value();
     T make_index(in T size, in T base);

testdata/p4_16_samples_outputs/interface-midend.p4

@@ -2,7 +2,7 @@
 
 extern Crc16<T> {
     Crc16();
-    Crc16(in int<32> x);
+    Crc16(int<32> x);
     void initialize<U>(in U input_data);
     T value();
     T make_index(in T size, in T base);

testdata/p4_16_samples_outputs/interface.p4

@@ -2,7 +2,7 @@
 
 extern Crc16<T> {
     Crc16();
-    Crc16(in int<32> x);
+    Crc16(int<32> x);
     void initialize<U>(in U input_data);
     T value();
     T make_index(in T size, in T base);

testdata/p4_16_samples_outputs/table-entries-exact-bmv2-midend.p4

@@ -0,0 +1,74 @@
+#include <core.p4>
+#include <v1model.p4>
+
+header hdr {
+    bit<8>  e;
+    bit<16> t;
+    bit<8>  l;
+    bit<8>  r;
+    bit<1>  v;
+}
+
+struct Header_t {
+    hdr h;
+}
+
+struct Meta_t {
+}
+
+parser p(packet_in b, out Header_t h, inout Meta_t m, inout standard_metadata_t sm) {
+    state start {
+        b.extract<hdr>(h.h);
+        transition accept;
+    }
+}
+
+control vrfy(in Header_t h, inout Meta_t m) {
+    apply {
+    }
+}
+
+control update(inout Header_t h, inout Meta_t m) {
+    apply {
+    }
+}
+
+control egress(inout Header_t h, inout Meta_t m, inout standard_metadata_t sm) {
+    apply {
+    }
+}
+
+control deparser(packet_out b, in Header_t h) {
+    apply {
+        b.emit<hdr>(h.h);
+    }
+}
+
+control ingress(inout Header_t h, inout Meta_t m, inout standard_metadata_t standard_meta) {
+    @name("a") action a_0() {
+        standard_meta.egress_spec = 9w0;
+    }
+    @name("a_with_control_params") action a_with_control_params_0(bit<9> x) {
+        standard_meta.egress_spec = x;
+    }
+    @name("t_exact") table t_exact {
+        key = {
+            h.h.e: exact @name("h.h.e") ;
+        }
+        actions = {
+            a_0();
+            a_with_control_params_0();
+        }
+        default_action = a_0();
+        const entries = {
+            8w0x1 : a_with_control_params_0(9w1);
+            8w0x2 : a_with_control_params_0(9w2);
+        }
+
+    }
+    apply {
+        t_exact.apply();
+    }
+}
+
+V1Switch<Header_t, Meta_t>(p(), vrfy(), ingress(), egress(), update(), deparser()) main;