varbit fields are not properly implemented

[mihaibudiu]

There are several issues with varbit:

• we don't properly convert P4-14 programs with varbit fields into P4-16 programs using v1model
• we do not generate proper code for handling varbit fields on BMv2

[antoninbas]

I can look into how to support the 2 argument extract in bmv2. It's not such a big difference with what is supported today. Today in bmv2, the VL field size expression is a property of the VL field; I just need to support providing this expression as part of the extract call instead. The only difficulty is that I need to make sure the JSON stays backward-compatible.

[mihaibudiu]

You can use another name for the method, like 'extractVarbits' if that helps.
The compiler should take care of that.

That will only be half of the equation, though, we also need to support converting P4-14 correctly, and also assignments between varbit values. I guess that today they can be done using copy_header.

Also, @akeep pointed out checksums; does the checksum work with a field list where a field is a varbit?

[antoninbas]

Apart from the parsing, I don't see what else would need to change. Assignments and using VL fields in calculations / checksums should work fine.

[antoninbas]

Support for this has been added in bmv2 with p4lang/behavioral-model#330
2 notable things:

• there is a new parser operation, extract_VL which takes 2 parameters, the second being an expression used to compute the length
• I introduced the notion of core primitives in bmv2, which are available on all bmv2 targets. These are assign, assign_VL and assign_header. assign replaces modify_field (which is still available). assign_VL implements correct assignment for varbit fields, as per the P4_16 spec (it is important to call assign_VL and not assume that bmv2 will work properly when calling assign on a varbit field); assign_VL does not really perform runtime checks so the compiler is expected to verify that the varbit assignment is conform to the spec. Finally assign_header replaces copy_header (which is still available) and it works even if the header includes a varbit field.

[mihaibudiu]

Thank you, this simplifies things. Can you have varbits in metadata?

[antoninbas]

It should be possible to use a varbit metadata field as either operand of assign_VL; if I'm not mistaken that is the only thing that is allowed by the spec (if you consider that a metadata field cannot be used with extract / emit).

[mihaibudiu]

@antoninbas : I have started to work on this issue. I have a P4-16 file file which seems to generate json that is parsed by the simple_switch, but it crashes the switch when executing. https://github.com/mbudiu-vmw/p4c-clone/tree/issue447 shows a reproduction. The test that fails is
./bmv2/testdata/p4_16_samples/issue447-bmv2.p4.test

[mihaibudiu]

BTW: the program contains just an extract and an emit of a header with a single varbit field; no other processing.

[antoninbas]

This doesn't seem related to varbit support at all. Your two pipelines are called "cIngress" and "cEgress", but simple_switch expects "ingress" and "egress".

[antoninbas]

At least that's what I see when I generate the JSON file for issue447-bmv2.p4 using your branch.

[mihaibudiu]

I believe that this is fixed.