Tighten p4c for add-on-miss #3614

hesingh · 2022-10-24T21:38:50Z

This PR fixes the following issues.

p4c-dpdk allows calling add_entry from a hit function.
p4c-dpdk allows first arg to add_entry to be the name of a default_action.
p4c-dpdk allows first arg to add_entry to be the name of an action not used by the table.

jafingerhut · 2022-10-24T22:32:42Z

backends/dpdk/dpdkArch.cpp

+    }
+    if (use_add_on_miss) {
+        if (it == structure->defActs.end()) {
+            ::error(ErrorType::ERR_UNEXPECTED, "%1% is not inside a miss action: %2%:",


It seems good to have at least one test program that exercises detecting this error, and confirms that the desired error message is printed.

An existing tests does test the code. See p4c/testdata/p4_16_samples/pna-add-on-miss-err.p4.test

jafingerhut · 2022-10-24T22:33:14Z

backends/dpdk/dpdkHelpers.cpp

@@ -1123,6 +1123,12 @@ bool ConvertStatementToDpdk::preorder(const IR::MethodCallStatement *s) {
            }
            auto action = a->expr->arguments->at(0)->expression;
            auto action_name = action->to<IR::StringLiteral>()->value;
+            auto it = structure->defActs.find(action_name);
+            if (it != structure->defActs.end()) {
+                  ::error(ErrorType::ERR_UNEXPECTED, "%1%: action cannot be default action: %2%:",


Similarly for this error message.

A new P4 program is added by this PR to test above code. See p4c/testdata/p4_16_samples/pna-example-miss.p4

I see that test program. I also see the expected output files with suffixes .p4-stderr and .p4-error are both empty in this PR. I don't see how this test is passing if the error message is actually printed.

Perhaps consider adding the test programs that should always print compile-time errors in the p4_16_errors and p4_16_errors_outputs directory, instead of the 'samples' directories? I believe that is their intent.

In fact, the automated tests are failing for this test program, probably because the .p4-error and/or .p4-stderr files are empty, instead of containing the expected error messages. You can run the test script under the build directory with the -f command line option to cause the actual output files to be copied into the p4_16_samples_outputs directory of your local cloned copy of this repo, where you are developing these changes.

I have used -f for a while with p4c. For some reason -f is not writing output. Will try errors directory one more time.

$ dpdk/testdata/p4_16_samples/pna-example-addhit.p4.test -f ./p4c-dpdk --dump ./tmpqfb7g5hb -o ./tmpqfb7g5hb/pna-example-addhit.p4.spec --arch pna --bf-rt-schema ./tmpqfb7g5hb/pna-example-addhit.p4.bfrt.json /tmp/p4c/testdata/p4_16_samples/pna-example-addhit.p4 Error compiling pna-example-addhit.p4(143): [--Werror=unexpected] error: add_entry add-on-miss not configured or not called from default action: next_hop: add_entry(action_name="next_hop", action_params = tmp, expire_time_profile_id = user_meta.timeout); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ hemant@lower:/tmp/p4c/build$ git status On branch tighten-addent Your branch is up to date with 'origin/tighten-addent'. Changes not staged for commit: (use "git add <file>..." to update what will be committed) (use "git checkout -- <file>..." to discard changes in working directory) modified: ../backends/dpdk/dpdkArch.cpp

I do not know why -f is not copying the .p4-error output file. Perhaps it is related to the fact that p4-dpdk back end uses a different suffix .p4-error vs. the .p4-stderr used by other back ends? In any case, looks like the quickest workaround is to create the .p4-error file you need by copying and pasting the error messages you see in the output to that file.

I downloaded latest p4c even in this repo the -stderr file has size zero. Also interesting is that it is not a -error file. My issue is I do not see any failed test case added by PNA in p4_16_errors directory and a failed test in p4_16_samples has zero content.

hemant@lower:/tmp/p4c/testdata$ find -name "pna-add-on-miss-err*" ./p4_16_samples/pna-add-on-miss-err.p4 ./p4_16_samples_outputs/pna-add-on-miss-err.p4-stderr ./p4_16_samples_outputs/pna-add-on-miss-err-frontend.p4 ./p4_16_samples_outputs/pna-add-on-miss-err-midend.p4 ./p4_16_samples_outputs/pna-add-on-miss-err-first.p4 ./p4_16_samples_outputs/pna-add-on-miss-err.p4 hemant@lower:/tmp/p4c/testdata$ ls -l ./p4_16_samples_outputs/pna-add-on-miss-err.p4-stderr -rw-rw-r-- 1 hemant hemant 0 Oct 25 16:53 ./p4_16_samples_outputs/pna-add-on-miss-err.p4-stderr hemant@lower:/tmp/p4c/testdata$

.p4-error files are ONLY produced by p4c-dpdk, not any of the other P4 compiler commands or targets. I believe that it uses a different suffix than the .p4-stderr used by other targets so that the same .p4 test program can be a test program for multiple p4c targets, and do automated checking of stderr output error messages that differ between DPDK and other targets.

Naturally one might wonder whether every target should have its own separate and independent expected output files. I don't know if that is useful now or in the near future, but it is not the way p4c's automated tests work now.

I am unlikely to give these code changes an approval without explicit test programs that cause the error cases to be exercised. If you can include or attach in a comment such test programs that you believe should give error messages, and what error messages they produce with your p4c changes, I may have some time to try the code in your PR out on my own system.

err1 says "either add_entry is called from a non default_action or add-on-miss is not configured".
err2 says "add_entry has first parameter as default action".

Test programs to use are:

dpdk/testdata/p4_16_samples/pna-example-addhit.p4.test // add_entry called from hit action, err1
dpdk/testdata/p4_16_samples/pna-example-miss.p4.test // add_entry uses default_action for first parameter, err2
dpdk/testdata/p4_16_samples/pna-add-on-miss-err.p4.test // add on miss is not configured., err2

usha1830 · 2022-10-26T03:03:01Z

We add error cases as XFAIL in the ./p4c/backends/dpdk/DpdkXfail.cmake. This way the test script compares the error output with the regex for XFAIL.
Somehow I did not notice that the -error file is empty. I will check the test script and update.

…tighten-addent

hesingh · 2022-10-27T12:18:25Z

@usha1830 Please review this PR.

usha1830

Code changes and tests look fine to me.
Please see if you can reuse the ValidateAddOnMissExterns pass which is exclusively for checking such constraints.

usha1830 · 2022-10-27T13:37:00Z

backends/dpdk/backend.cpp

@@ -71,6 +71,7 @@ void DpdkBackend::convert(const IR::ToplevelBlock *tlb) {
        new ConvertActionSelectorAndProfile(refMap, typeMap, &structure),
        new CollectTableInfo(&structure),
        new CollectAddOnMissTable(refMap, typeMap, &structure),
+        new CollectAddOnMissAdd(refMap, typeMap, &structure),
        new ValidateAddOnMissExterns(refMap, typeMap, &structure),


This pass name looks weird. The next pass ValidateAddOnMissExterns also checks such constraints. Can we not add these additional checks as well in that pass?

Will check the ValidateAddOnMissExterns pass and get back.

usha1830 · 2022-10-27T13:55:42Z

testdata/p4_16_samples_outputs/pna-example-addhit.p4.spec

@@ -0,0 +1,194 @@
+


This file shouldn't have been generated for error cases.

I don't know how to fix this - any idea?

usha1830 · 2022-10-27T13:56:22Z

testdata/p4_16_samples_outputs/pna-example-miss.p4.spec

@@ -0,0 +1,191 @@
+


Same here. On error, the .spec shouldn't have been generated.

usha1830

I think you can try removing the output .spec files. The checks would still pass.

hesingh · 2022-10-28T14:28:05Z

@mbudiu-vmw Please merge this PR, thanks.

jafingerhut · 2022-10-28T14:57:08Z

@hesingh @mbudiu-vmw I know this has one approval, but if you do not mind waiting until this weekend or so, I'd like a chance to run these changes with a few other test P4 programs to verify exactly which programs it gives error messages for, and which it does not. I haven't gotten to that yet, unfortunately.

hesingh · 2022-10-28T15:14:50Z

@jafingerhut Sure. @mbudiu-vmw This is a code change for the dpdk backend for PNA.

jfingerh · 2022-10-30T17:01:59Z

backends/dpdk/DpdkXfail.cmake

+  "add_entry action cannot be default action"
+   testdata/p4_16_samples/pna-example-miss.p4
+   )
+


I thought that the standard method for testing programs that give compile-time errors is not to add them to xfail, but to create an expected stderr output file, check that in, and the test script compares the expected vs. actual output? For DPDK, the choice has been made for this expected stderr output file to be called .p4-error (if I recall correctly), which is DIFFERENT than the expected stderr output file for non-DPDK programs, which use instead .p4-stderr The reason for these different suffixes is so that the same P4 source program can have different expected error messages printed to stderr for different back ends, and each compared against a different expected stderr output file.

jfingerh · 2022-10-30T17:10:23Z

In trying out the version of this PR after 8 commits on my local system, I compiled the added test programs to see how they behave. The following error message looks to me fairly unhelpful, and would ask if they could be improved. In particular, the error message contains no clues about the location of the add_entry call within the user's program, only the location of where it is defined within the pna.p4 include file.

$ cd testdata/p4_16_samples
$ ~/forks/p4c/build/p4c-dpdk pna-add-on-miss-err.p4 
/home/andy/forks/p4c/build/p4include/pna.p4(601): [--Werror=unexpected] error: add_entry must only be called from within an action with '' property equal to true
extern bool add_entry<T>(string action_name,
            ^^^^^^^^^

hesingh · 2022-10-30T17:54:04Z

$ cd testdata/p4_16_samples
$ ~/forks/p4c/build/p4c-dpdk pna-add-on-miss-err.p4 
/home/andy/forks/p4c/build/p4include/pna.p4(601): [--Werror=unexpected] error: add_entry must only be called from within an action with '' property equal to true
extern bool add_entry<T>(string action_name,
            ^^^^^^^^^

This test case and its error message is not added by this PR - its legacy. I will see what I can do.

jfingerh · 2022-10-30T18:18:57Z

@hesingh - My apologies, I did not realize this test case and error message existed before your changes. Feel free to ignore that comment if you cannot easily improve on the error message. If you want, I can file a separate issue to track a request for improving it.

hesingh · 2022-10-30T18:44:27Z

I fixed the legacy test to print line of code on error. The error itself looks fine to me. Thanks for testing.

jfingerh · 2022-10-30T18:58:35Z

Commit 9 of this PR I definitely consider an improvement on the error message I mentioned earlier. It now looks like this:

$ ~/forks/p4c/build/p4c-dpdk pna-add-on-miss-err.p4 
pna-add-on-miss-err.p4(138): [--Werror=unexpected] error: add_entry must only be called from within an action with '' property equal to true
        add_entry(action_name="next_hop", action_params = tmp, expire_time_profile_id = user_meta.timeout);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Again, no need for you to improve this @hesingh in this PR if it is not easy to determine how, as I can create a separate issue to track the enhancement request, but I suspect the original intent was that the error message might say from within an action with 'add_on_miss' property equal to true, rather than the empty string it prints now.

jfingerh · 2022-10-30T20:40:05Z

@hesingh @usha1830 I know that test program pna-dpdk-table-key-use-annon.p4 is not a new one added in this PR, but it seems like it is something that SHOULD be considered a compile time error, perhaps one that the changes in this PR could be enhanced to catch, but currently with commit 9 of this PR there is no error, and presumably there was no error before the changes in this PR, too.

Look at the definition of table ipv4_da2 and its action add_on_miss_action2, copied below for quick reference:

    action add_on_miss_action2() {
        add_entry(action_name="next_hop", action_params = {32w0, 32w1234}, expire_time_profile_id = user_meta.timeout);
    }
    table ipv4_da2 {
        key = {
            user_meta.key: exact;
        }
        actions = {
            @tableonly next_hop2;
            @defaultonly add_on_miss_action2;
        }
        add_on_miss = true;
        const default_action = add_on_miss_action2;
    }

Note that the add_entry call attempts to add the action named next_hop, but note that next_hop is not in the list of actions defined as allowed for table ipv4_da2. I think this program is a good test case that SHOULD cause a compile-time error, but currently it is silently allowed. (I have no idea what the run time behavior is for the spec file produced, but it seems like there is enough information in the spec file for DPDK to detect this problem and flag it as an error, too, but why not detect it at compile time rather than when you try to load it into DPDK?)

hesingh · 2022-10-30T21:47:12Z

Without my PR changes, p4c does not catch any error with pna-dpdk-table-key-use-annon.p4. I will see what I can do.

hesingh · 2022-10-31T01:31:14Z

@jfingerh Two more tests have the same error as pna-dpdk-table-key-use-annon.p4.

dpdk/testdata/p4_16_samples/pna-mux-dismantle.p4 // line 173 has error
dpdk/testdata/p4_16_samples/pna-dpdk-parser-state-err.p4 // line175 has error

jfingerh · 2022-10-31T01:41:24Z

I was pretty sure I saw at least one more program with a similar mistake, but did not recall which one. If you have updated error checking code that finds them all, then excellent! We can consider whether to either (a) update some of them to no longer have the mistake, or (b) keep the one with the mistake, and add another similar test program without the mistake, after we determine the complete set of programs with that mistake.

usha1830 · 2022-10-31T03:02:55Z

I was pretty sure I saw at least one more program with a similar mistake, but did not recall which one. If you have updated error checking code that finds them all, then excellent! We can consider whether to either (a) update some of them to no longer have the mistake, or (b) keep the one with the mistake, and add another similar test program without the mistake, after we determine the complete set of programs with that mistake.

@jfingerh I would suggest, that we go with option (a) and fix these tests as these tests seems to be written to test something else but used the some add-on-miss example as template. We can add a new test for this error checking.

usha1830 · 2022-10-31T03:39:42Z

Also noticed another issue with these tests. There is no check for number of parameters. These tests have different number of action parameters passed in add_entry and the number of expected params.

    action next_hop(PortId_t vport) {
        send_to_port(vport);
    }
    action add_on_miss_action2() {
        add_entry(action_name="next_hop", action_params = {32w0, 32w1234}, expire_time_profile_id = user_meta.timeout);
    }

jfingerh · 2022-10-31T12:12:53Z

Good catch. Ideally the front end compiler could have a check that the second parameter of add_entry is a struct that contains field names corresponding to the action parameter names, and types corresponding to the action parameter types, in the same order. It should be a compile-time error if it is anything else.

hesingh · 2022-10-31T12:30:43Z

By design, I nearly certain, p4c frontend does not verify args of an extern. A backend verifies the args.

jfingerh · 2022-10-31T12:40:56Z

"By design, I nearly certain, p4c frontend does not verify args of an extern. A backend verifies the args." You may be right. If so, I guess the DPDK back end is the place for now to do it, but it would be nice if such a check, which I expect to be common for any p4c back end that wishes to implement the PNA architecture, could easily get these checks.

@mbudiu-vmw Do you have any suggested places to put "passes that can be reused by all implementations of an architecture, independent of target device, but are only useful when implementing that architecture?"

usha1830 · 2022-10-31T12:42:29Z

By design, I nearly certain, p4c frontend does not verify args of an extern. A backend verifies the args.

Yes. This check should be added to the backend.

hesingh · 2022-10-31T14:51:31Z

Also noticed another issue with these tests. There is no check for number of parameters. These tests have different number of action parameters passed in add_entry and the number of expected params.
    action next_hop(PortId_t vport) {
        send_to_port(vport);
    }
    action add_on_miss_action2() {
        add_entry(action_name="next_hop", action_params = {32w0, 32w1234}, expire_time_profile_id = user_meta.timeout);
    }

There is a check here and here.

hesingh · 2022-10-31T14:56:59Z

@jfingerh If any more checks are needed after the commit today, I'd like to defer the work to a new Issue.

jafingerhut · 2022-10-31T15:09:45Z

@hesingh: The checks you linked to appear to be verifying that add_entry has exactly 3 parameters. However, do they check that the second parameter is a struct with the correct number, type, and name of fields, i.e. those that match the action name given?

hesingh · 2022-10-31T15:28:52Z

@hesingh: The checks you linked to appear to be verifying that add_entry has exactly 3 parameters. However, do they check that the second parameter is a struct with the correct number, type, and name of fields, i.e. those that match the action name given?

I haven't have looked hard, but clearly the code is accepting a bit-field for 2nd arg which should be a struct. Please file a new issue to track fixing the issue, thanks.

jafingerhut

There are a few minor issues I will file to track small problems in the error messages here, but it looks good enough to me to merge in, and file those separate small issues.

jafingerhut · 2022-10-31T15:55:40Z

backends/dpdk/DpdkXfail.cmake

+  "add_entry action cannot be default action"
+   testdata/p4_16_samples/pna-example-miss.p4
+   )
+


Should these be marked xfail?

Yes. -addhit calls add_entry from a non-default_action. -miss calls add_entry with first arg as the name of default_action.

Right, but I am pretty sure that is why you check in .p4-error files with the expected stderr output from p4c-dpdk for those, and the test infra checks that. At least for back ends other than DPDK, xfail means you wish the test case gives no errors, but it currently does for reasons of bugs whose fixes are not checked in, or you wish the test case DOES give an error, but it currently does NOT for reasons of bugs whose fixes are not checked in.

Does the test infra behave differently than that for DPDK back end tests?

No, both p4c-dpdk and test infra give same error results. Regarding -error file, most such files have zero size in p4_16_samples_output.

Also, I see @usha1830 asking Mihai to merge her PRs, so I don't think she has merge permission.

If she can't merge it in, I still want to give her a chance to look once more before someone else does, since further changes have been made since she approved.

Right, but I am pretty sure that is why you check in .p4-error files with the expected stderr output from p4c-dpdk for those, and the test infra checks that. At least for back ends other than DPDK, xfail means you wish the test case gives no errors, but it currently does for reasons of bugs whose fixes are not checked in, or you wish the test case DOES give an error, but it currently does NOT for reasons of bugs whose fixes are not checked in.

Does the test infra behave differently than that for DPDK back end tests?

@jafingerhut Test infra does has some issue with comparing the -error files, being tracked using #3640. I am working on it. I will cleanup the XFAIL list also post that.

jafingerhut · 2022-10-31T15:56:12Z

@usha1830 Do you want to take one last look as of the 10th commit on this PR, or go ahead and merge it in?

usha1830

Looks good to me. Reference output to be updated for failing tests.

Tighten p4c for add-on-miss

72ede41

hesingh mentioned this pull request Oct 24, 2022

Add Example for add_entry/"Tables with add-on-miss capability" p4lang/pna#54

Open

jafingerhut reviewed Oct 24, 2022

View reviewed changes

Cleanup

0aab772

hesingh and others added 4 commits October 26, 2022 16:26

Fix defAtion emplace; add error tests to xfail

9f45ce9

Merge branch 'main' into tighten-addent

bb83946

Manually edit frontend files for 2 new tests

1b89588

Merge branch 'tighten-addent' of https://github.com/hesingh/p4c into …

3c3a359

…tighten-addent

usha1830 reviewed Oct 27, 2022

View reviewed changes

address review comments

9d06eb4

usha1830 approved these changes Oct 28, 2022

View reviewed changes

Rm .spec files for failed tests

2d803b1

jfingerh reviewed Oct 30, 2022

View reviewed changes

Fix error to include loc

198ed32

Fix legacy tests; add checj for action not in tbl

2059232

jafingerhut approved these changes Oct 31, 2022

View reviewed changes

jafingerhut reviewed Oct 31, 2022

View reviewed changes

Merge branch 'main' into tighten-addent

98b481a

usha1830 approved these changes Nov 1, 2022

View reviewed changes

hesingh added 2 commits November 1, 2022 04:12

update spec files

7a0be79

update

018c2c4

jafingerhut merged commit 8d74653 into p4lang:main Nov 1, 2022

usha1830 mentioned this pull request Nov 2, 2022

DPDK Test infra for error has issues #3640

Closed

hesingh deleted the tighten-addent branch November 2, 2022 14:11

Tighten p4c for add-on-miss #3614

Tighten p4c for add-on-miss #3614

Conversation

hesingh commented Oct 24, 2022 • 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

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

usha1830 commented Oct 26, 2022

hesingh commented Oct 27, 2022

usha1830 left a 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

usha1830 left a comment

Choose a reason for hiding this comment

hesingh commented Oct 28, 2022

jafingerhut commented Oct 28, 2022

hesingh commented Oct 28, 2022

Choose a reason for hiding this comment

jfingerh commented Oct 30, 2022

hesingh commented Oct 30, 2022

jfingerh commented Oct 30, 2022

hesingh commented Oct 30, 2022

jfingerh commented Oct 30, 2022

jfingerh commented Oct 30, 2022

hesingh commented Oct 30, 2022

hesingh commented Oct 31, 2022

jfingerh commented Oct 31, 2022

usha1830 commented Oct 31, 2022

usha1830 commented Oct 31, 2022

jfingerh commented Oct 31, 2022

hesingh commented Oct 31, 2022

jfingerh commented Oct 31, 2022

usha1830 commented Oct 31, 2022

hesingh commented Oct 31, 2022

hesingh commented Oct 31, 2022

jafingerhut commented Oct 31, 2022

hesingh commented Oct 31, 2022

jafingerhut left a 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

Choose a reason for hiding this comment

jafingerhut commented Oct 31, 2022

usha1830 left a comment

Choose a reason for hiding this comment

hesingh commented Oct 24, 2022 •

edited

Loading