Improve diagnostics for MethodCallExpression

[kfcripps]

No description provided.

[kfcripps]

Does anybody know what these *.p4.spec files are used for, and if it's a problem that this PR changes their contents? I changed them by just running P4TEST_REPLACE=1 make check, but I don't really know what they are used for.

[fruffy]

This doesn't look right to me, I think this could lead to broken code because the DPDK back end relies on toString compare methods. It probably should not do that.

@usha1830 Do you have any other comments?

[jafingerhut]

@kfcripps The .spec files are the output of the p4c-dpdk back end. These .spec files can be loaded into a P4-DPDK software switch, which at a high level my understanding is that they are first translated into C source files, then compiled with a C compiler.

For the purposes of the p4c repository, they are expected output files for CI runs that if they change, someone working on the p4c-dpdk back end should verify whether that change is correct.

It seems like we have enough back ends and enough cases in how the automated p4c testing works that it might be nice to have a summary of the files in the p4c/testdata subdirectories, and how they are used during tests.

[usha1830]

Yes. there are several places where toString() is used in dpdk backend.
This current PR affects the places where hdr.isvalid() is used as table key. I can fix this specific case, however fixing all occurrences of toString() will need more time.

[kfcripps]

Thanks @fruffy @jafingerhut @usha1830. @usha1830 Let me know when you've fixed this, and I will rebase this PR. Or you can just add these changes to your PR and I will close this one.

[usha1830]

Thanks @fruffy @jafingerhut @usha1830. @usha1830 Let me know when you've fixed this, and I will rebase this PR. Or you can just add these changes to your PR and I will close this one.

@kfcripps I created a PR to fix the dpdk backend to use method->toString() instead of toString() directly on MCE #4383

[kfcripps]

This is ready for review.

[fruffy]

Similar error as with the DPDK back end here?

[kfcripps]

I assumed these were just log messages, are they not?

The entry action may vary based on the parameters of the action call, so just printing the action name here doesn't seem like enough information anyway.

For example: https://github.com/p4lang/p4c/pull/4354/files#diff-26cbcf98a38e0ae0e3e2de079be734bbda20fee14fb5c39cd6517e7c34d68d85R22-R24

[kfcripps]

@komaljai

[kfcripps]

Oh, this is a bash script. @komaljai @usha1830 Should the tc commands be updated to somehow take the action param values for each entry action, or is the action name enough?

[fruffy]

It looks like a command to me so in any case the formatting seems wrong here.

[kfcripps]

I have updated the p4tc backend to continue using just the action's name here.

[usha1830]

Oh, this is a bash script. @komaljai @usha1830 Should the tc commands be updated to somehow take the action param values for each entry action, or is the action name enough?

I presume we only expect the action name here. @vbnogueira @komaljai can comment further.

[vbnogueira]

Oh, this is a bash script. @komaljai @usha1830 Should the tc commands be updated to somehow take the action param values for each entry action, or is the action name enough?

I presume we only expect the action name here. @vbnogueira @komaljai can comment further.

So, @kfcripps is right. It should pass the parameters also.
The format would be the following (imagine that operation_add had two parameters "a" and "b" in the calc example). So something like:

action operation_add(bit<32> a, bit<32> b) {
        send_back(a + b);
}

The const entry declaration would look like:

const entries = {
            0x2b : operation_add(1 + 42);
           ...
}

And the template script should generate:

$TC p4template create table/calc/MainControlImpl/calculate entry op 0x2b permissions 0x1024 action calc/MainControlImpl/operation_test param a 1 param b 42