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.

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

BMV2 backend generates no errors for hash input expressions it can't handle #430

Closed
kucejan opened this issue Apr 4, 2017 · 5 comments
Closed
Assignees
Labels
bug This behavior is unintended and should be fixed. fixed This topic is considered to be fixed.

Comments

@kucejan
Copy link

kucejan commented Apr 4, 2017

According JSON input format BMV2 does not accept expressions as an input for hash calculations. However, no error is generated if you use an expression it can't handle; the result is just silent failure. BMV2 does not output any error with the generated JSON, but the output value of the hash is always zero regardless the input.

An example of a code:

hash(meta.hashvalue, HashAlgorithm.crc32, (bit<32>) 0, { hdr.ipv4.srcAddr & 0xFF000000 }, (bit<32>) TABSIZE);

Relevant fragment of the generated JSON:

  "calculations" : [
    {
      "name" : "calc",
      "id" : 0,
      "algo" : "crc32",
      "input" : [
        {
          "type" : "expression",
          "value" : {
            "op" : "&",
            "left" : {
              "type" : "field",
              "value" : ["ipv4", "srcAddr"]
            },
            "right" : {
              "type" : "hexstr",
              "value" : "0xff000000"
            }
          }
        }
      ]
    }
  ],
@mihaibudiu
Copy link
Contributor

If @antoninbas can confirm this diagnosis it should be relatively easy to fix this in the compiler.
The question is: what other built-in externs have similar restrictions?

@antoninbas
Copy link
Member

I can confirm this. I updated the bmv2 JSON parser to throw an error in this case (p4lang/behavioral-model#322).
The expressions need to be evaluated and the results written to temporaries.
Usually this restriction applies whenever a built-in extern has to consume a field list / tuple / list expression; generate_digest comes to mind, but support for that is a little hacky IIRC.

@kucejan
Copy link
Author

kucejan commented Apr 5, 2017

I can confirm the problem with digest() too.

digest<digest_t>(0, { meta.val + 5 });
-------
bit<32> temp = meta.val + 5;
digest<digest_t>(0, { temp });

In both cases expression type element is generated, but in the case of digest, running generated JSON in BMV2 ends up with SIGABRT due to assertion.

lt-simple_switch: P4Objects.cpp:1553: void bm::P4Objects::init_learn_lists(const Json::Value&): Assertion `type == "field"' failed.

But during my experiments, I have found another weird behaviour of digest(). If I use temporary metadata field and modify it just after the digest() call again, a digest with the new value of the field is sent. For example value meta.val + 10 is sent in the digest in this code:

meta.digestval = meta.val + 5;
digest<digest_t>(0, { meta.digestval });
meta.digestval = meta.digestval + 5;

@mihaibudiu
Copy link
Contributor

I think that the P4-14 spec was never clear about this fact, but actually the digest is given a reference to a field list, and the digest always executes at the end of the pipeline.

In P4-16 there are no references, so this kind of behavior is impossible to express in the language. By using a fresh temporary the compiler can implement the "eager" semantics.

We have a problem with regards to P4-14 programs; when translating them to P4-16 we can implement either semantics (delayed evaluation or immediate evaluation) by playing with temporaries.

There may be additional difficulties related to the control-plane API of the digest primitive; this is also not clearly specified.

mihaibudiu pushed a commit to mihaibudiu/p4c-clone that referenced this issue Apr 5, 2017
@mihaibudiu
Copy link
Contributor

The PR above fixes the "complex expression" problem, but it does not fix the weird semantics for digest.
So this issue should not yet be closed.

Unfortunately our testing framework does not support digest, so we can't write a test for this issue.

ChrisDodd pushed a commit that referenced this issue Apr 7, 2017
@mihaibudiu mihaibudiu added the bug This behavior is unintended and should be fixed. label Apr 7, 2017
@mihaibudiu mihaibudiu self-assigned this Apr 7, 2017
mihaibudiu pushed a commit to mihaibudiu/p4c-clone that referenced this issue May 12, 2017
@mihaibudiu mihaibudiu added the fixed This topic is considered to be fixed. label May 12, 2017
ChrisDodd pushed a commit that referenced this issue May 15, 2017
* Final fix for issue #430

* bug fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This behavior is unintended and should be fixed. fixed This topic is considered to be fixed.
Projects
None yet
Development

No branches or pull requests

3 participants