Fixes for issues #477 and #478

backends/bmv2/Makefile.am

@@ -48,3 +48,8 @@ bmv2tests.mk: $(GENTESTS) $(srcdir)/%reldir%/Makefile.am \
 	      $(srcdir)/testdata/p4_14_samples/switch_*/switch.p4 \
 	      $(srcdir)/testdata/p4_16_samples $(srcdir)/testdata/p4_14_samples
 	@$(GENTESTS) $(srcdir) bmv2 $(srcdir)/backends/bmv2/run-bmv2-test.py $^ >$@
+
+# These are issue #481
+XFAIL_TESTS += \
+   bmv2/testdata/p4_14_samples/09-IPv4OptionsUnparsed.p4.test \
+   bmv2/testdata/p4_14_samples/TLV_parsing.p4.test

backends/p4test/Makefile.am

@@ -49,6 +49,11 @@ p14_to_16tests.mk: $(GENTESTS) $(srcdir)/%reldir%/Makefile.am \
 	@$(GENTESTS) $(srcdir) p14_to_16 $(srcdir)/backends/p4test/run-p4-sample.py $^ >$@
 
 
-# This is issue #13
+# First is issue #13
+# Second one is issue #447
+# Next ones are all issue #481
 XFAIL_TESTS += \
-    p4/testdata/p4_16_samples/cast-call.p4.test
+    p4/testdata/p4_16_samples/cast-call.p4.test \
+    p4/testdata/p4_16_samples/spec-ex32.p4.test \
+    p14_to_16/testdata/p4_14_samples/09-IPv4OptionsUnparsed.p4.test \
+    p14_to_16/testdata/p4_14_samples/TLV_parsing.p4.test

frontends/Makefile.am

@@ -108,7 +108,6 @@ cpplint_FILES += frontends/p4/p4-lex.l \
 		 $(p4_frontend_UNIFIED) \
 		 $(p4_frontend_NONUNIFIED)
 
-
 common_frontend_UNIFIED = \
 	frontends/common/options.cpp \
 	frontends/common/constantFolding.cpp \

frontends/p4/frontend.cpp

