[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.

[fruffy]

I have split the PR into two. This PR only contains the P4Testgen fixes.

#4982 contains the BMv2 compiler fixes, which may or may not be used.

[vlstill]

The testgen change makes sense to me. A few small comments for the rest.

[vlstill]

Suggested change

	/// errorOnMethodCall flag will produce an error message if such a
	/// method is encountered.
	/// errorOnMethodCall flag will produce an error message if such a
	/// method is encountered.

[vlstill]

Can you elaborate a bit what this expansion means?

[fruffy]

Let me rethink this. The context is #4853 (comment)

The way P4Testgen interprets this expansion leads to behavior that is not correct. I do not recall whether other back ends have a similar problem but P4Testgen just interprets the spec which says:

u.hi.setValid(): sets the valid bit for header hi to true and sets the valid bit for all other headers to false, which implies that it is unspecified what value reading any member header of u will return.

It might actually make sense to generally disable this expansion until it is safe.

[vlstill]

Hmm, given that it is fundamentally broken, we might as well say that CopyStructures does not support expansion of header union assignments and error in such case. Still, I'm worried how this would impact backends that use it (if any).

It seems to me the only way to do this in midend would be to do consecutive ifsforisValid()` for all alternatives, which could lead to inefficient code in many backend. But backends can always opt-out of expanding headers.

BTW. how does testgen process header union assignments if it expands assignments of headers, but not of header unions?

---

Also, this PR should probably have a breaking-change tag because of the changes in CopyStructures.

[fruffy]

I think I will split the change to CopyStructures out again and mark it breaking, then rebase the P4Testgen fixes on top.

[fruffy]

BTW. how does testgen process header union assignments if it expands assignments of headers, but not of header unions?

Header union and header assignments should now be handled by https://github.com/p4lang/p4c/blob/main/backends/p4tools/modules/testgen/core/small_step/cmd_stepper.cpp#L68 and https://github.com/p4lang/p4c/blob/main/backends/p4tools/common/core/abstract_execution_state.cpp#L186C30-L186C46

Iirc CopyStructures is technically not needed by P4Testgen anymore but there are other passes it uses (ParserUnroll?) which require it.

[vlstill]

Iirc CopyStructures is technically not needed by P4Testgen anymore but there are other passes it uses (ParserUnroll?) which require it.

It might be worth checking if we can just disable it in testgen (or leave it just for structures).

[vlstill]

Any reason to use the underscore in name?

[fruffy]

It's a habit I have picked up recently. I can change it back. The reason it is useful is that you can declare private members as _member and then the public getter as member().

[vlstill]

I can see why this is useful (I used to use this convention myself), but my point was more that it is quite unusual in P4C codebase.

[fruffy]

Yup, will change it back. We have this style in the testgen back end so sometimes it creeps back in.

One problem is that the style in P4C can be random and changes from file to file.
Ideally, we should push #4410 forward.