Container arguments with type infint are treated as type variables (p…

…4lang#3889) Signed-off-by: Mihai Budiu <mbudiu@vmware.com>
kamleshbhalui · Feb 17, 2023 · 0c82a42 · 0c82a42
1 parent 19ed58d
commit 0c82a42
Show file tree

Hide file tree

Showing 17 changed files with 662 additions and 11 deletions.
diff --git a/README.md b/README.md
@@ -230,8 +230,6 @@ use them, but YMMV.
 
 - `git` for version control
 
-- GNU autotools for the build process
-
 - CMake 3.10.2 or higher
 
 - Boehm-Weiser garbage-collector C++ library
@@ -240,7 +238,7 @@ use them, but YMMV.
 
 - Google Protocol Buffers 3.0 or higher for control plane API generation
 
-- C++ boost library (minimally used)
+- C++ boost library
 
 - Python 3 for scripting and running tests
 

diff --git a/backends/ebpf/CMakeLists.txt b/backends/ebpf/CMakeLists.txt
@@ -195,7 +195,7 @@ if (LIBBPF)
 else()
   message(WARNING "Missing the libbpf dependency, disabling kernel tests."
   " You can install libbpf by running './build_libbpf' in the "
-  "${CMAKE_CURRENT_SOURCE_DIR}/runtime folder.")
+  "${CMAKE_CURRENT_SOURCE_DIR} folder.")
   set (SUPPORTS_KERNEL False)
 endif()
 

diff --git a/frontends/p4/typeChecking/typeChecker.cpp b/frontends/p4/typeChecking/typeChecker.cpp
@@ -47,12 +47,32 @@ class ConstantTypeSubstitution : public Transform {
         CHECK_NULL(tc);
         LOG3("ConstantTypeSubstitution " << subst);
     }
