Support header and struct for logging (fixes #2201 from p4c)

[lcikaric]

This PR is related to p4lang/p4c#2596 PR in p4c.

Example 1: header Hdr { int<4> a; bit<4> b; } Hdr h = {3,5}; log_msg("h = {}", {h}); Output: h = {'a': 3, 'b': 5, '$valid$': 1}

Example 2: struct Str { int<4> a; bit<4> b; bool c; } Str s = {3,5,true}; log_msg("s = {}", {s}); Output: s = {'a': 3, 'b': 5, 'c': 1}

[jafingerhut]

Looks like a useful enhancement to me.

Minor suggestion: For headers, it might be nice if the value of $valid$ was always first in the output, before the other fields. It might be reasonable to log invalid header values as {'$valid$':0} with no other fields shown, but I do not have a strong opinion on that as long as it is clear from the output when a header is valid vs. invalid.

I suspect taking advantage of these enhancements, if added to behavioral-model, will require some changes to p4c bmv2 back end code that might give an error, but of course those changes are not worth making until and unless these changes are approved and merged in.

[lcikaric]

There are 2 issues there IMO:

* it doesn't respect the type-value syntax used in the JSON format (https://github.com/p4lang/behavioral-model/blob/master/docs/JSON_format.md#the-type-value-object), some new `type` should be introduced

* there is some kind of asymmetry as inner-most structs are treated as headers, whereas outer structs are JSON arrays?

test.p4.txt out.json.txt

As you can see from provided p4 test and p4c JSON output, structure 'str1' which does not contain header as a field is represented as a header and structure 'str2' which contains header 'hdr1' is represented as a json array in value of type "parameters_vector" for log_msg. So there is asymmetry in representing inner-most struct and outer ones as you said.

Have you meant to introduce a new type for representing outer structs which have structs and headers as a fields? It makes sense to me.

[codecov-io]

Codecov Report

Merging #962 (cfa2a55) into master (a633059) will increase coverage by 0.23%. The diff coverage is 97.14%.

@@            Coverage Diff             @@
##           master     #962      +/-   ##
==========================================
+ Coverage   74.33%   74.57%   +0.23%     
==========================================
  Files         115      115              
  Lines       10252    10321      +69     
==========================================
+ Hits         7621     7697      +76     
+ Misses       2631     2624       -7     

Impacted Files	Coverage Δ
include/bm/bm_sim/actions.h	50.61% <ø> (+8.64%)	:arrow_up:
include/bm/bm_sim/core/primitives.h	50.00% <ø> (ø)
src/bm_sim/P4Objects.cpp	79.63% <95.45%> (+0.20%)	:arrow_up:
src/bm_sim/headers.cpp	99.47% <96.96%> (-0.53%)	:arrow_down:
include/bm/bm_sim/data.h	88.37% <100.00%> (+0.34%)	:arrow_up:
include/bm/bm_sim/headers.h	87.50% <100.00%> (+2.31%)	:arrow_up:
src/bm_sim/actions.cpp	89.91% <100.00%> (+0.64%)	:arrow_up:
src/bm_sim/learning.cpp	82.06% <0.00%> (+0.38%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a633059...cfa2a55. Read the comment docs.

[onf-cla-manager[bot]]

Hi @lcikaric, this is the ONF bot 🤖 I'm glad you want to contribute to our projects! However, before accepting your contribution, we need to ask you to sign a Contributor License Agreement (CLA). You can do it online, it will take only a few minutes:

✒️ 👉 https://cla.opennetworking.org

After signing, make sure to add your Github user ID lcikaric to the agreement.

For more information or help:" https://wiki.opennetworking.org/x/BgCUI

[onf-cla-manager[bot]]

Hi @lcikaric, this is the ONF bot 🤖 I'm glad you want to contribute to our projects! However, before accepting your contribution, we need to ask you to sign a Contributor License Agreement (CLA). You can do it online, it will take only a few minutes:

✒️ 👉 https://cla.opennetworking.org

After signing, make sure to add your Github user ID lcikaric to the agreement.

For more information or help:" https://wiki.opennetworking.org/x/BgCUI

[lcikaric]

I made changes according to your suggestions and I updated pr. What should be my further steps in order to merge this pr?

[lcikaric]

@antoninbas Can you suggest next steps on pr, please?

[antoninbas]

Very sorry about the delay, I missed the first notification (for https://github.com/p4lang/behavioral-model/pull/962#issuecomment-750922152).

Looking at this PR again, I think most of my original comments still stand. If we take your example:

Header hdr1 = {4,true};
Structure2 str2 = {3,hdr1};
log_msg("str2 = {}",{str2});

and the corresponding section from the generate JSON:

        {
          "op" : "log_msg",
          "parameters" : [
            {
              "type" : "string",
              "value" : "str2 = {}"
            },
            {
              "type" : "parameters_vector",
              "value" : [
                [
                  {
                    "type" : "hexstr",
                    "value" : "0x03"
                  },
                  {
                    "type" : "header",
                    "value" : "hdr1_0"
                  }
                ]
              ]
            }
          ],
          "source_info" : {
            "filename" : "test.p4",
            "line" : 80,
            "column" : 8,
            "source_fragment" : "        log_msg(\\\"str2 = {}\\\",{str2});"
          }
        }

As far as I'm concerned, this JSON is now what we want. It doesn't include any information about str2 (field names). The JSON should look like this:

{
              "type" : "parameters_vector",
              "value" : [
                {
                  "type" : "header",
                  "value" : "str2_0"
                }
              ]
            }

Where str2_0 is an instance of type Structure2, in which all fields have been flattened:

    {
      "name" : "Structure2",
      "id" : 2,
      "fields" : [
        ["val1", 7, true],
        ["val2.val1", 7, false],
        ["val2.val2", 1, false],
        ["_padding", 1, false]
      ]
    }

In other words, I would expect symmetry with str1. And for that example, I don't see any value to the concept of List.

[lcikaric]

Thank you for your response. I agree with you. I'm working on it.

[github-actions[bot]]

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment, or this will be closed in 180 days