[P4Testgen] Set other headers also invalid when calling setInvalid on a union header.

[fruffy]

Fixes #4448.

[fruffy]

@p-sawicki This is a potential fix but it currently causes BMv2 test failures.

[antoninbas]

I can think about whether it makes sense to do that in bmv2 (to be fair, I find the specified behavior a bit surprising here), but this part looks good to me for now.

[antoninbas]

LGTM on the bmv2 side

[fruffy]

The failure in issue1897-bmv2.p4 is because of #3842.
P4Testgen uses the CopyStructures pass, which incorrectly expands the header union assignment
from

hdr.addr_dst = addr_0;

to

hdr.addr_dst.ipv4.setValid();
hdr.addr_dst.ipv4.addr = addr_0.ipv4.addr;
hdr.addr_dst.ipv6.setValid();
hdr.addr_dst.ipv6.addr = addr_0.ipv6.addr;

This assignment always invalidates ipv4, even though it may be valid.

Unfortunately, the FlattenUnion pass, which could be used to fix this, does not work. Instead, I introduced a configuration option for CopyStructures to disable expansion of header union assignment in specific cases.

[fruffy]

@antoninbas I had to make some changes to the parser.cpp code, too. Unfortunately, that piece of code is written differently so I had to make some awkward changes. Ideally, the code there should be refactor but that is not in scope for this PR. Any suggestions for a better implementation?

[antoninbas]

@antoninbas I had to make some changes to the parser.cpp code, too. Unfortunately, that piece of code is written differently so I had to make some awkward changes. Ideally, the code there should be refactor but that is not in scope for this PR. Any suggestions for a better implementation?

