P4-14 to P4-16 fails in getting parameters from extern instatiation

[engjefersonsantiago]

The P4-14 to P4-16 conversion is not getting the correct instatiation parameters. E.g:

Following P4-14 code:

parser start { return ingress; }
extern_type extern_test {
    attribute att_test {
        type: int;
    }
    method my_extern_method ();
}
extern extern_test my_extern_inst {
  att_test: 0xab;
}
action a() { my_extern_inst.my_extern_method(); }
table t {
    actions { a; }
}
control ingress {
    apply(t);
}
control egress {}

Generates:

#include <core.p4>
#include <v1model.p4>

struct metadata {
}

struct headers {
}

extern extern_test {
    void my_extern_method();
    extern_test();
}

parser ParserImpl(packet_in packet, out headers hdr, inout metadata meta, inout standard_metadata_t standard_metadata) {
    @name("start") state start {
        transition accept;
    }
}

control egress(inout headers hdr, inout metadata meta, inout standard_metadata_t standard_metadata) {
    apply {
    }
}

control ingress(inout headers hdr, inout metadata meta, inout standard_metadata_t standard_metadata) {
    extern_test() my_extern_inst;
    @name(".a") action a() {
        my_extern_inst.my_extern_method();
    }
    @name("t") table t {
        actions = {
            a;
            @default_only NoAction;
        }
        default_action = NoAction();
    }
    apply {
        t.apply();
    }
}

control DeparserImpl(packet_out packet, in headers hdr) {
    apply {
    }
}

control verifyChecksum(in headers hdr, inout metadata meta) {
    apply {
    }
}

control computeChecksum(inout headers hdr, inout metadata meta) {
    apply {
    }
}

V1Switch(ParserImpl(), verifyChecksum(), ingress(), egress(), computeChecksum(), DeparserImpl()) main;

I believe there is an issue with the v1parser.ypp, but I feel a bit unconfortable to change it.

[sethfowler]

I'll take a look.

[mihaibudiu]

This is probably about converting properties; they have no equivalent in P4-16.
They should probably be converted to constructor parameter, but for backwards compatibility with some existing P4-14 programs which are not in the public test suite they seem to be converted also to IR properties, which do not really exist in P4-16.

[sethfowler]

By the way @engjefersonsantiago, when you paste code, it will be formatted more nicely if you surround it with the appropriate markdown syntax, like this:

```p4
<code here>
```

(The p4 is not strictly necessary, but it tells Github to highlight the code using P4 syntax.)

[sethfowler]

@mbudiu-vmw yeah, that makes sense. I don't know much about extern properties, so I may not be the best person to look at this, actually. (I saw mention of v1parser.ypp and thought that if it was a parser problem, I could probably fix it quickly.)

[mihaibudiu]

Unfortunately externs were a P4 v1.1 extension, so they are not even in the P4-14 spec.
So it won't be easy to figure out what to do about them.
@ChrisDodd has written some code to read extern properties, but they are not converted to constructors, but to properties, which flow through the IR all the way to the back-end. They do not generate P4-16 source.
Frankly, P4 v1.1 is deprecated, and we should stop writing code in that dialect.

[ChrisDodd]

The issue is that P4_16 does not support extern attributes at all. When we convert P4_14 to P4_16 code, we keep the attributes in the IR, as the 'attributes' in the IR::Type_Extern, and the 'properties' in the IR::Declaration_Instance object, which allows backends to get at them and continue to support them properly, but if you dump the IR as P4, they're dropped, as there is no valid way of representing them in P4_16 syntax. I've toyed around with allowing extern attributes as an extension in P4_16, and extending the ToP4 dumping code to dump them as such, but the current code base does not have support for extern attributes in P4_16, only in P4_14.

…

