Should varbit element be allowed in a tuple?

[jafingerhut]

The attached program gives an error with 2017-Apr-04 version of p4test and p4c-bm2-ss, because there is a varbit<> included in a tuple being fed as input to Checksum16's get() method.

terminate called after throwing an instance of 'Util::CompilerBug'
  what():  COMPILER BUG: ../frontends/p4/typeChecking/typeChecker.h:100
/home/andy/mnt/p4-docs/p4-spec/p4-16/spec/extract-hdr-with-varbit.p4(27): Field varbit<320> field_10 of struct tuple_0 cannot have type varbit<320>
    varbit<320> options;
           ^^^

p4test gives no error if the varbit<> is removed from the tuple. p4c-bm2-ss still crashes without the varbit<> element with the following messages:

terminate called after throwing an instance of 'Util::CompilerBug'
  what():  COMPILER BUG: ../backends/bmv2/jsonconverter.cpp:328
/home/andy/mnt/p4-docs/p4-spec/p4-16/spec/extract-hdr-with-varbit.p4(87): ck_1/ck;: could not convert to Json
               ck.get({hdr.ipv4.version,
               ^^

Aborted (core dumped)

I understand that the Checksum16 extern in v1model.p4 might not be the final version. I adapted the checksum-checking code from another program in testdata/p4_16_samples directory that did not include IPv4 options.
extract-hdr-with-varbit.p4.txt

[mihaibudiu]

Indeed, our BMv2 back-end does not support varbit fields at this point; there is no equivalent for the two-argument extract function in BMv2. P4-14 does it in a very different way. I will file a separate issue for this problem.

[jafingerhut]

I have tested with an updated P4_16 program that checks IPv4 header checksums, even for IPv4 headers with a varbits field to hold IPv4 options, and it handles input packets with correct checksums, and correctly updates the IPv4 header checksum after IPv4 header fields are modified. So not only is the p4c issue corrected, but it seems to generate code that causes bmv2 to do the right thing. Great!

The program I tested with is here: https://github.com/jafingerhut/p4-guide/blob/master/checksum/checksum-ipv4-with-options.p4

I tested with the latest version of p4c and behavioral-model repo code as of 2017-Aug-28.

[cc10512]

@jafingerhut do you happen to have an STF packet test for your test program? It would be fantastic to add it to the compiler testsuite.

[jafingerhut]

@cc10512 It looks like I did my testing mentioned above with a version of p4c after this commit of Mihai's went in "New checksum APIs in v1model; better alias analysis in side-effects; …" f09a95d

but before you reverted it on 2017-Aug-28.

My test program worked using the new update_checksum() primitive operation (or extern function?) created by Mihai's commit, and does not compile without that.

[cc10512]

@jafingerhut we'll need to support both versions for a little while such that we give folks a chance to transition their legacy codes. We're discussing a release process to avoid this kind of issues in the future. In the meantime, you could create a branch that includes that commit and use that to test your program.

[jafingerhut]

I created a branch of p4c in my forked repo, branched just before the v1model checksum API additions were reverted, and created a PR of a test P4_16 program and an STF file here: #882