Remove unnecessary check in conversion of log_msg to a JSON

[VolodymyrPeschanenkoIntel]

These small changes fix integration bug between behavioral-model and p4c-bm2-ss.
The p4c-bm2-ss generated the error if second argument is not Type_Bits and Type_Boolean.
The ExternConverter_log_msg::convertExternFunction makes conversion to a json. I think that in second argument could be any type of expression.

To repeat this bug, please, run the following command:
./p4c-bm2-ss -o ./11/issue2201-bmv2.json ../testdata/p4_16_samples/issue2201-bmv2.p4

The behavioral-model accepts created json without any errors.

[jfingerh]

Look for the earlier commit that added these checks. They are there specifically because at least at the time they were added, BMv2 did NOT support these types in its logging action. Are you saying that has changed since that time? In what behavioral-model commit was the logging support generalized to other types?

[jfingerh]

Just because bmv2 can READ the BMv2 JSON file, does not mean that it can execute the log action without errors. Test actually executing the log message action by sending through appropriate packets to find out whether BMv2 gives an error then.

[VolodymyrPeschanenkoIntel]

@jfingerh, Is it possible to create a issue for behavioral-model to support log_msg with other types? It looks like the v1model supports log_msg with any type?

[jfingerh]

It is trivial to create an issue for anything you want. Whether it gets implemented or not is the bigger question, and by whom :-)

If you create the issue, and are also interested in:

• developing the changes to behavioral-model that implement it
• test them
• write p4c test programs that exercise them
• get the behavioral-model changes to pass code review

then by all means, please go for it.

[mihaibudiu]

It may be easier to implement this entirely in the compiler.

[VolodymyrPeschanenkoLitSoft]

It may be easier to implement this entirely in the compiler.

@mbudiu-vmw, Do you have any suggestions for that?
Now this example crashed here, because tag == ActionParam::HEADER

[mihaibudiu]

We already have a pass doing this "flattenInterfaceStructs", but it doesn't touch values that are passed as arguments to functions. So a pass like this should be written for the arguments to log_msg, expanding one log_msg into many.

[VolodymyrPeschanenkoLitSoft]

We already have a pass doing this "flattenInterfaceStructs", but it doesn't touch values that are passed as arguments to functions. So a pass like this should be written for the arguments to log_msg, expanding one log_msg into many.

Great!!!! I will continue with that on this PR, thanks.

[VolodymyrPeschanenkoIntel]

@jfingerh , @mbudiu-vmw PR is ready for review, but we need to clarify one question: what we need to do with errors inside log_msg? We have some pass which could replace them into bit constants. Do we need to replace them in the SimpleSwitch?

[jfingerh]

@jfingerh , @mbudiu-vmw PR is ready for review, but we need to clarify one question: what we need to do with errors inside log_msg? We have some pass which could replace them into bit constants. Do we need to replace them in the SimpleSwitch?

Can you give an example of an error inside of log_msg that you are asking about?

If it is an unsupported type, I would recommend a compile-time error with as clear an error message as you can make it.

[VolodymyrPeschanenkoIntel]

The problem is here and here.

[jfingerh]

I see these example v1model P4 program lines you are referring to in your links:

        log_msg("hdr.ethernet={}", {hdr.ethernet});

and this one:

        log_msg("stdmeta={}", {stdmeta});

I still do not know what the errors are that you are asking about.

If you want to enhance the v1model implementation so that those log_msg() calls are supported, by printing out all of the fields of the stdmeta struct, for example, or the valid true/false of a header and the value of all of its fields, that sounds like a great idea.

If you don't want to make changes so that those are supported, then I recommend that whichever cases you do not support should give a compile time error when running p4c.

If you have a question not answered by the suggestions above, please ask a more specific question.

[VolodymyrPeschanenkoIntel]

I try to enhance the v1model implementation. The result of this transformation could be accepted by behavioral-model. Speaking true, I forgot to extract valid filed for header, thanks.

Without this transformation it gives the error:
// Assertion 'Default switch case should not be reachable' failed, file '../../include/bm/bm_sim/actions.h' line '334'.

[jfingerh]

Maybe I am assuming something here that needs to be said explicitly:

If you make enhancements in the p4c v1model back end, either:

(a) with those enhancements, the simple_switch/ simple_switch_grpc processes should be not only able to read the BMv2 JSON file without error, it should also be able to execute those log_msg calls without error.

(b) or, there should be corresponding enhancements to simple_switch/simple_switch_grpc code made first, such that (a) is true.

I am perfectly happy if you do not make any changes to simple_switch/simple_switch_grpc, as long as (a) is true. If you violate (a), then you are introducing new cases where the compiler gives no errors, but simple_switch/simple_switch_grpc will give a run-time error when trying to execute the log_msg calls.

[jfingerh]

My apologies, I might understand your original question now.

Are you asking: "What if someone tries to do a log_msg call on an expression with type error? Or a struct that contains a field with type error?"

If that is your question, then my answer would be one of:

(a) Give a compile-time error that log_msg calls are not supported on values with type error

or

(b) figure out a useful string representation to print for values of type error, and make the p4c v1model back end print the appropriate string representation somehow.

Approach (a) sounds easier to me for now. I do not have specific suggestions for how to implement (b).

[VolodymyrPeschanenkoIntel]

Ok, I will replace errors by string. For example, error.PacketTooShort => "PacketTooShort", thanks

[jfingerh]

If you do that, note that a P4 developer can define new constant values with type error in their P4 program, and it would be nice to support printing those names, too.

[VolodymyrPeschanenkoIntel]

If you do that, note that a P4 developer can define new constant values with type error in their P4 program, and it would be nice to support printing those names, too.

Does P4 have such examples in p4_16_samples folder?

[jfingerh]

Probably several, but this is the first one that I found: https://github.com/p4lang/p4c/blob/main/testdata/p4_16_samples/verify-bmv2.p4

[jfingerh]

While you are in this area, note that there are also non-serializable enums, i.e. those declared like this in the P4 source code: enum MyEnumType { name1, name2 }; that are probably turned into numerical values somewhere before the bmv2 v1model back end output, but those numeric values are made up by the compiler, and a P4 developer would have no reason to know which numbers correspond to which names in their source code.

[mihaibudiu]

Also note that midend passes may remove enums and errors, so you would have to do this before these passes are run.

[jfingerh]

Basically, if it gets to be too much work to print out names for these things, we understand :-) The next best thing would be to print out the numeric values for them, and we can add some documentation to the log_msg call explaining to developers where to determine the mapping between the names and numeric values.

[mihaibudiu]

Which test exercises this change?
I would expect that the test used to fail and now it doesn't.
Does this require changes to bmv2?

[VolodymyrPeschanenkoIntel]

I think that the best way to deal with errors is to replace them by constants like it was done here.

All enums eliminated here. So, we can ignore them.

To repeat this problem, please, run the following command:
./p4c-bm2-ss -o ./11/issue2201-bmv2.json ../testdata/p4_16_samples/issue2201-bmv2.p4

For that case we make log_msg flatten and replace Type_Error. After that p4c-bm2-ss successfully finished and result can be used to run behavioral-model without any errors.

[mihaibudiu]

Looks like bmv2 crashes for some of these tests.

[VolodymyrPeschanenkoIntel]

It looks like behavioral-models support Type_Error for verify and log_msg.

[mihaibudiu]

what happens if you don't find this substring?
should you give a warning?

[VolodymyrPeschanenkoIntel]

It should add other arguments in StructExpression to this {} like in your example below.

[mihaibudiu]

I will approve this, but perhaps you can make sure that removing the type will not result in an incorrect program before we merge.

[mihaibudiu]

The type you generate is new, but you could have something like this:

struct S { bit<8> f; } 
...
S s;
log_msg("{}", {s,s});

In this case you don't want to remove the type S and replace it with your new generated type.
The new type should be inserted right before the control/parser/action that calls log_msg.

[VolodymyrPeschanenkoIntel]

I will approve this, but perhaps you can make sure that removing the type will not result in an incorrect program before we merge.

The p4c has tuple type in second argument for all examples in testdata. This code doesn't change the type S, it changes type for {s,s}. In the program you have the type Tuple(S,S) for {s,s}, after it will be Tuple(bit<8>, bit<8>) and log_msg("{}", {s,s}) will be translated into log_msg("(f={}, f={})", {s.f,s.f}) and corresponded type will become Tuple(bit<8>, bit<8>) for {s.f,s.f}.

I removed generation of BUG see above, because of two variables s for one {} in log_msg.