p4c-bm2-ss writes incorrect JSON file if there are "Removing unused action parameter" warnings

[jafingerhut]

See attached .p4 and .json files. When compiled there are (correct) warnings about unused parameters for action set_bd_dmac_intf.

If you look at action set_bd_dmac_intf in the JSON file, it has 3 entries for the used arguments under key "runtime_data". However, under the "primitives" key, every time there is a "modify_field" primitive action with source data from a parameter that comes after an unused (and thus removed) parameter, the index is the original index in the parameter list, before the unused ones were removed, which are now wrong.
demo1-variant.p4.txt
demo1-variant.p4.txt

[jafingerhut]

And if anyone else hits this issue before it is fixed, it does seem that removing all unused action parameters until those warnings are gone does work around this problem, as one might expect.

[sethfowler]

IMO, we should just stop removing unused action parameters.

We deliberately aren't doing it for P4Runtime.

Is there any reason not to make that change?

[mihaibudiu]

I was about to work on a fix, but indeed, it would be better if we could keep the unused parameters.
This behavior is inherited from the P4-14 compiler.

[jafingerhut]

I think continuing to warn about unused parameters would be useful, even if they are left in the bmv2 JSON file.

[mihaibudiu]

I will attempt to provide a fix for this that uses an #if REMOVE_PARAMETERS. So then we can do it in either way. But, of course, I won't be able to test both cases until BMv2 handles unused parameters.

Note that removing parameters violates the contract that the control-plane API is readable from the program.

[antoninbas]

There is nothing required from bmv2 AFAIK. If you leave an unused parameter in the JSON, nothing bad will happen, it will just not be used. And as pretty much everyone has mentioned, from a control-plane perspective, it is better to leave unused parameters and simply print a warning.

[mihaibudiu]

OK, then I will just them in. Much simpler.
Note that there seem to be several Travis build failures related to this bug, I don't know why it didn't show up before.

[antoninbas]

I updated bmv2 to sanitize runtime data indices in action bodies in the JSON. p4c-bmv2-ss outputs invalid JSON every time there is an unused parameter in an action, which is now rejected by bmv2. Unlike my previous sanity check (duplicate names / ids), I did not revert this fix, because I think it is a major bug, yet one "easy" to fix in p4c (a quick workaround may be to remove unused parameters in test programs).

[antoninbas]

I merged @mbudiu-vmw's PR, I think we can consider this closed.