Add duplicate action control plane name check pass #4951

jafingerhut · 2024-10-08T16:43:17Z

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 @name annotations 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:

https://docs.google.com/presentation/d/1JqQciG8hWdd2UlbXbDENw36yfGGRk2LugjzRrbv6Sf4/edit?usp=sharing

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 @name annotations. Unless we change that behavior, any checks for duplicate @name annotations 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:

p4c should prohibit different controllable entities having the same control-plane name #4651
- I have re-read this issue after commit 24 on this PR, and this PR should fully address it. I have invited kfcripps to review this PR. If this PR is merged, I believe issue 4651 should be closed, since the issue will be fixed.
Incorrect P4Info generation when different actions have same name annotation #2755
- I have re-read issue 2755 after commit 24 on this PR, and this PR should fully address it. If this PR is merged, I believe issue 2755 should be closed, since the issue will be fixed.
Compiler silently allows duplicate name annotations #1949
- I have re-read this issue after commit 24 on this PR, and this PR should fully address that issue. I have re-tested the one test program that gave an erroneous P4Info output file attached to a comment on that issue, and with this PR's changes that program gives a clear error message about the duplicate @name annotations if you attempt to generate P4Info from it. There is a similar P4 program in this PR, so no need to add that particular test program to this PR. If this PR is merged, I believe issue 2755 should be closed, since the issue will be fixed.

The 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:

Is this the intended effect of @name annotations when generating P4Info files? #2716
- I have re-read this issue after commit 24 on this PR, and I believe I should not have linked to it from this PR at all. The questions raised in issue 2716 are definitely related to name annotations and how they affect the contents of P4Info files, but if there are any bugs there, this PR is not attempting to change the contents of correctly-generated P4Info files at all -- it is only attempting to give errors when P4Info files should not be generated.

I 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:

Control-plane name clash introduced by LocalizeAllActions and RemoveActionParameters #2749

asl · 2024-10-08T17:45:49Z

frontends/p4/duplicateHierarchicalNameCheck.h

