Add duplicate action control plane name check pass #4951
Conversation
jafingerhut
force-pushed
the
add-duplicate-hierarchical-name-check-pass
branch
from
6279f27
to
2961cb3
Compare
|
|
||
| namespace P4 { | ||
|
|
||
| #define ERR_STR_DUPLICATED_NAME "conflicting control plane name" |
There was a problem hiding this comment.
Better use static constexpr std::string_view here.
There was a problem hiding this comment.
Thanks for all the suggestions. I plan to address your suggestions, but I may wait to make the required changes until after I have first figured out how to make all the tests pass.
There was a problem hiding this comment.
addressed in commit 17 by eliminating this definition, and just using the literal string in the one place it was used.
| class DuplicateHierarchicalNameCheck : public Transform { | ||
| std::vector<cstring> stack; | ||
| /// Used for detection of conflicting control plane names | ||
| std::map<cstring, const IR::Node *> annotatedNodes; |
There was a problem hiding this comment.
| std::map<cstring, const IR::Node *> annotatedNodes; | |
| string_map<const IR::Node *> annotatedNodes; |
There was a problem hiding this comment.
addressed in commit 17
| if (annotation->name != IR::Annotation::nameAnnotation) return annotation; | ||
|
|
||
| cstring name = annotation->getName(); | ||
| if (!name.startsWith(".")) { |
There was a problem hiding this comment.
Probably it would be better to use absl::StrJoin here, something like this:
absl::StrCat(absl::StrJoin(stack, "."), name.string_view()). Though this might break up as we'd need to explicitly cast from cstring to string_view, then we'd need to provide lambda to StrJoin.
There was a problem hiding this comment.
I would like to address this comment, but my C++-fu is pretty weak these days, even with your code snippet and other tips. If you or someone else has the time to provide a specific working replacement, I am happy to use it.
There was a problem hiding this comment.
@asl As of commit 25 on this PR, I have tried to address 2 of your 3 comments, and I think it is ready for a fresh review. If you have more specific suggestions for code changes on this commentt, which I have not addressed as mentioned in my comment above, please let me know.
There was a problem hiding this comment.
So, does the following work?
name = absl::StrCat(absl::StrJoin(stack, ".", [](std::string *out, cstring s) {
absl::StrAppend(out, s.string_view());
}), name.string_view())Yes, we have not finished the cstring-related implicit conversion saga (I need to find another continuous chunk of time to convert everything...), so we cannot make cstring implicitly converted to string_view (yet), so these .string_view() calls are sadly necessary.
After the conversion it would be much nicer:
name = absl::StrCat(absl::StrJoin(stack, "."), name);There was a problem hiding this comment.
addressed in commit 26
There was a problem hiding this comment.
@jafingerhut After #4971 I believe we can just do name = absl::StrCat(absl::StrJoin(stack, "."), name);
There was a problem hiding this comment.
I will try to keep an eye out for when that PR is merged in, but feel free to ping me when it is, in case I miss it.
|
@ChrisDodd Does it sound normal to you that local variable declarations within a parser or control have I do want this new check to be performed for extern instantiations and their names, since many of those have control plane APIs and thus should have unique global names. But if it is a local variable with a non-extern type, I cannot think of any reason it should be visible in a control plane API. |
fruffy
added
the
core
|
A higher-level question here -- does it never make sense for a user to use the same This is in line with what is going on with the compiler-generated names -- when the compiler is splitting a logical table into multiple tables, the user does not want to need to be aware of that and need to use different names for things as a result. If it would make sense in some situations, maybe the duplicate names should be a warning rather than an error? The original concept for |
I'm not sure why non-extern local variables would get/need |
The P4Runtime API is certainly not the only control plane API in existence, but taking it as one prominent example, it has built into its data model that every action name must be unique. If, beneath the covers of a network device's implementation of the P4Runtime API, it combines that action name with a table to decide what exact implementation to use for that particular table, that is an implementation detail for that target, and perfectly acceptable. It does not affect the P4Runtime API specification, nor the messages passed between client and server. The P4Runtime API specification could be modified to support multiple actions with the same name, but I'm not aware of any desire by its users to change it in that way. The P4Runtime API spec also requires that every table name must be unique. It actually permits a table and an action to have the same name as each other, and the p4c implementation currently allows this, and there is code in this PR to preserve that capability. I was reminded of this because there is one particular test program that exercises this possibility, named It might definitely be the case that some other control plane API does not have such restrictions. As of the current state of this PR, you can skip its new checks by skipping the new pass, via command line options |
Signed-off-by: Andy Fingerhut <[email protected]>
Signed-off-by: Andy Fingerhut <[email protected]>
…ce files that are the output of the midend pass of the compiler, because they can include intentional name duplication created in the LocalizeActions pass. + P4-16 source files created by translating a P4-14 source file to P4-16. Signed-off-by: Andy Fingerhut <[email protected]>
Signed-off-by: Andy Fingerhut <[email protected]>
Signed-off-by: Andy Fingerhut <[email protected]>
Signed-off-by: Andy Fingerhut <[email protected]> Signed-off-by: Andy Fingerhut <[email protected]>
Signed-off-by: Andy Fingerhut <[email protected]>
Explicitly allow a table and action to have the same name, since there is a test case that was written explicitly to test that this is permitted. Signed-off-by: Andy Fingerhut <[email protected]>
and for some others, update the error messages we expect to see. Signed-off-by: Andy Fingerhut <[email protected]>
Signed-off-by: Andy Fingerhut <[email protected]>
jafingerhut
force-pushed
the
add-duplicate-hierarchical-name-check-pass
branch
from
ccfca0a
to
91166c8
Compare
Signed-off-by: Andy Fingerhut <[email protected]>
Signed-off-by: Andy Fingerhut <[email protected]>
|
Another idea: This new pass could be enabled only if P4Info files were being generated. |
Signed-off-by: Andy Fingerhut <[email protected]>
Signed-off-by: Andy Fingerhut <[email protected]>
Signed-off-by: Andy Fingerhut <[email protected]>
So if the compiler takes one action written by the P4 developer, and creates two or more specialized versions of it, then modulo bugs in the compiler I can believe that these two specializations will have the same logical behavior according to the P4 language spec, and it makes perfect sense to use the same name for them from the control plane API. Note: This is exactly what happens today with the LocalizeAllActions pass - single actions are duplicated, and all have a copy of the However, what if someone writes a P4 program with two very different actions, and puts the same What if a developer creates two actions with the same As a concrete example, see actions with P4 program names
In my opinion, this program should be rejected. I cannot think of any reason why we should accept it if you want to generate P4Info from it. Even if some future control plane API always uses (table name, action name) pairs to disambiguate actions, I would bet $100 that there is some bug in p4c in this area that would require changes to make that work. I say: if such a control plane API arises, let the team developing it enhance p4c to support it. |
Signed-off-by: Andy Fingerhut <[email protected]>
|
FYI, since this change might cause issues for automated tests on the Tofino code base, I plan to hold off attempting to merge this code until after that team has published their work. The existing bugs this is trying to avoid have existed for 5 years -- we can wait a month or two longer. That and I would like to get careful review from anyone who might be affected by these changes, because the current behavior has been the way it is for so long. I am happy to try to write up some article explaining the reasons for the change, and why things are the way they are, if that is of interest for future reference. |
Signed-off-by: Andy Fingerhut <[email protected]>
…rchical-name-check-pass
Signed-off-by: Andy Fingerhut <[email protected]>
…om:jafingerhut/p4c into add-duplicate-hierarchical-name-check-pass
…rchical-name-check-pass
Signed-off-by: Andy Fingerhut <[email protected]>
Signed-off-by: Andy Fingerhut <[email protected]>
|
@ChrisDodd As of commit 24, I have modified this PR so that the new DuplicateHierarchicalNameCheck pass is only enabled if you give one or more of the command line options that cause P4Runtime API output files to be written. This makes these checks enabled at exactly the times that other duplicate @name annotation checks on tables, parser value sets, extern instances, etc. are enabled, too. With these changes, there are no errors if you simply run |
…rchical-name-check-pass
Signed-off-by: Andy Fingerhut <[email protected]>
Signed-off-by: Andy Fingerhut <[email protected]>
| bool foundDuplicate = false; | ||
| auto *otherNode = annotatedNode; | ||
| if (annotatedNode->is<IR::P4Table>()) { | ||
| if (annotatedTables.count(name)) { |
There was a problem hiding this comment.
You could probably save one map lookup here, e.g.:
auto [it, inserted] = annotatedTables.insert(name, annotatedNode);
if (!inserted) {
foundDuplicate = true;
otherNode = *it;
}Same for code below.
Probable the code could be deduplicated as well? Something like this:
auto checkForDuplicates = [&foundDuplicate, &otherNode](auto &namesToCheck, const IR::Node *annotatedNode) {
auto [it, inserted] = namesToCheck.insert(name, annotatedNode);
if (!inserted) {
foundDuplicate = true;
otherNode = *it;
}
}And then just call this lambda in each of this is checks:
if (annotatedNode->is<IR::P4Table>() {
checkForDuplicates(annotatedTables, annotatedNode);
} else () ...…ations Signed-off-by: Andy Fingerhut <[email protected]>
…rchical-name-check-pass
Signed-off-by: Andy Fingerhut <[email protected]>
jafingerhut
changed the title
Table and other P4 object control plane name conflicts are already checked for in the P4Info file generation code. Action control plane names are the only thing it does not do correctly. Signed-off-by: Andy Fingerhut <[email protected]>
Signed-off-by: Andy Fingerhut <[email protected]>
@jafingerhut I have not looked at any of the PR's changes so sorry if the answer to my below questions would be obvious by looking at them. Just to clarify, does this PR address both the "error" case mentioned in #4651 (comment), as well as the "bug" case mentioned in #4651 (comment)? Also, does the PR add both of these cases (or equivalent cases) as tests to testdata/p4_16_errors/, as well as testdata/p4_16_samples/, respectively? |
[jafingerhut] Yes, this is a case where the user's P4 program is trying to specify something incorrect. For a program like that, this PR:
There are several test cases added that cover variations on this, e.g. some with actions at top level, with and without name annotations, and some with actions within controls, with and without name annotations, because the implementation actually had some unique sub-bugs in some of those cases.
For that case, that is the behavior of pass LocalizeAllActions. The new pass this PR adds always runs before LocalizeAllActions, so that it only detects Aside: Without this PR's changes, or something like them, two actions with the same @name annotation might have been a mistake written by the P4 developer, or it might have been introduced by the LocalizeAllActions pass, or some mix of each. With something like this PR's changes, I think I can promise you that after LocalizeAllActions runs, if two actions have the same @name annotation, it is only because they were originally the same action in the original input P4 source code, but LocalizeAllActions duplicated it, because it was used in multiple tables, or a top-level action was used in multiple controls. That seems to me like a useful invariant to know and document. If you do not want the behavior of pass LocalizeAllActions, then I would recommend you try omitting pass LocalizeAllActions in your downstream version of p4c. I have not tested what happens if you omit this pass, but there might be later passes that assume LocalizeAllActions has already run. My understanding of the intent of LocalizeAllActions pass is that it intentionally makes multiple copies of actions in the IR, if those actions are used in multiple tables. This allows later compiler passes to optimize those copies independently of each other. If you omitted pass LocalizeAllActions, then even if that does not introduce any bugs due to assumptions made by later passes, you would be giving up on that opportunity for optimizing the same action used in multiple places of the original P4 program.
|
|
@kfcripps Last night I did a couple of experiments to see what happens if I comment out LocalizeAllActions pass in p4c/frontend/p4/frontend.cpp, and in some of the tests I comment out more than just that pass. I ran a full p4c test suite, and counted % of passed tests by "category", e.g. tests whose names begin with "p4/" are tallied separately with those beginning with "bmv2/". Results can be seen here: https://docs.google.com/spreadsheets/d/18l6s997BhNfV375LDBq4SNTdM7dW4XvuaZi3abX6CwE/edit?usp=sharing Note: I do not have a good understanding of the dependencies between the passes. I was just trying a quick experiment to see what happened. |
|
Closing this PR. Please review #4975 instead, which replaces this one and fixes one more thing. |
I have updated this initial comment with the status of the PR after 32 commits.
I recommend that we abandon this PR, and instead focus on #4975 instead. #4975 is intended to combine all of the fixes for #4951 and #4970. I thought #4970 might be merged quickly, and then #4951 would be after that, but there are some small test case dependencies between the two that it is nicer to do all at once. I will copy this initial big comment over to the first comment on #4975, as a summary of its status.
These changes are intended to address the problem where p4c currently allows a user to have identical
@nameannotations on multiple objects, e.g. on multiple actions. This can cause incorrect P4Info files to be generated, among other likely problems resulting from that root cause. See this slide deck for examples and more details:The code changes here are based upon this PR, which stalled a while ago: #3228
These changes are noticeably different from that PR, however, in that this one adds a new pass that occurs before the LocalizeAllActions pass, which is the pass where p4c by design can synthesize actions that are duplicates of user-written actions, all of which have identical
@nameannotations. Unless we change that behavior, any checks for duplicate@nameannotations must be done before that pass. Such a change is described verbally in this comment on the PR above: #3228 (comment) but to my knowledge has not been implemented before. This PR is trying to implement that approach.It passes all of the CI tests, after modifying a handful of P4 programs that had the error being checked for, so they no longer had the error. One of those was preserved to verify that p4c catches the error with the expected error messages.
The new pass is only enabled if one or more of the command line options are given to generate P4Info output files. The duplicate @name annotations checked for in the new pass only cause problems (that I am aware of) if P4Info output files are being generated. This is similar to some existing checks for duplicate names of P4 objects in the control plane serialization code for P4Info.
I think it would be best to avoid merging in this PR (even if the community finds it a good idea) until after the Tofino team has finished their work of publishing their code in open source. I would hate for an extra check like this to cause them an extra day or two of work in tweaking their automated tests.
There have been multiple related issues filed on p4c over several years that this PR might address, at least partially if not completely.
These issues I believe are fully fixed by this PR:
p4cshould prohibit different controllable entities having the same control-plane name #4651The issue below should remain open even if this PR is merged, as the issue is really asking questions about what names should be used for controllable P4 objects in a P4Info file. This PR does not change any names in P4Info files -- it only gives errors and prevents P4Info files from being generated when the current names would conflict:
@nameannotations when generating P4Info files? #2716I still need to re-read and think about whether the following issues are fixed by this PR, or whether there are other questions in them that are not fixed by this PR: