p4c
p4c copied to clipboard
p4lang
Structs, headers and tuples initialization/assignment support (#762) …
…(#2017)
Struct, header or tuple can be initialized automatically with a default value of
the suitable type using the syntax ... or using a mix of explicit values and
default values by using the notation ... in a list expression
(and struct expression for structs and headers).
Syntax { ... } and {/* expressionList | kvList */, ... } can also be used in assignments.
Default values are as proposed in p4-spec pull request #797.
Header stacks and varbits are not supported as a leaf fields of structs nor elements of tuples if using this kind of initialization/assignment, because in current version of spec there are no constructs to initialize these types. If they are used as a leaf fields compiler will report an error.
If header union is used as an element of tuple, compiler will produce warning that temporary header union variable used for the element initialization may not be completely initialized, despite setting it invalid.
Frontnend reorganized, Struct initializer now processes declaration initializers along with assignment statemants, which is the cause of changes in expected outputs of some tests.
Before I review this I want to mention that during the last design working group we have briefly discussed the possibility of using {} to indicate an invalid header. If we did adopt this syntax (or some other equivalent syntax) it could probably simplify this PR...
Hi @mbudiu-vmw , I applied requested changes. Please leave a comment if you want anything more from me in order to merge this PR.
Hi @marko-stanoj, this is the ONF bot 🤖 I'm glad you want to contribute to our projects! However, before accepting your contribution, we need to ask you to sign a Contributor License Agreement (CLA). You can do it online, it will take only a few minutes:
✒️ 👉 https://cla.opennetworking.org
After signing, make sure to add your Github user ID marko-stanoj to the agreement.
For more information or help:" https://wiki.opennetworking.org/x/BgCUI
![onf-cla-manager[bot] avatar](https://avatars.githubusercontent.com/in/52900?v=4 "Published by a pub.dev verified publisher"){kind=small}
Hi @marko-stanoj, this is the ONF bot 🤖 I'm glad you want to contribute to our projects! However, before accepting your contribution, we need to ask you to sign a Contributor License Agreement (CLA). You can do it online, it will take only a few minutes:
✒️ 👉 https://cla.opennetworking.org
After signing, make sure to add your Github user ID marko-stanoj to the agreement.
For more information or help:" https://wiki.opennetworking.org/x/BgCUI
250 commits will pollute the history, can you please rebase on main in the recommended way instead? https://github.com/p4lang/p4c/tree/main/docs#git-usage
@mbudiu-vmw Yes, I made a mistake, sorry, I'm currently working on that.
Hi @marko-stanoj, this is the ONF bot 🤖 I'm glad you want to contribute to our projects! However, before accepting your contribution, we need to ask you to sign a Contributor License Agreement (CLA). You can do it online, it will take only a few minutes:
✒️ 👉 https://cla.opennetworking.org
After signing, make sure to add your Github user ID marko-stanoj to the agreement.
For more information or help:" https://wiki.opennetworking.org/x/BgCUI
This PR is now in accordance with the main and a test for using this initialization in operations is added. Leave a comment if more change is needed.
Hi @mbudiu-vmw, I applied the requested changes. For operation relations, I left it to be an unsupported operation. For now, default initializations are implemented as separate assignments. Because of the use in arguments and return statements, it's a repeated pass. If a literal for an invalid header were to be introduced, implementation would be more consistent with the current, not default initialization. Please leave a comment if more changes are needed.
Hi @mbudiu-vmw, I applied the requested changes but I don't know why my tests failed on validation.
These validation failures should normally be caught, I will take care of this.
Hi @mbudiu-vmw. All checks are passing now, is there anything more you want me to do in order to merge this solution?
I tested this PR, the version after there were 5 commits, on 2022-Feb-26. From the language spec, it gives these two examples in Section 8.23 "Initializing with default values" (https://p4.org/p4-spec/docs/P4-16-v1.2.2.html#sec-initializing-with-default-values):
H h1 = ...; // initialize h1 with a header that is invalid
H h3 = { ... }; // initialize h3 with a header that is valid, field f1 0, field f2 0
With this PR's changes, both h1 and h3 are made invalid, but h3 should be made valid, with both fields equal to 0.
This PR also gives errors when attempting to do the following initializations from the examples in the spec:
N0 n0 = ...; // initialize n0 with the default value 0
N1 n1 = ...; // initialize n1 with the default value N1.A
T t0 = ...; // initialize t0 with the value { { 0, false }, 0, N1.A }
T t1 = { s = ..., ... }; // initialize t1 with the value { { 0, false }, 0, N1.A }
T t2 = { s = ... }; // error: no initializer specified for fields n0 and n1
I would guess that all of these errors are because this PR does not yet handle enums, neither the serializable nor the non-serializable kinds?
Hi @jfingerh. The problem is that ... and {...} calls are different just in case of a header initialization, in other cases like a struct or tuple initialization or in the future for enums, these two calls act the same. Is it okay to have this inconsistency?
.. and {...} are implemented as ListExpressions. I can add a new variable to distinguish these two expressions and implement it differently when used in headers. But, if an invalid header literal is to be introduced is there a need for ... to do the same thing?
I suggest that ... and {...} do the same thing (set all header fields to default values) and when an invalid literal is introduced use it to set headers to invalid.
Yes, you are right. This PR does not yet handle enums, neither the serializable nor the non-serializable kinds. It just handles structs, headers, and tuples default initialization, and if their fields are of type enum, serializable or non-serializable kind, it sets them to default values.
@Jovana-Syrmia wrote in an earlier comment:
.. and {...} are implemented as ListExpressions. I can add a new variable to distinguish these two expressions and implement it differently when used in headers. But, if an invalid header literal is to be introduced is there a need for ... to do the same thing?
I suggest that ... and {...} do the same thing (set all header fields to default values) and when an invalid literal is introduced use it to set headers to invalid.
My response:
The current P4_16 language specification currently says that ... should initialize a header to invalid, and {...} should initialize a header to valid, with default values for all fields.
As far as how this might best be implemented in p4c, I defer to @mbudiu-vmw for ideas there, as I am fairly ignorant of the p4c compiler IR. If he agrees with your idea that ... assigned to a header is the same as assigning {...} to a header (i.e. both make the header valid), then we should change the P4_16 language spec to match what is implemented in p4c at some point in the future.
It has not yet been decided if there will be a literal syntax for an invalid header, and if so, what that syntax will be. See this issue open for the language spec: https://github.com/p4lang/p4-spec/issues/1021
We don't need to have the enums as part of this PR, should be easy to add later, but @Jovana-Syrmia will have to fix the problem with ... for headers. I apologize for not having noticed this.
@marko-stanoj @mbudiu-vmw @jnfoster I would be interested in discussing this PR, or any PR for p4c that would lead us towards ... syntax being implemented according to the P4 language spec.
Marko, do you know which of these two PRs you consider more well developed at this time? Do you consider either of them obsolete? Were you being paid to work on this PR, Marko, and are no longer being paid to work on it? Or have other reasons to no longer be interested in working on it?
- https://github.com/p4lang/p4c/pull/2224
- https://github.com/p4lang/p4c/pull/2368
Would any or all of you be interested in discussing this at the next P4.org open source developer days meeting, scheduled for 11am-noon Pacific time on Tuesday, October 25, 2022? Details for joining the Zoom meeting are available on this calendar of events: https://p4.org/working-groups/
If that time is inconvenient, let me know and we can try to arrange a different time.
@mbudiu-vmw I understand, which is why I created this issue: https://github.com/p4lang/p4-spec/issues/1021
It might have been discussed once, but it led to a discussion that perhaps we should simply use ... for that. I am perfectly OK to create a different syntax that is a literal for an invalid header -- we just need to pick one and move forward.
The way forward is for me to make up a syntax for invalid headers and perhaps stacks too and implement that. Then we can see it works and we can just argue about what the syntax should be.
@mbudiu-vmw I did a quick check, and the string "invalid_header" does not appear anywhere in any of the p4c P4_16 test programs, except inside of a string @noWarn("invalid_header"), so invalid_header as a literal syntax for a value that is an invalid header would not cause any conflicts with existing names used in existing compiler test programs. I am fine with other proposed literal syntax for experimentation purposes, too -- just throwing in my 2 cents.