Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support header and struct for logging (fixes #2201) #2596

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Support header and struct for logging (fixes #2201) #2596

wants to merge 1 commit into from

Conversation

lcikaric
Copy link

@lcikaric lcikaric commented Oct 27, 2020 •
edited
Loading

Changing the condition enabled to json file generation. This is a prerequisite for the execution of behavioral-model, where other changed were made. This PR is related to p4lang/behavioral-model#962 PR in behavioral-model.

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
Copy link
Contributor

I think the prerequisite might be best described in the other direction. That is, until the corresponding changes to p4lang/behavioral-model repository are approved and merged in, it is undesirable to merge these changes to p4c.

jafingerhut
jafingerhut reviewed Oct 27, 2020
View reviewed changes
testdata/p4_16_samples/issue2201-bmv2-test1.p4 Outdated

Header hdr1 = {3,-1,true};
log_msg("hdr1 = {}",{hdr1});
// Output: hdr1 = {'val1': 3, 'val2': -1, 'val3': 1, '$valid$': 1}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know there is currently no convenient way to automate the checking of the log message output in p4c's automated tests, but even so, I think it would be good to add a call to log_msg() on a header that is currently invalid, to at least test that this doesn't cause behavioral-model to crash.

@mihaibudiu
Copy link
Contributor

This is very nice. Hopefully more architectures will support this.

mihaibudiu
mihaibudiu approved these changes Oct 27, 2020
View reviewed changes
Copy link
Contributor

@mihaibudiu mihaibudiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, is there any reason not to merge this? No tests we have are impacted.

@jafingerhut
Copy link
Contributor

I guess the test program does not have a name ending in '-bmv2.p4' and thus will not run bmv2 with the compiled output of this program as input. It seems best for long term slightly better test coverage if it did run bmv2. If so, then merging these changes should wait for the corresponding p4lang/behavioral-model merge, plus enough hours for that to be used for p4lang/p4c automated tests.

@lcikaric
Support header and struct for logging
fa11ffe 
Changing the condition enabled to json file generation.
This is a prerequest for the execution of behavioral-model, where other changed were made.

    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}
@lcikaric
Copy link
Author

On my local machine (Linux, Ubuntu 18.04) all tests are passing, same as for macOS on Travis. Can you possibly help me interpret why those two tests which I added fail, on Linux on Travis, with message:
terminate called after throwing an instance of 'std::runtime_error' what(): in Json::Value::operator[](char const*)const: requires objectValue Calling target program-options parser Could not connect to any of [('127.0.0.1', 9398), ('127.0.0.1', 9398)] Could not connect to thrift client on port 9398 Make sure the switch is running and that you have the right port Check for /p4c/testdata/p4_16_samples/issue2201-test1-bmv2.stf *** simple_switch died with return code -6 *** CLI process failed with exit code 1 *** Test failed
because I can't reproduce error locally?

@mihaibudiu
Copy link
Contributor

What happens when you run this on your machine: ./bmv2/testdata/p4_16_samples/issue2201-test1-bmv2.p4.test?

@lcikaric lcikaric closed this Oct 30, 2020
@mihaibudiu
Copy link
Contributor

Why close the PR?

@lcikaric lcikaric reopened this Oct 30, 2020
@lcikaric
Copy link
Author

What happens when you run this on your machine: ./bmv2/testdata/p4_16_samples/issue2201-test1-bmv2.p4.test?

This is the output:
~/p4c/build$ ./bmv2/testdata/p4_16_samples/issue2201-test1-bmv2.p4.test
Check for /home/lidija/p4c/testdata/p4_16_samples/issue2201-test1-bmv2.stf
Calling target program-options parser
Obtaining JSON from switch...
Done
Control utility for runtime P4 table manipulation
RuntimeCmd:

@mihaibudiu
Copy link
Contributor

It looks like Travis is running a different version of bmv2. I think it rebuilds a new one periodically, so perhaps it does not yet have all changes made in that repo?

@lcikaric
Copy link
Author

Ah, yes, as soon as bmv2 PR p4lang/behavioral-model#962 is merged into bmv2 tests should pass. Thank you!

@lcikaric lcikaric mentioned this pull request Oct 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jafingerhut jafingerhut jafingerhut left review comments

@mihaibudiu mihaibudiu mihaibudiu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lcikaric @jafingerhut @mihaibudiu