Fix for issue #542 #544
Fix for issue #542 #544
Conversation
|
LGTM, but probably we should discuss briefly at the January P4 LDWG meeting? |
p4-16/spec/P4-16-spec.mdk
Outdated
| @@ -3620,6 +3635,10 @@ parser Tcp_option_parser(packet_in b, out Tcp_option_stack vec) { | |||
| } | |||
| ~End P4Example | |||
|
|
|||
| Two header unions can be compared for equality (==) or inequality (!=) | |||
| if they have the exact same type. The unions are equal if and only if | |||
| all their corresponding fields are equal. | |||
There was a problem hiding this comment.
Only one field in a union can be "hot" so the "all" seems potentially confusing.
p4-16/spec/P4-16-spec.mdk
Outdated
| @@ -3620,6 +3635,10 @@ parser Tcp_option_parser(packet_in b, out Tcp_option_stack vec) { | |||
| } | |||
| ~End P4Example | |||
|
|
|||
| Two header unions can be compared for equality (==) or inequality (!=) | |||
| if they have the exact same type. The unions are equal if and only if | |||
There was a problem hiding this comment.
Should we use the same phrasing as for structs here? (i.e., "same type and fields can be recursively...") I'm not sure why this is "exact same type."
There was a problem hiding this comment.
Perhaps this is opening a can of worms by asking this question, but it seems that most places in P4_16 use what I think @jnfoster called "structural typing", which I believe means that two types are considered "compatible" for purposes of comparison, assignment, and passing as a run-time parameter, if they have the same number of sub-elements, in the same relative order, and recursively those also have compatible types. For example, the existing spec and open source tools treat any values typedef'd with different names, but with the same bit<17> underlying type, as compatible.
This is as opposed to "nominal typing", I think, where such an assignment between two typedef'd values with different typedef names, but the same underlying bit<17> type, would require an explicit cast or it would be a type error?
If I've got those definitions at least approximately correct, then it sounds like @jnfoster's comment here is that "exact same type" is misleading, if structural typing is allowed for determining whether two values can be compared via == or !=. They don't have to be the exact same type, just structurally the same.
There was a problem hiding this comment.
typedef just introduces another name for the same type, but the type is essentially the same.
This is like C.
Everywhere we require types to be the same, not structurally the same.
There was a problem hiding this comment.
So is there a reason that one of these proposed additions says "exact same types", but the others do not?
|
This is ready for re-review. |
p4-16/spec/P4-16-spec.mdk
Outdated
| Two header stacks can be compared for equality (==) or inequality (!=) | ||
| only if they have the same element type and the same length. Two | ||
| stacks are equal if and only if all their corresponding elements are | ||
| equal. |
There was a problem hiding this comment.
Actually, one comment here. Do we want to explicitly say that the 'nextIndex' hidden state that exists at least during parsing time is not relevant for equality comparisons between header stacks? I know that is implicit in the way it is worded here, but explicit might be better.
There was a problem hiding this comment.
This makes me realize that the situation is pretty ugly: since assignment is supposed to preserve the nextIndex field, this means that this should field should be even preserved between architectural blocks. So the nextIndex left over in the ingress parser could be accessed in the egress parser!
It probably should not be used for comparisons, although one could argue either way.
There was a problem hiding this comment.
In PSA, the only way to preserve an already-parsed header from ingress to egress would be to carry the header in user-defined metadata. Kind of an odd choice, but I guess not currently disallowed by the architecture spec.
Not sure if there would be any desired use cases for preserving this kind of thing, but I guess you are worrying about the general case of everything possible (unless it has been explicitly prohibited).
There was a problem hiding this comment.
Have you considered the idea of allowing constructs that are not in the base P4_16 language spec in the intermediate representations that you can spit out in the compiler? That would allow you to explicitly define a my_hdr_stack_variable.nextIndex field in that slightly extended language, even if it is not available to P4_16 developers.
Or just go for it and make such a field part of the language explicitly, available everywhere, but not for P4_16 programmer convenience, but because it is helpful in some compilers?
No description provided.