p4lang / p4c

P4_16 reference compiler
https://p4.org/
Apache License 2.0
679 stars 444 forks source link

PSA back end generates BMv2 JSON that refers to non-existent header #1919

Open jafingerhut opened 5 years ago

jafingerhut commented 5 years ago

Compiling the attached P4_16+PSA architecture program using the latest version of p4c as of 2019-May-03:

$ p4c-bm2-psa psa-issue1919-bmv2.p4 -o psa-issue1919-bmv2.json

yields an incorrect BMV2 JSON file, which the latest version of psa_switch as of the same date correctly gives an error message when reading and validating it:

$ psa_switch --log-console -i 0@veth2 -i 1@veth4 ~/forks/p4c/testdata/p4_16_samples/psa-issue1919-bmv2.json 
Calling target program-options parser
Invalid reference to object of type 'header' with name 'scalars'

If this issue is corrected, the attached STF file test should work.

psa-issue1919-bmv2.p4.txt psa-issue1919-bmv2.stf.txt

yunheL commented 4 years ago

@jafingerhut Hi Andy, I just ran the psa-issue1919-bmv2.p4 and .stf locally. It looks like it has a very similar problem that this new PR.

Specifically, metadata_t.port is trying to refer a header called scalars but in the headers section, the header is somehow called null but not scalars. Below are screenshots of generated json for psa-issue1919-bmv2.p4

Here is the use of metadata_t.port, it has a header named scalars: scalar

But in the headers section of the generated json file, there is NO header object named scalar. There is a header object whose name is null and has scalar_t type. null

I think this is the bug and causing bmv2 to give the error Invalid reference to object of type 'header' with name 'scalars'.

I can look into how to solve this problem.

jafingerhut commented 4 years ago

@yunheL Have you tried running psa-issue1919-bmv2.p4 with your changes for PR https://github.com/p4lang/p4c/pull/2420 ?

If not, please do. If it fixes the problems you were addressing there, and this one, that is excellent.

yunheL commented 4 years ago

@jafingerhut Yes, I just tried running psa-issue1919-bmv2.p4 with PR #2420 included. It did not solve the problem here in issue1919, but it did solve the register example I included in PR #2420.

I think the problem here should be solvable with similar approach as the PR, I will investigate this problem.

jafingerhut commented 4 years ago

I know I've mentioned this before, and I do not have the detailed knowledge of p4c to back up this guess, but it seems to me that psa-issue1919-bmv2.p4 compiles wrongly in a way that the v1model-specific code in p4c handles perfectly, and there is so little PSA-specific code in that test program, e.g. no externs. It seems likely that there is some part of the v1model back end code that is simply missing or mis-copied-and-pasted from v1model back end to PSA back end. But, take that guess with a big grain of salt.

hesingh commented 4 years ago

p4c/backends/bmv2/common/header.cpp has a few LOG messages. Run the test using `-T header.cpp:1" to see LOG1 messages. Add your own LOG messages to debug more.

hesingh commented 4 years ago

This method is receiving a null name and printing null for scalar.

https://github.com/p4lang/p4c/blob/master/backends/bmv2/common/JsonObjects.cpp#L202

This is the call stack.

./p4c-bm2-psa : BMV2::JsonObjects::add_header(cstring const&, cstring const&)+0x149
  ./p4c-bm2-psa : BMV2::PsaProgramStructure::createScalars(BMV2::ConversionContext*)+0x8c
  ./p4c-bm2-psa : BMV2::PsaProgramStructure::create(BMV2::ConversionContext*)+0x49
yunheL commented 4 years ago

This method is receiving a null name and printing null for scalar.

https://github.com/p4lang/p4c/blob/master/backends/bmv2/common/JsonObjects.cpp#L202

This is the call stack.

./p4c-bm2-psa : BMV2::JsonObjects::add_header(cstring const&, cstring const&)+0x149
  ./p4c-bm2-psa : BMV2::PsaProgramStructure::createScalars(BMV2::ConversionContext*)+0x8c
  ./p4c-bm2-psa : BMV2::PsaProgramStructure::create(BMV2::ConversionContext*)+0x49

Ah, thanks! That is very helpful!

hesingh commented 4 years ago

The preorder(const IR::Declaration_Variable* dv), see below, is never invoked during the test.

https://github.com/yunheL/p4c/blob/psa-scalar/backends/bmv2/psa_switch/psaSwitch.cpp#L415

So, the scalars std::map is empty and this is why the null name is added to scalars.

I see that simple_switch does not use a scalars std::map. Why did psa_switch decide to use one?

More digging into bmv2 is needed.

jafingerhut commented 3 days ago

Just doing a status check on the PSA implementation in BMv2. This issue is still true of the latest version of p4c-bm2-ss and psa_switch executables built from source as of 2024-oct-31.