p4c should prohibit different controllable entities having the same control-plane name

[kfcripps]

For example:

header h_t {
    bit<8> f;
}

control C() {
    @name(".act") action bar() {}

    @name(".act") action foo() {}

    table t {
        actions = { bar; }
        default_action = bar;
    }

    table t2 {
        actions = { foo; }
        default_action = foo;
    }

    apply {
        t.apply();
        t2.apply();
    }
}

control proto();
package top(proto p);

top(C()) main;

Per the P4-16 spec, shouldn't this result in an error message?

Every controllable entity exposed in a P4 program must be assigned a unique, fully-qualified name, which the control plane may use to interact with that entity. The following entities are controllable.

value sets
tables
keys
actions
extern instances

...

Each element may be annotated with at most one @name or @hidden annotation, and each control plane name must refer to at most one controllable entity. This is of special concern when using an absolute @name annotation: if a type containing a @name annotation with an absolute pathname (i.e., one starting with a dot) is instantiated more than once, it will result in the same name referring to two controllable entities.

...

Without the @name annotation, this program would produce two controllable entities with fully-qualified names main.c1.t and main.c2.t. However, the @name(".foo.bar") annotation renames table t in both instances to foo.bar, resulting in one name that refers to two controllable entities, which is illegal.

[kfcripps]

Similarly, the following P4 program:

header h_t {
    bit<8> f;
}

control C() {
    action bar() {
    }

    table t {
        actions = { bar; }
        default_action = bar;
    }

    table t2 {
        actions = { bar; }
        default_action = bar;
    }

    apply {
        t.apply();
        t2.apply();
    }
}

control proto();
package top(proto p);

top(C()) main;

gets transformed into:

header h_t {
    bit<8> f;
}

control C() {
    @name("C.bar") action bar() {
    }
    @name("C.bar") action bar_1() {
    }
    @name("C.t") table t_0 {
        actions = {
            bar();
        }
        default_action = bar();
    }
    @name("C.t2") table t2_0 {
        actions = {
            bar_1();
        }
        default_action = bar_1();
    }
    apply {
        t_0.apply();
        t2_0.apply();
    }
}

control proto();
package top(proto p);
top(C()) main;

so we end up with two actions defined in the same scope that have the same local name (C.bar). Is this a bug per the spec?

[kfcripps]

@jphurl @oleg-ran-amd @rsmith-pensando

[jafingerhut]

@kfcripps This behavior has existed in p4c for quite some time. I will send links to some other p4c issues that I am aware of from years back with some discussion on the topic, but if I am recalling correctly, the reason that p4c internally takes one action and makes multiple copies of it, one for each table it is used within, has its motivation in allowing each of those copies to be optimized independently of each other, based upon the context in which the table invoking it is applied.

Here is the oldest issue I could find with the most discussion on it:

• Compiler silently allows duplicate name annotations #1949

Here are some later issues, where either other people rediscovered this, or a similar thing, or in some case I probably forgot about the old one and filed another. As one of my comments in the first issue linked below says, I would love it if we could get all the stakeholders involved into a discussion with the goal of coming up with an answer to this.

• Add check of uniqueness of control plane names #3228
• Incorrect P4Info generation when different actions have same name annotation #2755
• Is this the intended effect of @name annotations when generating P4Info files? #2716

There is also a PR that started with one potential change, and I believe the discussion on it led to a proposal for a different approach, one which would be a small p4c change, but I haven't thought about the details in a while to have a good evaluation of either of those proposed changes at this moment:

• Add check of uniqueness of control plane names #3228

In particular, this comment: #3228 (comment) mentions a specific approach that might be a good one to try. I have copied the text of that comment below for convenience:

dmatousek I spoke with some folks at Intel including Han Wang about the underlying issue here, and he had a suggestion for an experiment to try: put the @name conflict checking somewhere in the front end before the LocalizeAllActions pass, which is one that we know intentionally creates copies of actions with identical @name annotations. If there is a @name conflict before that pass, it should be one that the P4 developer themselves wrote.

The only disadvantage I can think of to such an approach is that now P4_16 source code produced as the output of passes after LocalizeAllActions could cause errors when used as input to p4c. I do not know how serious that is, but perhaps having a command line option that skips the new @name conflict check would help with that.

[jafingerhut]

@kfcripps This PR #4951 is intended to address this issue. Feel free to take a look at it, and review it if you are interested. It only issues error messages if you invoke a p4c command with the command line options to generate one or more P4Runtime API files. Without such options, there are no errors given for such programs, just as there are no errors for similar programs with two tables in the same control with the same @name annotation.

[kfcripps]

@oleg-ran-amd Please take a look at #4951. This is related to issue # 4025 in our backend.

[jafingerhut]

@kfcripps @oleg-ran-amd Sorry for any confusion, but I would recommend we abandon PR #4951, and instead focus on #4975, which includes all of the changes of #4951 plus one more fix.

[jafingerhut]

If/when you do review PR #4975, this slide deck might help provide useful background on what is changing, what is not changing, and why: https://docs.google.com/presentation/d/1JqQciG8hWdd2UlbXbDENw36yfGGRk2LugjzRrbv6Sf4/edit?usp=sharing

That link is in the first comment on PR #4975, but it is a big comment, so wanted to call it out to you.

[jafingerhut]

Just a reminder: You and I are probably the two people in the world who care most about this issue. My proposed PR #4975 is unlikely to move forward without someone reviewing it. This issue is likely to be even lower priority for everyone else reviewing p4c PRs. No rush on my part.

[kfcripps]

@jafingerhut Sorry for the delay - I promise, I have not forgotten about this. 😃 Oleg is probably the best person to review your PR but I think he has been swamped lately. If he hasn't responded by the end of the Thanksgiving holiday, I will message him directly to remind him about this. I could also try to find time to review it myself but I think it would be great if other members on our team (in addition to just me and Anton) can get more involved with P4C, so I don't want to take the opportunity to do that away from Oleg 😉

[jafingerhut]

Now that #4975 is merged, I believe that fully addresses the statements made in the first comment of this issue.

The behavior in the second comment here #4651 (comment) remains exactly as it was before #4975 was merged. That is the behavior by design of the pass LocalizeAllActions, which, while it is certainly possible to change, is not necessarily easy, and for some back ends, perhaps not even desirable to change.

If you want the ability to make a compiler from what p4c offers, without using LocalizeAllActions, that is not something I am currently interested in working on, and don't expect to be any time later, either. You may want to consider creating a separate issue to track such an enhancement, if you want that.