p4c test program issue1653-bmv2.p4 is perhaps a bad test

[jafingerhut]

https://github.com/p4lang/p4c/blob/master/testdata/p4_16_samples/issue1653-bmv2.p4#L49

The field list given to the clone operation contains a packet header.

There are other potential issues with clone/recirc/resub and lists of metadata fields that are given in other issues, but at least for P4_14, which is the origin of these operations and the reason they are present in P4_16's v1model architecture, the specification says:

"Only metadata (and not header) fields can be specified in the field_list." (documentation for clone_ingress_pkt_to_egress operation) "Only meta- data (and not header) fields can be specified in the field_list." (documentation for clone_egress_pkt_to_ingress operation)

Similar phrases were added for recirculate and resubmit operations in v1.0.5 of the P4_14 spec.

p4c does not give an error when you attempt to use a field_list for these operations that violates those sentences, but perhaps it should?

In any case, it seems like a good idea not to create test cases that expect good behavior for an unsupported case like this.

[hesingh]

Great catch. Sorry, I missed the behavior about Clone3 during creating the test.

Yes, I agree the compiler should give an error when a header is used with Clone3.

[hesingh]

I have fixed the issue in the diffs and will send out the PR soon.

[hesingh]

Created the PR for code changes.

https://github.com/p4lang/p4c/pull/1688

[mihaibudiu]

I think that in P4-16 we should support cloning/recirculation for any kind of serializable values. The distinction between headers and metadata is artificial.

[jafingerhut]

Does that mean that if there was a restriction to non-header metadata fields in P4_14, you would prefer that such a restriction were specific to when it is dealing with P4_14 source programs? If so, that would make sense to me.

With the field lists being name references in P4_14 recirculate/resubmit/clone metadata-to-preserve, allowing constants or named references to packet header fields really doesn't seem to make a lot of sense.

[mihaibudiu]

The way we architected the compiler we try not to do anything that depends on the fact that a source is p4-14 or p4-16. The only real difference is that p4-14 allows declarations in any order, but other than that the whole compiler only handles p4-16 programs. We translate p4-14 to p4-16 as the first step and then try to forget about how it originated. If we use this recipe this simplifies very much the maintenance, and it also ensures that p4-16 indeed covers all aspects of p4-14 (at this point we are missing recirculate and parser exceptions).

We could reject P4-14 programs that attempt to recirculate header fields before we translate them.

Since we have this principle of equals substituting equals it means that in P4-16 we cannot distinguish between a header field and a metadata field if they evaluate to the same value. So this restriction cannot exist for p4-16 programs.

[hesingh]

@mbudiu-vmw Could you please take over the PR and add rejection in the P4-14 program. I haven't worked with P4-14.

[mihaibudiu]

I have other higher priority items on my agenda right now.

[jafingerhut]

Closing this old issue, which seem obsolete/irrelevant now. clone3 has been replaced with clone_preserving_field_list at this point in v1model, and similarly for resubmit.