I haven't worked on this code in so long that it's hard for me to provide good insights there (I also didn't author it in the first place). Ideally there wouldn't be that many differences in the backend code between parser and actions, as there aren't really differences when it comes to the format of the output JSON.

A simple alternative would be to add a new remove_header_conformant primitive to bmv2 targets, which would strictly conform to the spec. In the longer term it could also become the default remove_header implementation.
I bring it up because it wouldn't be too hard to implement in bmv2. However, it is quite likely that we will see a few compatibility issues if we go this way (old simple_switch build with new p4c build).

The easiest alternative all around would be to change the implementation of remove_header. Usually I prefer to avoid a behavioral change like this in bmv2. This is why I first suggested handling this in the compiler. In the past, when I had to do something like this, I introduced a compile-time flag to toggle the behavior (p4lang/behavioral-model@9131ed9). If there is some sort of consensus that changing the bmv2 behavior (note that it will also bring assign_header to conformance) is the right thing to do (and that the current implementation should be considered a bug), it will hopefully only take a few lines of code. I actually think that there is a strong case to be made in favor of this, as going back to the first P4_16 version (https://p4.org/p4-spec/docs/P4-16-v1.0.0-spec.html#sec-header-unions), this was always the specified behavior. I am now leaning towards this option. I can open a PR this weekend. Let me know what you think.

[fruffy]

@antoninbas I had to make some changes to the parser.cpp code, too. Unfortunately, that piece of code is written differently so I had to make some awkward changes. Ideally, the code there should be refactor but that is not in scope for this PR. Any suggestions for a better implementation?

I haven't worked on this code in so long that it's hard for me to provide good insights there (I also didn't author it in the first place). Ideally there wouldn't be that many differences in the backend code between parser and actions, as there aren't really differences when it comes to the format of the output JSON.

A simple alternative would be to add a new remove_header_conformant primitive to bmv2 targets, which would strictly conform to the spec. In the longer term it could also become the default remove_header implementation. I bring it up because it wouldn't be too hard to implement in bmv2. However, it is quite likely that we will see a few compatibility issues if we go this way (old simple_switch build with new p4c build).

The easiest alternative all around would be to change the implementation of remove_header. Usually I prefer to avoid a behavioral change like this in bmv2. This is why I first suggested handling this in the compiler. In the past, when I had to do something like this, I introduced a compile-time flag to toggle the behavior (p4lang/behavioral-model@9131ed9). If there is some sort of consensus that changing the bmv2 behavior (note that it will also bring assign_header to conformance) is the right thing to do (and that the current implementation should be considered a bug), it will hopefully only take a few lines of code. I actually think that there is a strong case to be made in favor of this, as going back to the first P4_16 version (https://p4.org/p4-spec/docs/P4-16-v1.0.0-spec.html#sec-header-unions), this was always the specified behavior. I am now leaning towards this option. I can open a PR this weekend. Let me know what you think.

The current changes in the compiler work, but they are inefficient/hacky. We could end up generating many redundant remove_header calls. So I also prefer fixing this in BMv2.

If the implementation in BMv2 is not too hard we can do it. It might require a package update too etc. I am all for considering this a bug in BMv2 and adding a config (this could even be optional).

We can get the conversation on that started with the PR on the BMv2 side.

[jafingerhut]

@antoninbas I had to make some changes to the parser.cpp code, too. Unfortunately, that piece of code is written differently so I had to make some awkward changes. Ideally, the code there should be refactor but that is not in scope for this PR. Any suggestions for a better implementation?

I haven't worked on this code in so long that it's hard for me to provide good insights there (I also didn't author it in the first place). Ideally there wouldn't be that many differences in the backend code between parser and actions, as there aren't really differences when it comes to the format of the output JSON.
A simple alternative would be to add a new remove_header_conformant primitive to bmv2 targets, which would strictly conform to the spec. In the longer term it could also become the default remove_header implementation. I bring it up because it wouldn't be too hard to implement in bmv2. However, it is quite likely that we will see a few compatibility issues if we go this way (old simple_switch build with new p4c build).
The easiest alternative all around would be to change the implementation of remove_header. Usually I prefer to avoid a behavioral change like this in bmv2. This is why I first suggested handling this in the compiler. In the past, when I had to do something like this, I introduced a compile-time flag to toggle the behavior (p4lang/behavioral-model@9131ed9). If there is some sort of consensus that changing the bmv2 behavior (note that it will also bring assign_header to conformance) is the right thing to do (and that the current implementation should be considered a bug), it will hopefully only take a few lines of code. I actually think that there is a strong case to be made in favor of this, as going back to the first P4_16 version (https://p4.org/p4-spec/docs/P4-16-v1.0.0-spec.html#sec-header-unions), this was always the specified behavior. I am now leaning towards this option. I can open a PR this weekend. Let me know what you think.

The current changes in the compiler work, but they are inefficient/hacky. We could end up generating many redundant remove_header calls. So I also prefer fixing this in BMv2.

If the implementation in BMv2 is not too hard we can do it. It might require a package update too etc. I am all for considering this a bug in BMv2 and adding a config (this could even be optional).

We can get the conversation on that started with the PR on the BMv2 side.

A minor suggestion:

If you make changes to BMv2 to make this more efficient, I would recommend adding a new operation that causes all members of the union to become invalid, while also keeping the operation that makes only the selected member of the header union invalid, without modifying the validity of any other members.

Reason: In case the language spec changes to have both kinds of operations in the future. I have no plans to make such a change, but others have asked about it on occasion.

[jafingerhut]

Antonin's suggestion of adding a new method to BMv2, e.g. "A simple alternative would be to add a new remove_header_conformant primitive to bmv2 targets, which would strictly conform to the spec." and then having the p4c BMv2 back end use that, and leave the existing BMv2 methods unchanged, is certainly the easiest way to get backwards compatibility across all code that exists today.

I don't see any disadvantages to that approach, especially if it is documented in comments why the new method was created, and how it differs in behavior from the existing method.

[fruffy]

This PR is currently stuck. I can move out the testgen-specific fixes and mark the failures Xfail.