Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add check of uniqueness of control plane names #3228

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dmatousek
Copy link
Contributor

Control plane names are not checked for uniqueness. It may lead to various issues in subsequent passes when uniqueness is assumed.

@jafingerhut
Copy link
Contributor

By any chance, is this intended to address an issue like the one described in this issue? #2755

One of the example programs on that issue has identical @name annotations on two different actions, which seems to me like a bug to accept, but note that internal passes in open source p4c create this situation by design right now (which I personally also think is a mistake, and would be better to avoid, but it has been like this for years).

@mihaibudiu
Copy link
Contributor

See also #3037, which didn't work.

@jafingerhut
Copy link
Contributor

jafingerhut commented Apr 25, 2022

I tried building p4c with the proposed changes on this PR after 3 commits, and ran it locally on my system.

It fails for a very simple P4 program with 2 tables, both of which have NoAction in their actions list. You should be able to reproduce this with the following commands:

$ git clone https://github.com/jafingerhut/p4-guide
$ cd p4-guide/name-annotations/v1model
$ p4c --target bmv2 --arch v1model --p4runtime-files actions-5-no-annot.p4info.txt actions-5-no-annot.p4 
/usr/local/share/p4c/p4include/core.p4(70): [--Werror=duplicate] error: .NoAction: conflicting control plane name: .NoAction
action NoAction() {}
       ^^^^^^^^
/usr/local/share/p4c/p4include/core.p4(70)
action NoAction() {}
       ^^^^^^^^

As far as the original source code given to the compiler, there are no name conflicts at all.

The error messages occur, I believe, because of the way that the p4c LocalizeAllActions frontend pass duplicates actions in the IR when the same action is an action of multiple P4 tables, and automatically creates identical @name annotations on these duplicated actions.

See #2755 for additional notes and details. I would love to have a meeting of the minds of P4 Runtime API folks, compiler folks, etc. to see what can or should be done about this. I have ideas described on that issue, but not sure what the best way to go here is.

@dmatousek
Copy link
Contributor Author

Thanks for your feedback and referencing related issues. I agree that there is no clear fix and it has to be further discussed.

@jafingerhut
Copy link
Contributor

@dmatousek I have scheduled a meeting with a few Intel folks knowledgeable about the P4 compiler to discuss a possible way forward on issue #2755. I will update that issue when/if there is any news to share.

@jfingerh
Copy link

@dmatousek I spoke with some folks at Intel including @hanw about the underlying issue here, and he had a suggestion for an experiment to try: put the @name conflict checking somewhere in the front end before the LocalizeAllActions pass, which is one that we know intentionally creates copies of actions with identical @name annotations. If there is a @name conflict before that pass, it should be one that the P4 developer themselves wrote.

The only disadvantage I can think of to such an approach is that now P4_16 source code produced as the output of passes after LocalizeAllActions could cause errors when used as input to p4c. I do not know how serious that is, but perhaps having a command line option that skips the new @name conflict check would help with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
control-plane Topics related to the control-plane or P4Runtime.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants