Support expressions as l-values for action primitives

[antoninbas]

See #835 (comment)

The bmv2 expression engine currently only supports returning Data objects, and not Data & references. This means that expressions cannot be used as l-values for the assign action primitive. See https://github.com/p4lang/behavioral-model/blob/master/include/bm/bm_sim/actions.h#L278.

This makes it difficult to support assignments to a header stack field accessed with a runtime index (<hs>[<index expression>].<field> = X in P4). If one tries to hand-generate a JSON for such an assignment (using an expression involving the access_field and dereference_header_stack operations on the left-hand side), one will run into this assertion: https://github.com/p4lang/behavioral-model/blob/master/include/bm/bm_sim/actions.h#L294

[antoninbas]

A workaround for the header stack field case would be to introduce one or more new types for the "type value object" in the bmv2 JSON. For example, we could introduce stack_field_access, for which the value would be a JSON 3-tuple: [<stack name>, <index expression>, <field name>]. We would need to do the same for header union stacks. It is unclear to me whether this is the right thing to do. We would just be introducing more new cases for the left-hand side of assignments, instead of using the support we already have in the expression engine (dereference_header_stack, ...). Maybe we should strive to simplify the "actions" implementation in bmv2 and unify it with the "expressions" support. Introducing the new types could be a good solution for the short-term, while in the long term we can think about refactoring the actions / expressions implementation, and improving the JSON format.

[hesingh]

@antoninbas thanks for all the debugging, narrowing down the problem and filing this issue.

[hesingh]

What is the bmv2 expression engine? Is it code in https://github.com/p4lang/p4c/blob/master/backends/bmv2/common/expression.h ?

[antoninbas]

No, it's this: https://github.com/p4lang/behavioral-model/blob/master/include/bm/bm_sim/expressions.h. At the moment it only supports returning Data and bool values. I think it should be more general and be able to return Header, HeaderStack, HeaderUnion and HeaderUnionStack values, as well as references for all these types. I don't know at the moment what the best approach is to make these changes.

Edit: only the reference variant is needed for Header, HeaderStack, HeaderUnion and HeaderUnionStack.

[antoninbas]

I opened a patch for this. Probably not perfect but it covers the specific case we looked at (assignments to a header stack field accessed with a runtime index) and many others. I think the expression evaluation engine is now generic enough that a lot of the code can be simplified and many special cases can be removed.

I added a few unit tests, but I also tested with the attached JSON file.
runtime_index_LV.json.txt

which corresponds to the following program:

#include <v1model.p4>

header h1_t {
    bit<32> v;
    bit<32> idx;
}

header h2_t {
    bit<32> v;
}

struct headers {
    h1_t h1;
    h2_t[3] h2;
}

struct metadata { }

parser parserI(packet_in pkt,
               out headers hdr,
               inout metadata meta,
               inout standard_metadata_t stdmeta) {
    state start {
        pkt.extract(hdr.h1);
        pkt.extract(hdr.h2[0]);
        pkt.extract(hdr.h2[1]);
        pkt.extract(hdr.h2[2]);
        transition accept;
    }
}

control cIngress(inout headers hdr,
                 inout metadata meta,
                 inout standard_metadata_t stdmeta) {
    apply {
        hdr.h2[hdr.h1.idx].v = hdr.h1.v;
        stdmeta.egress_spec = stdmeta.ingress_port;
    }
}

control cEgress(inout headers hdr,
                inout metadata meta,
                inout standard_metadata_t stdmeta) {
    apply { }
}

control vc(inout headers hdr,
           inout metadata meta) {
    apply { }
}

control uc(inout headers hdr,
           inout metadata meta) {
    apply { }
}

control DeparserI(packet_out packet,
                  in headers hdr)
{
    apply {
        packet.emit(hdr);
    }
}

V1Switch(parserI(),
         vc(),
         cIngress(),
         cEgress(),
         uc(),
         DeparserI()) main;

Sending the following packet with Scapy:

"\xab\xab\xab\xab\x00\x00\x00\x02\x00\x00\x00\x01\x00\x00\x00\x02\x00\x00\x00\x03"
# abababab00000002000000010000000200000003

should lead to the following output packet in bmv2 log message:

abababab000000020000000100000002abababab

From a syntax perspective, nothing changed in the bmv2 JSON format. But of course, some work is required in the bmv2 compiler backend to leverage this new "feature" and support a P4 program like the one above.

[hesingh]

Thanks so much!

[hesingh]

I have a PR open to make changes to p4c bmv2 backend. I can look into your changes from today and see what I have to change in the backend - thanks.

[jafingerhut]

@antoninbas Out of curiosity, what happens if the run-time index is out of bounds of the size of the header stack array? A 32-bit index for a header stack ... :-)

[hesingh]

I suppose the behavioral-model should add a check if (index <= 0xffffffff). then access the index in the header stack array.

For a switching asic, the asic tends to include an ALU for arithmetic operations. The ALU bus (assume 32 bits wide) won't allow numbers greater than 32 bits.

[antoninbas]

@jafingerhut at this time, you should get a nice uncaught C++ out_of_range exception and the process will crash. Which I guess is better than undefined behavior. I didn't change the behavior of the "dereference_header_stack" operator in my patch. The index can be arbitrarily large by the way, I just used 32-bit in my example.

[jafingerhut]

Thanks for the info. A process crash seems reasonable to me. At least it will not be a quietly wrong answer.