Compiler fails to reject passing of non-compile-time-known values as arguments to directionless parameters of non-actions

[kfcripps]

Per P4-16 spec:

For anything other than an action, e.g. a control, parser, or function, a directionless parameter means that the value supplied as an argument in a call must be a compile-time known value (see Section 18.1).

Parameters with type int are not supported for actions. Parameters with type int for other callable entities of a program, e.g. controls, parsers, or functions, must be directionless, indicating that all calls must provide a compile-time known value as an argument for such a parameter.

However, for example, the test testdata/p4_16_samples/extern-int-arg1.p4 (as well as several other tests) successfully compiles, despite:

• idx, a non-compile-time-known value is passed to Counter::count in actions a1 and a2,
• and Counter::count(not an action)'s idx param's type is a directionless int (and therefore must accept a compile-time-known value).

If I am understanding the spec correctly, this should instead result in an error message.

The contents of this test are pasted below for convenience:

#include <core.p4>

control generic<M>(inout M m);
package top<M>(generic<M> c);

header h1_t {
    bit<32>     f1;
    bit<16>     h1;
    bit<8>      b1;
    bit<8>      cnt;
}

struct headers_t {
    h1_t        head;
}

extern Counter {
    Counter(int size);
    void count(int idx);
}

control c(inout headers_t hdrs) {
    Counter(256) ctr;

    action a0() {
        ctr.count(0);
    }
    action a1(bit<8> idx) {
        ctr.count(idx);
    }
    action a2(bit<4> idx) {
        ctr.count(idx);
    }

    table t1 {
        key = { hdrs.head.f1 : exact; }
        actions = { a0; a1; a2; }
    }

    apply {
        t1.apply();
    }
}

top(c()) main;

[fruffy]

@KunJeong @jaehyun1ee any comments, related issues you have filed?

[kfcripps]

I have not attempted to analyze the resulting failing tests, but the following diff might fix this:

diff --git a/frontends/p4/typeChecking/typeCheckExpr.cpp b/frontends/p4/typeChecking/typeCheckExpr.cpp
index 2079589aba8..fff8ccff309 100644
--- a/frontends/p4/typeChecking/typeCheckExpr.cpp
+++ b/frontends/p4/typeChecking/typeCheckExpr.cpp
@@ -1319,7 +1319,8 @@ const IR::Node *TypeInferenceBase::postorder(const IR::PathExpression *expressio
             paramDecl->direction == IR::Direction::Out) {
             setLeftValue(expression);
             setLeftValue(getOriginal<IR::Expression>());
-        } else if (paramDecl->direction == IR::Direction::None) {
+        } else if (paramDecl->direction == IR::Direction::None &&
+                   !findContext<IR::P4Action>()) {
             setCompileTimeConstant(expression);
             setCompileTimeConstant(getOriginal<IR::Expression>());
         }

[jaehyun1ee]

I don't have any related issue to this, but seems like @kfcripps's point is right. Directionless parameters to an action should be treated differently from directionless parameters of other callable entities, because for actions, directionless does not imply compile-time known.

Unrelated to compile-time known-ness, but a problem I see in the example

Though I see some parts of the example program deviating from the P4 spec, related to implicit casts.

extern Counter {
    Counter(int size);
    void count(int idx);
}

control c(inout headers_t hdrs) {
    Counter(256) ctr;

    action a1(bit<8> idx) {
        ctr.count(idx); // A
    }
    action a2(bit<4> idx) {
        ctr.count(idx); // B
    }
}

At method call annotated A, the type of the argument (bit<8>) and the parameter (int) mismatches. But because we allow implicit casts on directionless arguments, we can try implicitly casting bit<8> to int. However, we can't implicitly cast fixed-width bit type to arbitrary precision integer type (while the converse is true). Similar argument applies for the method call B.

Suggestion for changing the example

To focus on the issue of compile-time known-ness of directionless parameters of actions, how about changing our example to:

#include<core.p4>

control generic<M>(inout M m);
package top<M>(generic<M> c);

header h1_t {
    bit<32>     f1;
    bit<16>     h1;
    bit<8>      b1;
    bit<8>      cnt;
}

struct headers_t {
    h1_t        head;
}

extern Counter {
    Counter(int size);
    void count(bit<8> idx);
}

control c(inout headers_t hdrs) {
    Counter(256) ctr;

    action a1(bit<8> idx) {
        ctr.count(idx);
    }

    table t1 {
        key = { hdrs.head.f1 : exact; }
        actions = { a1; }
    }

    apply {
        t1.apply();
    }
}

top(c()) main;

With this modified example, I agree with Kyle's point. The bit<8> idx parameter to action a1 should not be regarded as compile-time known, so a call to ctr.count(idx) should not be allowed.

In relation to P4-SpecTec

I think we made the same mistake in our P4-SpecTec formalization as well, where the modified example also passes the type checker in P4-SpecTec. I guess we should also apply similar patches as Kyle proposed in the P4C source.

Would it be helpful if I patch P4-SpecTec accordingly and report our test results, to cross-check the results of P4C and P4-SpecTec?