+
+namespace P4 {
+
+#define ERR_STR_DUPLICATED_NAME "conflicting control plane name"


Better use static constexpr std::string_view here.

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.

addressed in commit 17 by eliminating this definition, and just using the literal string in the one place it was used.

asl · 2024-10-08T17:46:04Z

frontends/p4/duplicateHierarchicalNameCheck.h

+class DuplicateHierarchicalNameCheck : public Transform {
+    std::vector<cstring> stack;
+    /// Used for detection of conflicting control plane names
+    std::map<cstring, const IR::Node *> annotatedNodes;


Suggested change

std::map<cstring, const IR::Node *> annotatedNodes;

string_map<const IR::Node *> annotatedNodes;

addressed in commit 17

asl · 2024-10-08T18:28:10Z

frontends/p4/duplicateHierarchicalNameCheck.cpp

+    if (annotation->name != IR::Annotation::nameAnnotation) return annotation;
+
+    cstring name = annotation->getName();
+    if (!name.startsWith(".")) {


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.

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.

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

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);

addressed in commit 26

@jafingerhut After #4971 I believe we can just do name = absl::StrCat(absl::StrJoin(stack, "."), name);

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.

jafingerhut · 2024-10-08T22:08:09Z

@ChrisDodd Does it sound normal to you that local variable declarations within a parser or control have @name annotations generated for them within the IR? I do not know which pass they are created in, but it is somewhere before the LocalizeActions frontend pass. They are not printed when you use the --dump and --toP4 command line options, but the method DuplicateHierarchicalNameCheck::postorder added by this PR is called for such local variable declarations, and does not take the return statement in the first line of that method.

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.

ChrisDodd · 2024-10-09T04:02:10Z

A higher-level question here -- does it never make sense for a user to use the same @name on different things? One situation that comes to mind is having slightly different actions in different tables that do the same logical thing -- it makes sense to give them the same name so that code that is adding the "same" action to multiple tables looks more consistent.

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 @name was to allow the user to specify a "cleaner" interface for the control plane to hide some complexity of the P4 program. Getting things wrong can lead to confusion, so it needs to be used with care.

ChrisDodd · 2024-10-09T04:06:52Z

@ChrisDodd Does it sound normal to you that local variable declarations within a parser or control have @name annotations generated for them within the IR? I do not know which pass they are created in, but it is somewhere before the LocalizeActions frontend pass. They are not printed when you use the --dump and --toP4 command line options, but the method DuplicateHierarchicalNameCheck::postorder added by this PR is called for such local variable declarations, and does not take the return statement in the first line of that method.

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.

I'm not sure why non-extern local variables would get/need @name annotations as they are not generally visible to the control plane API. However, it might make sense to do that for debugging info purposes -- one could imagine a debugger being able to inspect whatever resource these locals are allocated to and want to be able to map it back to the original name in the source code -- particularly in the presence of control/parser inlining and other transformations.

jafingerhut · 2024-10-09T04:36:24Z

A higher-level question here -- does it never make sense for a user to use the same @name on different things? One situation that comes to mind is having slightly different actions in different tables that do the same logical thing -- it makes sense to give them the same name so that code that is adding the "same" action to multiple tables looks more consistent.

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 @name was to allow the user to specify a "cleaner" interface for the control plane to hide some complexity of the P4 program. Getting things wrong can lead to confusion, so it needs to be used with care.

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 same_name_for_table_and_action.p4.

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 --excludeFrontendPasses DuplicateHierarchicalNameCheck. I am certainly open to the idea of adding a new shorter command line option that achieves the same effect.

Signed-off-by: Andy Fingerhut <andy.fingerhut@gmail.com>

…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 <andy.fingerhut@gmail.com>

Signed-off-by: Andy Fingerhut <andy.fingerhut@gmail.com>

Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu> Signed-off-by: Andy Fingerhut <andy.fingerhut@gmail.com>

Signed-off-by: Andy Fingerhut <andy.fingerhut@gmail.com>

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 <andy.fingerhut@gmail.com>

and for some others, update the error messages we expect to see. Signed-off-by: Andy Fingerhut <andy.fingerhut@gmail.com>

Signed-off-by: Andy Fingerhut <andy.fingerhut@gmail.com>

jafingerhut · 2024-10-09T05:11:55Z

Another idea: This new pass could be enabled only if P4Info files were being generated.

Signed-off-by: Andy Fingerhut <andy.fingerhut@gmail.com>

jafingerhut · 2024-10-09T16:07:36Z

One situation that comes to mind is having slightly different actions in different tables that do the same logical thing -- it makes sense to give them the same name so that code that is adding the "same" action to multiple tables looks more consistent.

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 @name annotation of the original. I have no problem preserving this behavior, and this PR does preserve that behavior. The new pass added in this PR is executed before the LocalizeAllActions pass, specifically because I was trying to preserve that behavior. This new error-checking pass should always be done before any pass that duplicates actions, tables, etc. and their @name annotations whenever it is used. See the block comment in file duplicateHierarchicalNameCheck.h of this PR to see if it says this clearly and explicitly enough. I am happy to change it if it is not clear enough.

However, what if someone writes a P4 program with two very different actions, and puts the same @name annotation on both of them? In that situation, it seems to me to make 0 sense to call those by the same name.

What if a developer creates two actions with the same @name annotation, but believes they both have the same logical behavior, and are interchangeable? It doesn't make sense to me to implement a check in p4c that verifies whether those two actions actually are logically equivalent. If the compiler does not implement such an equivalent-behavior check, and the actions are different, and p4c treats them as logically the same, I suppose you can call that a place where the P4 developer shot themselves in the foot. But is this a use case we believe is useful to support? If so, then I am pretty sure they cannot use the P4Runtime API, and must use some other control plane API (which is fine, but out of scope of this PR, I think).

As a concrete example, see actions with P4 program names a1 and a2 in this program, that both have the same @name annotation: https://github.com/jafingerhut/p4-guide/blob/master/name-annotations/actions-5-same-name-annot.p4

Actions a1 and a2 are not identical. They have different behaviors.
If you compile this program with the latest p4c, it creates an incorrect P4Info file with only one action named apiname1, used by both tables t1 and t2. There are no errors, nor any warning messages, to alert you to this bug.
It has been this way for at least 5 years.

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 <andy.fingerhut@gmail.com>

jafingerhut · 2024-10-09T17:43:55Z

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 <andy.fingerhut@gmail.com>

…rchical-name-check-pass

Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>

…om:jafingerhut/p4c into add-duplicate-hierarchical-name-check-pass

…rchical-name-check-pass

Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>

jafingerhut · 2024-10-20T15:57:30Z

@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 p4test myprog.p4 on a program with duplicate @name annotations, whether they are on actions or anywhere else.

…rchical-name-check-pass

Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>

asl · 2024-10-21T04:45:35Z

frontends/p4/duplicateHierarchicalNameCheck.cpp

+    bool foundDuplicate = false;
+    auto *otherNode = annotatedNode;
+    if (annotatedNode->is<IR::P4Table>()) {
+        if (annotatedTables.count(name)) {


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 <andy_fingerhut@alum.wustl.edu>

…rchical-name-check-pass

Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>

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 <andy_fingerhut@alum.wustl.edu>

Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>

kfcripps · 2024-10-24T01:31:27Z

#4651
I have re-read this issue after commit 24 on this PR, and this PR should fully address it. I have invited kfcripps to review this PR. If this PR is merged, I believe issue 4651 should be closed, since the issue will be fixed.

@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 · 2024-10-24T03:48:52Z

#4651
I have re-read this issue after commit 24 on this PR, and this PR should fully address it. I have invited kfcripps to review this PR. If this PR is merged, I believe issue 4651 should be closed, since the issue will be fixed.

@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),

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

does give an error message if you run p4test or p4c with one or more command line options that generate P4Info files, because there is no correct way to generate a P4Info file for such a program.
does not give any error if you run p4c or p4test without generating P4Info files, because p4c and p4test do not generate erorrs if tables have equal @name annotations, when you do not generate P4Info files. This has been the the behavior for a long time now, and I was a bit worried about changing it. This PR preserves that behavior, even if you consider it a bug. In this case, pretty much all @name annotations in the program are ignored.

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.

as well as the "bug" case mentioned in #4651 (comment)?

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 @name conflicts that the user specfies, not one that LocalizeAllActions creates. If I added such a check after LocalizeAllActions, then many, many useful and legal P4 programs with correct @name annotations would stop compiling and start giving errors, ones that the developer did not cause.

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.

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 · 2024-10-24T16:38:38Z

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

jafingerhut · 2024-10-24T19:23:29Z

Closing this PR. Please review #4975 instead, which replaces this one and fixes one more thing.

jafingerhut mentioned this pull request Oct 8, 2024

Add check of uniqueness of control plane names #3228

Open

jafingerhut marked this pull request as draft October 8, 2024 16:49

jafingerhut force-pushed the add-duplicate-hierarchical-name-check-pass branch from 6279f27 to 2961cb3 Compare October 8, 2024 16:53

asl reviewed Oct 8, 2024

View reviewed changes

fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Oct 8, 2024

jafingerhut and others added 10 commits October 8, 2024 21:54

Add pass to give error if duplicate hierarchical names are present

e429c8a

Signed-off-by: Andy Fingerhut <andy.fingerhut@gmail.com>

Add new source files to CMakeLists.txt

39538bf

Signed-off-by: Andy Fingerhut <andy.fingerhut@gmail.com>

Skip new checks for P4-14 source files in run-bmv2-test.py

a2792ce

Signed-off-by: Andy Fingerhut <andy.fingerhut@gmail.com>

Fix lint error

a553fe1

Signed-off-by: Andy Fingerhut <andy.fingerhut@gmail.com>

Fix lint problems.

26f0501

Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu> Signed-off-by: Andy Fingerhut <andy.fingerhut@gmail.com>

Do not give errors for variable declarations with identical names

da456fe

Signed-off-by: Andy Fingerhut <andy.fingerhut@gmail.com>

Limit search for duplicate names to actions, tables, or "other objects"

b683571

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 <andy.fingerhut@gmail.com>

Update test programs that had errors in their name annotations

ad40fe9

and for some others, update the error messages we expect to see. Signed-off-by: Andy Fingerhut <andy.fingerhut@gmail.com>

Update expected output files for modified test programs

91166c8

Signed-off-by: Andy Fingerhut <andy.fingerhut@gmail.com>

jafingerhut force-pushed the add-duplicate-hierarchical-name-check-pass branch from ccfca0a to 91166c8 Compare October 9, 2024 04:54

jafingerhut added 2 commits October 8, 2024 22:01

Move new test program with error into p4_16_errors directory

9acb7e0

Signed-off-by: Andy Fingerhut <andy.fingerhut@gmail.com>

Add expected output files for new test programs

b133b28

Signed-off-by: Andy Fingerhut <andy.fingerhut@gmail.com>

jafingerhut added 3 commits October 9, 2024 07:57

More fine-tuning to enable tests to pass

76bf24f

Signed-off-by: Andy Fingerhut <andy.fingerhut@gmail.com>

Fix lint error

f7ec2fe

Signed-off-by: Andy Fingerhut <andy.fingerhut@gmail.com>

Fix lint error

1cc8224

Signed-off-by: Andy Fingerhut <andy.fingerhut@gmail.com>

jafingerhut added 2 commits October 9, 2024 09:22

Merge branch 'master' into add-duplicate-hierarchical-name-check-pass

4b03eb5

Address some review comments

93daba5

Signed-off-by: Andy Fingerhut <andy.fingerhut@gmail.com>

jafingerhut added the run-validation Use this tag to trigger a Validation CI run. label Oct 9, 2024

jafingerhut added 2 commits October 9, 2024 15:01

Rewording documentation of new pass for clarity.

66a0a0a

Signed-off-by: Andy Fingerhut <andy.fingerhut@gmail.com>

Merge remote-tracking branch 'upstream/main' into add-duplicate-hiera…

083db34

…rchical-name-check-pass

jafingerhut mentioned this pull request Oct 18, 2024

Add check for duplicate @name annotations written by the P4 developer p4lang/project-ideas#1

Open

jafingerhut added 5 commits October 20, 2024 04:42

Enable the new pass only if P4Runtime control plane API gen is enabled

99beb30

Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>

Merge branch 'add-duplicate-hierarchical-name-check-pass' of github.c…

2b7c58a

…om:jafingerhut/p4c into add-duplicate-hierarchical-name-check-pass

Merge remote-tracking branch 'upstream/main' into add-duplicate-hiera…

54007df

…rchical-name-check-pass

Undo changes to Python test scripts

d89f214

Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>

Undo changes to gtestp4c source

7424db3

Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>

Merge remote-tracking branch 'upstream/main' into add-duplicate-hiera…

55ad18f

…rchical-name-check-pass

jafingerhut marked this pull request as ready for review October 20, 2024 16:19

jafingerhut mentioned this pull request Oct 20, 2024

p4c should prohibit different controllable entities having the same control-plane name #4651

Open

jafingerhut added 2 commits October 20, 2024 20:07

Use abseil StrCat and StrAppend instead of loop

799448a

Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>

Fix formatting with clang-format

377f41a

Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>

asl reviewed Oct 21, 2024

View reviewed changes

jafingerhut added 3 commits October 21, 2024 17:36

Also catch duplicates involving top-level actions without @name annot…

d265e1c

…ations Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>

Merge remote-tracking branch 'upstream/main' into add-duplicate-hiera…

606300e

…rchical-name-check-pass

Add new test program to exercise latest code changes

6bcb614

Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>

jafingerhut changed the title ~~Add duplicate hierarchical name check pass~~ Add duplicate action control plane name check pass Oct 21, 2024

jafingerhut added 2 commits October 21, 2024 18:51

Fix lint warning, and add one more successful test program

d67df83

Signed-off-by: Andy Fingerhut <andy_fingerhut@alum.wustl.edu>

kfcripps requested a review from oleg-ran-amd October 23, 2024 20:46

jafingerhut mentioned this pull request Oct 24, 2024

Combo of duplication action name check and LocalizeAllActions fix #4975

Open

jafingerhut closed this Oct 24, 2024

asl mentioned this pull request Oct 29, 2024

Add abseil string helpers #4971

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add duplicate action control plane name check pass #4951

Add duplicate action control plane name check pass #4951

jafingerhut commented Oct 8, 2024 •

edited

Loading

asl Oct 8, 2024

jafingerhut Oct 8, 2024 •

edited

Loading

jafingerhut Oct 9, 2024

asl Oct 8, 2024

jafingerhut Oct 9, 2024

asl Oct 8, 2024

jafingerhut Oct 9, 2024

jafingerhut Oct 20, 2024

asl Oct 20, 2024 •

edited

Loading

jafingerhut Oct 21, 2024

asl Oct 29, 2024

jafingerhut Oct 29, 2024

jafingerhut commented Oct 8, 2024 •

edited

Loading

ChrisDodd commented Oct 9, 2024

ChrisDodd commented Oct 9, 2024

jafingerhut commented Oct 9, 2024

jafingerhut commented Oct 9, 2024

jafingerhut commented Oct 9, 2024 •

edited

Loading

jafingerhut commented Oct 9, 2024

jafingerhut commented Oct 20, 2024

asl Oct 21, 2024

kfcripps commented Oct 24, 2024 •

edited

Loading

jafingerhut commented Oct 24, 2024 •

edited

Loading

jafingerhut commented Oct 24, 2024

jafingerhut commented Oct 24, 2024


		namespace P4 {

		#define ERR_STR_DUPLICATED_NAME "conflicting control plane name"

	std::map<cstring, const IR::Node *> annotatedNodes;
	string_map<const IR::Node *> annotatedNodes;

Add duplicate action control plane name check pass #4951

Add duplicate action control plane name check pass #4951

Conversation

jafingerhut commented Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

jafingerhut Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asl Oct 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jafingerhut commented Oct 8, 2024 • edited Loading

ChrisDodd commented Oct 9, 2024

ChrisDodd commented Oct 9, 2024

jafingerhut commented Oct 9, 2024

jafingerhut commented Oct 9, 2024

jafingerhut commented Oct 9, 2024 • edited Loading

jafingerhut commented Oct 9, 2024

jafingerhut commented Oct 20, 2024

Choose a reason for hiding this comment

kfcripps commented Oct 24, 2024 • edited Loading

jafingerhut commented Oct 24, 2024 • edited Loading

jafingerhut commented Oct 24, 2024

jafingerhut commented Oct 24, 2024

jafingerhut commented Oct 8, 2024 •

edited

Loading

jafingerhut Oct 8, 2024 •

edited

Loading

asl Oct 20, 2024 •

edited

Loading

jafingerhut commented Oct 8, 2024 •

edited

Loading

jafingerhut commented Oct 9, 2024 •

edited

Loading

kfcripps commented Oct 24, 2024 •

edited

Loading

jafingerhut commented Oct 24, 2024 •

edited

Loading