+
+#if 0
+    // If two Type_InfInt types have been unified, we replace
+    // one of them with the other.
+    const IR::Node *postorder(IR::Type_InfInt* type) override {
+        auto repl = subst->get(type);
+        if (!repl)
+            return type;
+        while (repl->is<IR::Type_Var>()) {
+            auto next = subst->get(repl->to<IR::ITypeVar>());
+            BUG_CHECK(next != repl, "Cycle in substitutions: %1%", next);
+            if (!next) break;
+            repl = next;
+        }
+        LOG2("Replacing " << type << " with " << repl);
+        return repl;
+    }
+#endif
+
     const IR::Node *postorder(IR::Constant *cst) override {
         auto cstType = typeMap->getType(getOriginal(), true);
         if (!cstType->is<IR::ITypeVar>()) return cst;
         auto repl = cstType;
         while (repl->is<IR::ITypeVar>()) {
             auto next = subst->get(repl->to<IR::ITypeVar>());
+            BUG_CHECK(next != repl, "Cycle in substitutions: %1%", next);
             if (!next) break;
             repl = next;
         }
@@ -1116,6 +1136,26 @@ std::pair<const IR::Type *, const IR::Vector<IR::Argument> *> TypeInference::con
     constructor = cloneWithFreshTypeVariables(constructor->to<IR::IMayBeGenericType>())
                       ->to<IR::Type_Method>();
     CHECK_NULL(constructor);
+
+    {
+        // Constructor parameters that are infint are also treated as type variables
+        TypeVariableSubstitution tvs;
+        auto params = constructor->parameters;
+        for (auto param : params->parameters) {
+            if (auto v = param->type->to<IR::Type_InfInt>()) {
+                auto tv = new IR::Type_InfInt(param->type->srcInfo);
+                bool b = tvs.setBinding(v, tv);
+                BUG_CHECK(b, "failed replacing %2% with %3%", v, tv);
+            }
+        }
+        TypeVariableSubstitutionVisitor sv(&tvs, true);
+        sv.setCalledBy(this);
+        constructor = constructor->apply(sv)->to<IR::Type_Method>();
+        CHECK_NULL(constructor);
+        learn(constructor, this);
+        LOG3("Cloned constructor arguments " << constructor);
+    }
+
     bool isPackage = container->is<IR::Type_Package>();
     auto params = constructor->parameters;
     checkParameters(params, !forbidModules, !isPackage);

diff --git a/frontends/p4/typeChecking/typeSubstitution.cpp b/frontends/p4/typeChecking/typeSubstitution.cpp
@@ -47,7 +47,7 @@ cstring TypeVariableSubstitution::compose(const IR::ITypeVar *var, const IR::Typ
     // Check to see whether we already have a binding for this variable
     if (containsKey(var)) {
         const IR::Type *bound = lookup(var);
-        BUG("Two constraints on the same variable %1%: %2% and %3%", var->toString(),
+        BUG("Two bindings for the same variable %1%: %2% and %3%", var->toString(),
             substitution->toString(), bound->toString());
     }
 
@@ -57,18 +57,29 @@ cstring TypeVariableSubstitution::compose(const IR::ITypeVar *var, const IR::Typ
     if (!success) BUG("Cannot set binding");
 
     TypeVariableSubstitutionVisitor visitor(tvs);
+    bool cycle = false;  // set if we detect X -> V and V -> X substitutions.
     for (auto &bound : binding) {
         const IR::Type *type = bound.second;
         const IR::Node *newType = type->apply(visitor);
         if (newType == nullptr) return "Could not replace '%1%' with '%2%'";
         if (newType == type) continue;
 
-        LOG3("Refining subsitution for " << bound.first->getNode() << " to " << newType);
-        bound.second = newType->to<IR::Type>();
+        if (bound.first->asType() == newType) {
+            cycle = true;
+        } else {
+            LOG3("Refining substitution for " << bound.first->getNode() << " to " << newType);
+            bound.second = newType->to<IR::Type>();
+        }
     }
 
-    success = setBinding(var, substitution);
-    if (!success) BUG("Failed to insert binding");
+    if (cycle) {
+        LOG3("Ignoring substitution already implied " << var << "->" << substitution);
+    } else {
+        LOG3("Actual binding " << var << " " << dbp(var) << "->" << dbp(substitution) << "="
+                               << substitution);
+        success = setBinding(var, substitution);
+        if (!success) BUG("Failed to insert binding");
+    }
     return "";
 }
 
@@ -82,6 +93,7 @@ void TypeVariableSubstitution::simpleCompose(const TypeVariableSubstitution *oth
         LOG3("Setting substitution for " << v.first->getNode() << " to " << subst);
         binding[v.first] = subst;
     }
+    debugValidate();
 }
 
 bool TypeVariableSubstitution::setBindings(const IR::Node *errorLocation,
@@ -110,6 +122,19 @@ bool TypeVariableSubstitution::setBindings(const IR::Node *errorLocation,
     return true;
 }
 
+void TypeVariableSubstitution::debugValidate() {
+#if 0
+    // Turn on only for debugging
+    for (auto v : binding) {
+        const IR::Type *subst = v.second;
+        TypeOccursVisitor occurs(v.first);
+        subst->apply(occurs);
+        BUG_CHECK(!occurs.occurs,
+                  "'%1%' occurs in '%2%' which replaces it", v.first, v.second);
+    }
+#endif
+}
+
 // to call from gdb
 void dump(P4::TypeVariableSubstitution &tvs) { std::cout << tvs << std::endl; }
 void dump(P4::TypeVariableSubstitution *tvs) { std::cout << *tvs << std::endl; }

diff --git a/frontends/p4/typeChecking/typeSubstitution.h b/frontends/p4/typeChecking/typeSubstitution.h
@@ -44,7 +44,7 @@ class TypeSubstitution : public IHasDbPrint {
 
     /* This can fail if id is already bound.
      * @return true on success. */
-    bool setBinding(T id, const IR::Type *type) {
+    virtual bool setBinding(T id, const IR::Type *type) {
         CHECK_NULL(id);
         CHECK_NULL(type);
         auto it = binding.find(id);
@@ -61,7 +61,7 @@ class TypeSubstitution : public IHasDbPrint {
         bool first = true;
         for (auto it : binding) {
             if (!first) out << std::endl;
-            out << it.first << " -> " << dbp(it.second);
+            out << dbp(it.first) << " " << it.first << " -> " << dbp(it.second) << " " << it.second;
             first = false;
         }
     }
@@ -82,6 +82,12 @@ class TypeVariableSubstitution final : public TypeSubstitution<const IR::ITypeVa
     // In this variant of compose all variables in 'other' that are
     // assigned to are disjoint from all variables already in 'this'.
     void simpleCompose(const TypeVariableSubstitution *other);
+    void debugValidate();
+    bool setBinding(const IR::ITypeVar *id, const IR::Type *type) override {
+        auto result = TypeSubstitution::setBinding(id, type);
+        debugValidate();
+        return result;
+    }
 };
 
 }  // namespace P4

diff --git a/testdata/p4_16_samples/issue3884-1.p4 b/testdata/p4_16_samples/issue3884-1.p4
@@ -0,0 +1,32 @@
+control C1()(int Size)
+{
+    apply {}
+}
+
+control C2()(int macSize, int macSizeI)
+{
+  C1(macSize) outerDst;
+  C1(macSize) outerSrc;
+  C1(macSizeI) innerDst;
+  C1(macSizeI) innerSrc;
+
+  apply {
+    outerDst.apply();
+    outerSrc.apply();
+    innerDst.apply();
+    innerSrc.apply();
+  }
+}
+
+control ingress()
+{
+  C2(65536,65536) c2;
+  apply {
+      c2.apply();
+  }
+}
+
+control Ingress();
+package Switch(Ingress ingress);
+
+Switch(ingress()) main;
diff --git a/testdata/p4_16_samples/issue3884.p4 b/testdata/p4_16_samples/issue3884.p4
@@ -0,0 +1,74 @@
+#include <core.p4>
+
+control C1(in bit<48> address, inout bit<4> val, in bool valid)(int Size)
+{
+  action A(bit<4> v) {val = v;}
+  action B() {val = 0;}
+  table lookup {
+    key = {address : exact;}
+    actions = {A; B;}
+    const default_action = A((((1<<4)-1)));
+    size = Size;
+  }
+  apply {
+    if(valid)
+      lookup.apply();
+    else A((((1<<4)-1)));
+  }
+}
+
+control C2(
+  in bit<48> srcMacO, in bit<48> dstMacO, in bit<48> srcMacI, in bit<48> dstMacI,
+  in bool outerEthValid, in bool innerEthValid,
+  inout bit<4> valFinal)
+(int macSize, int macSizeI)
+{
+  bit<4> val1 = 0;
+  bit<4> val2 = 0;
+  bit<4> val3 = 0;
+  bit<4> val4 = 0;
+
+  C1(macSize) outerDst;
+  C1(macSize) outerSrc;
+  C1(macSizeI) innerDst;
+  C1(macSizeI) innerSrc;
+
+  apply {
+    outerDst.apply(dstMacO,val1,outerEthValid);
+    outerSrc.apply(srcMacO,val2,outerEthValid);
+    innerDst.apply(dstMacI,val3,innerEthValid);
+    innerSrc.apply(srcMacI,val4,innerEthValid);
+  }
+}
+
+header eth_t {
+  bit<48> dstAddr;
+  bit<48> srcAddr;
+  bit<16> etherType;
+}
+
+struct internal_t {
+  bit<4>  val;
+}
+
+struct headers_t {
+  eth_t baseEth;
+  eth_t extEth;
+  internal_t internal;
+}
+
+control ingress(inout headers_t hdr)
+{
+  C2(65536,65536) c2;
+  apply {
+      c2.apply(hdr.baseEth.srcAddr,hdr.baseEth.dstAddr,hdr.extEth.srcAddr,hdr.extEth.dstAddr,
+               hdr.baseEth.isValid(), hdr.extEth.isValid(), hdr.internal.val);
+  }
+}
+
+control Ingress<H>(inout H hdr);
+package Pipeline<H>(Ingress<H> ingress);
+package Switch<H>(Pipeline<H> pipe);
+
+Pipeline(ingress()) ig;
+Switch(ig) main;
diff --git a/testdata/p4_16_samples_outputs/issue3884-1-first.p4 b/testdata/p4_16_samples_outputs/issue3884-1-first.p4
@@ -0,0 +1,28 @@
+control C1()(int Size) {
+    apply {
+    }
+}
+
+control C2()(int macSize, int macSizeI) {
+    C1(macSize) outerDst;
+    C1(macSize) outerSrc;
+    C1(macSizeI) innerDst;
+    C1(macSizeI) innerSrc;
+    apply {
+        outerDst.apply();
+        outerSrc.apply();
+        innerDst.apply();
+        innerSrc.apply();
+    }
+}
+
+control ingress() {
+    C2(65536, 65536) c2;
+    apply {
+        c2.apply();
+    }
+}
+
+control Ingress();
+package Switch(Ingress ingress);
+Switch(ingress()) main;
diff --git a/testdata/p4_16_samples_outputs/issue3884-1-frontend.p4 b/testdata/p4_16_samples_outputs/issue3884-1-frontend.p4
@@ -0,0 +1,8 @@
+control ingress() {
+    apply {
+    }
+}
+
+control Ingress();
+package Switch(Ingress ingress);
+Switch(ingress()) main;
diff --git a/testdata/p4_16_samples_outputs/issue3884-1-midend.p4 b/testdata/p4_16_samples_outputs/issue3884-1-midend.p4
@@ -0,0 +1,8 @@
+control ingress() {
+    apply {
+    }
+}
+
+control Ingress();
+package Switch(Ingress ingress);
+Switch(ingress()) main;
diff --git a/testdata/p4_16_samples_outputs/issue3884-1.p4 b/testdata/p4_16_samples_outputs/issue3884-1.p4
@@ -0,0 +1,28 @@
+control C1()(int Size) {
+    apply {
+    }
+}
+
+control C2()(int macSize, int macSizeI) {
+    C1(macSize) outerDst;
+    C1(macSize) outerSrc;
+    C1(macSizeI) innerDst;
+    C1(macSizeI) innerSrc;
+    apply {
+        outerDst.apply();
+        outerSrc.apply();
+        innerDst.apply();
+        innerSrc.apply();
+    }
+}
+
+control ingress() {
+    C2(65536, 65536) c2;
+    apply {
+        c2.apply();
+    }
+}
+
+control Ingress();
+package Switch(Ingress ingress);
+Switch(ingress()) main;
diff --git a/testdata/p4_16_samples_outputs/issue3884-1.p4-stderr b/testdata/p4_16_samples_outputs/issue3884-1.p4-stderr