On Wed, May 10, 2017 at 5:10 PM, Mihai Budiu ***@***.***> wrote: Unfortunately externs were a P4 v1.1 extension, so they are not even in the P4-14 spec. So it won't be easy to figure out what to do about them. @ChrisDodd <https://github.com/ChrisDodd> has written some code to read extern properties, but they are not converted to constructors, but to properties, which flow through the IR all the way to the back-end. They do not generate P4-16 source. Frankly, P4 v1.1 is deprecated, and we should stop writing code in that dialect. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#604 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD4c8bkEdbeeMUsUxJ0ntquuoBOK0xjfks5r4lIDgaJpZM4NXVpJ> .

[jnfoster]

I lack a detailed understanding of v1.1, but could we simply pick a canonical encoding as P4-16 extern objects? For example, maybe each attribute generates a corresponding constructor parameter and a getter/setter method. Then we could eliminate them in the front-end instead of carrying them down. We could also annotate the IR nodes to indicate they were encoding a v1.1 extern so that back-ends that wanted to treat these particular objects in a special way could do so.

[ChrisDodd]

The original plan was to transform extern attributes into construtor params. The problem is that this scale poorly for externs with many (mostly optional) attributes, and doesn't work at all for attributes that are expressions rather than constants -- those need to be transformed method arguments somehow, but there isn't really clean one-to-one mapping. You really need to redesign v1.1 externs that use general expression attributes to have methods instead.

[cc10512]

The resolution then is that extern attributes are not supported officially supported. They are part of the IR, so a particular backend can take advantage of them, but since they have no P4_16 equivalent, they will not be printed in P4_16. As @ChrisDodd points out, you should redesign the externs for P4_16 to use methods.

[engjefersonsantiago]

Ok then, it is not a problem to use P4-16. I am already using it. I opened the issue because I worked in a patch to support arbritrary externs in the bmv2 backend compiler and I faced this "problem".
However, I do believe we should throw an warning if the user tries to use extern attributes.

[jnfoster]

+1 We should add a warning to more gracefully alert programmers to this limitation of the converter.

…

-N

On Thu, May 11, 2017 at 11:12 AM, Jeferson Santiago da Silva < ***@***.***> wrote: Ok then, it is not a problem to use P4-16. I am already using it. I opened the issue because I worked in a patch to support arbritrary externs in the bmv2 backend compiler and I faced this "problem". However, I do believe we should throw an warning if the user tries to use extern attributes. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#604 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABwi0u8B2vhnRoLwEEC5ejRGklVN0VIKks5r40-agaJpZM4NXVpJ> .

[mihaibudiu]

Talk to @ChrisDodd first; it looks like Barefoot is actually using this feature.
He introduced it in the compiler.

[jnfoster]

It's safe to pass these attributes through as annotations on the IR nodes. A backend can do what it wants.

However, we should warn if we try to use a backend that doesn't support them (like Bmv2) or try to print them as P4 via the --top4 or --pp flags.

-N

[cc10512]

Sure, let's warn. @engjefersonsantiago would you mind submitting that patch for this issue?

[engjefersonsantiago]

Sure I can.
My suggestion is to insert this warnig in the parser.
So, I have an opened PR #591. Should I include the patch in the same PR?

[jnfoster]

Rather than issuing warnings in the front-end, perhaps they should go in the pretty printer (when we would be silently and confusingly eliding these IR nodes) and/or in backends that cannot handle them.

[jnfoster]

In general, we try to split up PRs into as small as is reaosnable. Since these warnings are an independent improvement to p4c, it'd be great to do it separately from your other PR.

If it'd be a burden to split it out you can do them together. But it's more work to review...

[engjefersonsantiago]

@jnfoster, I feel that I have not such experience with the compiler code to progate this warning deeper in the code. I think that someone more experint should address this issue.

[ChrisDodd]

We've added infrastructure to be able to add custom ExternConverters in the compiler to translate P4_14 extern attributes into something else in P4_16 (constructor params or type params or abstract functions or anything else that can be represented in P4_16). It is likely that the precise nature of the transform needed will depend on the purpose and use of the extern.

[mihaibudiu]

So can this issue be closed?

[mihaibudiu]

Since no one answered my question for a few months, I will assume that we can close.