bmv2: parser `set` operation contains an expression that is not an expression

[Wojtek242]

Example P4 program that produces this issue: https://github.com/p4lang/tutorials/blob/76a9067deaf35cd399ed965aa19997776f72ec55/exercises/link_monitor/solution/link_monitor.p4

Relevant part of the JSON:

            {
              "parameters" : [
                {
                  "type" : "field",
                  "value" : ["scalars", "userMetadata._egress_spec0"]
                },
                {
                  "type" : "expression",
                  "value" : {
                    "type" : "stack_field",
                    "value" : ["probe_fwd", "egress_spec"]
                  }
                }
              ],
              "op" : "set"
            }

The BMv2 spec states that the parser operation set takes two parameters, the second of which may be an expression.

Above, the second parameter has type expression, but its value is not an expression type value. It is a stack_field instead. It appears that the JSON spec does not seem to account for the possibility of a stack field being the second parameter of the set operation so I'm not sure what the correct resolution should be. Since the second parameter can already be a field it could be that a stack_field can also be allowed as the second parameter.

[antoninbas]

Moving this issue to the bmv2 repo.

[antoninbas]

This JSON is correct (at least it will be handled correctly by the bmv2 implementation), but the documentation is misleading and we should update it. It is misleading because an expression type can mean 2 different things: either a "top-level" expression node or a binary expression (with an operator and left & right operands).

Maybe the following example will make it clear:

{
    "op" : "assign",
    "parameters" : [
        {
            "type" : "expression",
            "value": {
                "type": "expression",
                "value": {
                    "op": "access_field",
                    "left": {
                        "type": "expression",
                        "value": {
                            "op": "dereference_header_stack",
                            "left": {
                                "type": "header_stack",
                                "value": "h2"
                            },
                            "right": {
                                "type": "field",
                                "value": ["h1", "idx"]
                            }
                        }
                    },
                    "right": 0
                }
            }
        },
        {
            "type" : "expression",
            "value": {
                "type": "expression",
                "value": {
                    "op": "+",
                    "left": {
                        "type" : "field",
                        "value" : ["h1", "v"]
                    },
                    "right": {
                        "type" : "hexstr",
                        "value" : "0x0000004d"
                    }
                }
            }
        }
    ]
}

Both parameters are expression nodes and these expressions are both binary expressions. Which is why for each one you have 2 expression objects at the "top" (2 objects with "type": "expression). But notice that the format of value is different for each one.

Something else that may help clarify things. The following objects have an equivalent meaning in most cases in a bmv2 JSON file:

{
    "type": "field",
    "value": ["h1", "idx"]
}

and

{
    "type": "expression",
    "value": {
        "type": "field",
        "value": ["h1", "idx"]
    }
}

The first one will evaluate faster in bmv2 because it translates to simpler operations (direct access to the field's value while the second one involves the more complex expression evaluation engine).

In your case, (stack_field as parameter in a set parser operation), the first form is not available for some (legacy?) reason, but the second form is valid.

I think the reason why we have 2 ways of expressing the same thing is that initially the bmv2 expression evaluation engine wasn't complete. But it seems with the code as it is now, we could uniformize the format. However, it is unlikely to happen soon as it would require breaking backwards-compatibility and would require non-trivial changes in bmv2 and the compiler. In the mean time, I will try to improve the documentation and make it less misleading, despite expression having an overloaded meaning.

[Wojtek242]

Thanks for the quick response! I've seen the top-level expression node before, but in all cases I've seen it before, the value was always the binary expression. Thanks for the clarification!

[jnfoster]

Closing this in p4c.

[antoninbas]

@jnfoster the issue has been transferred, so it shouldn't show as a p4c open issue any more

re-opening this as there is some work required on the bmv2 side

[jnfoster]

Whoops. Really sorry about that!

[github-actions[bot]]

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment, or this will be closed in 180 days