@@ -50,8 +50,13 @@ limitations under the License.
 namespace P4 {
 
 namespace {
+/**
+This pass outputs the program as a P4 source file.
+*/
 class PrettyPrint : public Inspector {
+    /// output file
     cstring ppfile;
+    /// The file that is being compiled.  This used
     cstring inputfile;
  public:
     explicit PrettyPrint(const CompilerOptions& options) {
@@ -71,7 +76,9 @@ class PrettyPrint : public Inspector {
 };
 }  // namespace
 
-// This pass does nothing, it's just here to mark the end of the front-end
+/**
+This pass does nothing, it's just here to mark the end of the front-end
+*/
 class FrontEndLast : public PassManager {
  public:
     FrontEndLast() { setName("FrontEndLast"); }

frontends/p4/methodInstance.h

@@ -25,7 +25,21 @@ limitations under the License.
 
 namespace P4 {
 
-// Represents compile-time information about a MethodCallExpression
+/**
+This class is very useful for extracting information out of
+MethodCallExpressions.  Since there are no function pointers in P4,
+methods can completely be resolved at compilation time.  The static
+method 'resolve' will categorize each method call into one of several
+kinds:
+- apply method, could be of a table, control or parser
+- extern function
+- extern method (method of an extern object)
+- action call
+- built-in method (there are five of these: setValid, setInvalid, isValid, push, pop
+
+See also the ConstructorCall class and the MethodCallDescription class
+below.
+*/
 class MethodInstance {
  protected:
     MethodInstance(const IR::MethodCallExpression* mce,
@@ -38,16 +52,22 @@ class MethodInstance {
 
  public:
     const IR::MethodCallExpression* expr;
-    const IR::IDeclaration* object;  // Declaration of object that method is applied to.
-                                     // May be null for plain functions.
-    const IR::Type_MethodBase* originalMethodType;  // the type of the *original* called method,
-                                                    // without instantiated type parameters.
-    const IR::Type_MethodBase* actualMethodType;  // type of called method,
-                                                  // with instantiated type parameters.
+    /** Declaration of object that method is applied to.
+        May be null for plain functions. */
+    const IR::IDeclaration* object;
+    /** The type of the *original* called method,
+        without instantiated type parameters. */
+    const IR::Type_MethodBase* originalMethodType;
+    /** Type of called method,
+        with instantiated type parameters. */
+    const IR::Type_MethodBase* actualMethodType;
 
     virtual bool isApply() const { return false; }
     virtual ~MethodInstance() {}
 
+    /** @param useExpressionType If true, the typeMap can be nullptr,
+        and then mce->type is used.  For some technical reasons
+        neither the refMap or the typeMap are const here.  */
     static MethodInstance* resolve(const IR::MethodCallExpression* mce,
                                    ReferenceMap* refMap, TypeMap* typeMap,
                                    bool useExpressionType = false);
@@ -62,7 +82,8 @@ class MethodInstance {
     template<typename T> const T* to() const { return dynamic_cast<const T*>(this); }
 };
 
-// The call of an Apply method on an object that implements IApply
+/** Represents the call of an Apply method on an object that implements IApply:
+    a table, control or parser */
 class ApplyMethod final : public MethodInstance {
     ApplyMethod(const IR::MethodCallExpression* expr, const IR::IDeclaration* decl,
                 const IR::IApply* applyObject) :
@@ -78,7 +99,7 @@ class ApplyMethod final : public MethodInstance {
     bool isTableApply() const { return object->is<IR::P4Table>(); }
 };
 
-// A method call on an extern object
+/** Represents a method call on an extern object */
 class ExternMethod final : public MethodInstance {
     ExternMethod(const IR::MethodCallExpression* expr, const IR::IDeclaration* decl,
                  const IR::Method* method,
@@ -96,7 +117,7 @@ class ExternMethod final : public MethodInstance {
     const IR::Type_Extern* actualExternType;      // with type variables substituted
 };
 
-// An extern function
+/** Represents the call of an extern function */
 class ExternFunction final : public MethodInstance {
     ExternFunction(const IR::MethodCallExpression* expr,
                    const IR::Method* method,
@@ -109,7 +130,10 @@ class ExternFunction final : public MethodInstance {
     const IR::Method* method;
 };
 
-// Calling an action directly
+/** Represents the direct call of an action; This also works for
+    correctly for actions declared in a table 'actions' list, and for
+    action instantiations such as the default_action or the list of
+    table entries */
 class ActionCall final : public MethodInstance {
     ActionCall(const IR::MethodCallExpression* expr,
                const IR::P4Action* action,
@@ -122,9 +146,14 @@ class ActionCall final : public MethodInstance {
     const IR::P4Action* action;
 };
 
-// A built-in method.
-// These methods are:
-// header.setValid(), header.setInvalid(), header.isValid(), stack.push(int), stack.pop(int)
+/** This class represents the call of a built-in method:
+These methods are:
+- header.setValid(),
+- header.setInvalid(),
+- header.isValid(),
+- stack.push(int),
+- stack.pop(int)
+*/
 class BuiltInMethod final : public MethodInstance {
     friend class MethodInstance;
     BuiltInMethod(const IR::MethodCallExpression* expr, IR::ID name,
@@ -136,7 +165,12 @@ class BuiltInMethod final : public MethodInstance {
     const IR::Expression* appliedTo;  // object is an expression
 };
 
-// Similar to a MethodInstance, but for a constructor call expression
+/** This class is used to disambiguate constructor calls.
+    The core method is the static method 'resolve', which will categorize a
+    constructor as one of
+    - Extern constructor
+    - Container constructor (parser, control or package)
+*/
 class ConstructorCall {
  protected:
     virtual ~ConstructorCall() {}
@@ -150,6 +184,7 @@ class ConstructorCall {
     template<typename T> const T* to() const { return dynamic_cast<const T*>(this); }
 };
 
+/** Represents a constructor call that allocates an Extern object */
 class ExternConstructorCall : public ConstructorCall {
     explicit ExternConstructorCall(const IR::Type_Extern* type) :
             type(type) { CHECK_NULL(type); }
@@ -158,6 +193,8 @@ class ExternConstructorCall : public ConstructorCall {
     const IR::Type_Extern* type;  // actual extern declaration in program IR
 };
 
+/** Represents a constructor call that allocates an object that implements IContainer.
+    These can be package, control or parser */
 class ContainerConstructorCall : public ConstructorCall {
     explicit ContainerConstructorCall(const IR::IContainer* cont) :
             container(cont) { CHECK_NULL(cont); }
@@ -166,14 +203,21 @@ class ContainerConstructorCall : public ConstructorCall {
     const IR::IContainer* container;  // actual container in program IR
 };
 
-// Abstraction for a method call: maintains mapping between arguments
-// and parameters.  This will make it easier to introduce different
-// calling conventions in the future.
-// TODO: convert all code to use this class.
+/**
+   Abstraction for a method call: in addition to information about the
+   MethodInstance, this class also maintains a mapping between
+   arguments and the corresponding parameters.  This will make it
+   easier to introduce different calling conventions in the future,
+   e.g. calls by specifying the name of the parameter.
+
+   TODO: Today not all code paths use this class for matching
+   arguments to parameters; we should convert all code to use this
+   class.
+*/
 class MethodCallDescription {
  public:
     MethodInstance       *instance;
-    // For each callee parameter the corresponding argument
+    /// For each callee parameter the corresponding argument
     ParameterSubstitution substitution;
 
     MethodCallDescription(const IR::MethodCallExpression* mce,

frontends/p4/toP4/toP4.h

@@ -22,8 +22,11 @@ limitations under the License.
 #include "lib/sourceCodeBuilder.h"
 
 namespace P4 {
-    // conversion from P4 v1.2 IR back to P4 source
 
+/**
+This pass converts a P4-16 IR into a P4 source (text) program.
+It can optionally emit as comments a representation of the program IR.
+*/
 class ToP4 : public Inspector {
     int expressionPrecedence;  // precedence of current IR::Operation
     bool isDeclaration;  // current type is a declaration
@@ -82,9 +85,9 @@ class ToP4 : public Inspector {
      * useful functionality the ostream does not already provide; it just serves to
      * obfuscate the code */
     std::ostream* outStream;
-    // If this is set to non-nullptr, some declarations
-    // that come from libraries and models are not
-    // emitted.  Currently unused.
+    /** If this is set to non-nullptr, some declarations
+        that come from libraries and models are not
+        emitted. */
     cstring mainFile;
 
     ToP4(Util::SourceCodeBuilder& builder, bool showIR, cstring mainFile = nullptr) :

frontends/p4/typeChecking/typeChecker.cpp

@@ -18,6 +18,7 @@ limitations under the License.
 #include "typeUnification.h"
 #include "frontends/p4/substitution.h"
 #include "typeConstraints.h"
+#include "frontends/p4/coreLibrary.h"
 #include "syntacticEquivalence.h"
 #include "frontends/common/resolveReferences/resolveReferences.h"
 #include "frontends/p4/methodInstance.h"
@@ -1143,6 +1144,22 @@ const IR::Node* TypeInference::postorder(IR::Type_Header* type) {
     auto validator = [] (const IR::Type* t)
             { return t->is<IR::Type_Bits>() || t->is<IR::Type_Varbits>(); };
     validateFields(canon, validator);
+
+    const IR::StructField* varbit = nullptr;
+    for (auto field : *type->fields) {
+        auto ftype = getType(field);
+        if (ftype == nullptr)
+            return type;
+        if (ftype->is<IR::Type_Varbits>()) {
+            if (varbit == nullptr) {
+                varbit = field;
+            } else {
+                typeError("%1% and %2%: multiple varbit fields in a header",
+                          varbit, field);
+                return type;
+            }
+        }
+    }
     return type;
 }
 
@@ -2328,6 +2345,88 @@ TypeInference::actionCall(bool inActionList,
     return actionCall;
 }
 
+bool TypeInference::hasVarbits(const IR::Type_Header* type) const {
+    for (auto f : *type->fields) {
+        auto ftype = typeMap->getType(f);
+        if (ftype == nullptr)
+            continue;
+        if (ftype->is<IR::Type_Varbits>())
+            return true;
+    }
+    return false;
+}
+
+void TypeInference::checkEmitType(const IR::Expression* emit, const IR::Type* type) const {
+    if (type->is<IR::Type_Header>() || type->is<IR::Type_Stack>() || type->is<IR::Type_Union>())
+        return;
+
+    if (type->is<IR::Type_Struct>()) {
+        for (auto f : *type->to<IR::Type_Struct>()->fields) {
+            auto ftype = typeMap->getType(f);
+            if (ftype == nullptr)
+                continue;
+            checkEmitType(emit, ftype);
+        }
+        return;
+    }
+
+    ::error("%1%: argument must be a header, stack or union, or a struct of such types",
+            emit);
+}
+
+void TypeInference::checkCorelibMethods(const ExternMethod* em) const {
+    P4CoreLibrary &corelib = P4CoreLibrary::instance;
+    auto et = em->actualExternType;
+    unsigned argCount = em->expr->arguments->size();
+
+    if (et->name == corelib.packetIn.name) {
+        if (em->method->name == corelib.packetIn.extract.name) {
+            if (argCount == 0) {
+                // core.p4 is corrupted.
+                ::error("%1%: Expected exactly 1 argument for %2% method",
+                        em->expr, corelib.packetIn.extract.name);
+                return;
+            }
+
+            auto arg0 = em->expr->arguments->at(0);
+            auto argType = typeMap->getType(arg0, true);
+            if (!argType->is<IR::Type_Header>()) {
+                ::error("%1%: argument must be a header", em->expr->arguments->at(0));
+                return;
+            }
+
+            if (argCount == 1) {
+                if (hasVarbits(argType->to<IR::Type_Header>()))
+                    ::error("%1%: argument cannot contain varbit fields", arg0);
+            } else if (argCount == 2) {
+                if (!hasVarbits(argType->to<IR::Type_Header>()))
+                    ::error("%1%: argument should contain a varbit field", arg0);
+            } else {
+                // core.p4 is corrupted.
+                ::error("%1%: Expected 1 or 2 arguments for '%2%' method",
+                        em->expr, corelib.packetIn.extract.name);
+            }
+        }
+    } else if (et->name == corelib.packetOut.name) {
+        if (em->method->name == corelib.packetOut.emit.name) {
+            const IR::Expression* arg = nullptr;
+            if (argCount == 1) {
+                arg = em->expr->arguments->at(0);
+            } else if (argCount == 2) {
+                arg = em->expr->arguments->at(1);
+            } else {
+                // core.p4 is corrupted.
+                ::error("%1%: Expected 1 or 2 arguments for '%2%' method",
+                        em->expr, corelib.packetOut.emit.name);
+                return;
+            }
+
+            auto argType = typeMap->getType(arg, true);
+            checkEmitType(em->expr, argType);
+        }
+    }
+}
+
 const IR::Node* TypeInference::postorder(IR::MethodCallExpression* expression) {
     if (done()) return expression;
     methodArguments.pop_back();
@@ -2420,6 +2519,9 @@ const IR::Node* TypeInference::postorder(IR::MethodCallExpression* expression) {
                 ::error("%1%: tables cannot be invoked from actions", expression);
         }
 
+        if (mi->is<ExternMethod>())
+            checkCorelibMethods(mi->to<ExternMethod>());
+
         return result;
     }
     return expression;