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

Bug fix in typechecker: fixes #1937 #1948

Merged
merged 4 commits into from
Jul 10, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Bug fix in typechecker: fixes #1937
  • Loading branch information
mbudiu-vmw committed Jul 10, 2019
commit c923254b621f5b7d70d17d52182453da0b0e944b
5 changes: 4 additions & 1 deletion frontends/p4/typeChecking/typeChecker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -955,6 +955,8 @@ const IR::Node* TypeInference::preorder(IR::Declaration_Instance* decl) {
decl, decl->arguments, simpleType->to<IR::IContainer>());
auto type = typeAndArgs.first;
auto args = typeAndArgs.second;
auto *learn = clone();
(void)type->apply(*learn);
mihaibudiu marked this conversation as resolved.
Show resolved Hide resolved
if (type == nullptr || args == nullptr) {
prune();
return decl;
Expand Down Expand Up @@ -2779,7 +2781,8 @@ TypeInference::actionCall(bool inActionList,
// Check remaining parameters: they must be all non-directional
bool error = false;
for (auto p : left) {
if (p.second->direction != IR::Direction::None) {
if (p.second->direction != IR::Direction::None &&
p.second->defaultValue == nullptr) {
typeError("%1%: Parameter %2% must be bound", actionCall, p.second);
error = true;
}
Expand Down
39 changes: 39 additions & 0 deletions testdata/p4_16_samples/issue1937-1-bmv2.p4
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
Copyright 2019 Cisco Systems, Inc.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

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

header h1_t { bit<8> f1; bit<8> f2; }
struct headers_t { h1_t h1; }
struct metadata_t { }

action foo (out bit<8> x, in bit<8> y = 5) {
x = (y >> 2);
}

control ingressImpl(inout headers_t hdr, inout metadata_t meta, inout standard_metadata_t stdmeta) {
apply {
foo(hdr.h1.f1, hdr.h1.f1);
foo(hdr.h1.f2);
}
}
parser parserImpl(packet_in packet, out headers_t hdr, inout metadata_t meta, inout standard_metadata_t stdmeta) { state start { transition accept; } }
control verifyChecksum(inout headers_t hdr, inout metadata_t meta) { apply { } }
control egressImpl(inout headers_t hdr, inout metadata_t meta, inout standard_metadata_t stdmeta) { apply { } }
control updateChecksum(inout headers_t hdr, inout metadata_t meta) { apply { } }
control deparserImpl(packet_out packet, in headers_t hdr) { apply { } }
V1Switch(parserImpl(), verifyChecksum(), ingressImpl(), egressImpl(), updateChecksum(), deparserImpl()) main;
44 changes: 44 additions & 0 deletions testdata/p4_16_samples/issue1937-2-bmv2.p4
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
Copyright 2019 Cisco Systems, Inc.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

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

header h1_t { bit<8> f1; bit<8> f2; }
struct headers_t { h1_t h1; }
struct metadata_t { }

parser foo (out bit<8> x, in bit<8> y = 5) {
state start {
x = (y >> 2);
transition accept;
}
}

parser parserImpl(packet_in packet, out headers_t hdr, inout metadata_t meta, inout standard_metadata_t stdmeta) {
state start {
foo.apply(hdr.h1.f1, hdr.h1.f1);
foo.apply(hdr.h1.f2);
transition accept;
}
}

control ingressImpl(inout headers_t hdr, inout metadata_t meta, inout standard_metadata_t stdmeta) { apply { } }
control verifyChecksum(inout headers_t hdr, inout metadata_t meta) { apply { } }
control egressImpl(inout headers_t hdr, inout metadata_t meta, inout standard_metadata_t stdmeta) { apply { } }
control updateChecksum(inout headers_t hdr, inout metadata_t meta) { apply { } }
control deparserImpl(packet_out packet, in headers_t hdr) { apply { } }
V1Switch(parserImpl(), verifyChecksum(), ingressImpl(), egressImpl(), updateChecksum(), deparserImpl()) main;
41 changes: 41 additions & 0 deletions testdata/p4_16_samples/issue1937-3-bmv2.p4
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
Copyright 2019 Cisco Systems, Inc.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

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

header h1_t { bit<8> f1; bit<8> f2; }
struct headers_t { h1_t h1; }
struct metadata_t { }

control foo (out bit<8> x, in bit<8> y = 5) {
apply {
x = (y >> 2);
}
}

control ingressImpl(inout headers_t hdr, inout metadata_t meta, inout standard_metadata_t stdmeta) {
apply {
foo.apply(hdr.h1.f1, hdr.h1.f1);
foo.apply(hdr.h1.f2);
}
}
parser parserImpl(packet_in packet, out headers_t hdr, inout metadata_t meta, inout standard_metadata_t stdmeta) { state start { transition accept; } }
control verifyChecksum(inout headers_t hdr, inout metadata_t meta) { apply { } }
control egressImpl(inout headers_t hdr, inout metadata_t meta, inout standard_metadata_t stdmeta) { apply { } }
control updateChecksum(inout headers_t hdr, inout metadata_t meta) { apply { } }
control deparserImpl(packet_out packet, in headers_t hdr) { apply { } }
V1Switch(parserImpl(), verifyChecksum(), ingressImpl(), egressImpl(), updateChecksum(), deparserImpl()) main;
23 changes: 23 additions & 0 deletions testdata/p4_16_samples/issue1937.p4
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#include <core.p4>

header h1_t { bit<8> f1; bit<8> f2; }

parser foo (out bit<8> x, in bit<8> y = 5) {
state start {
x = (y >> 2);
transition accept;
}
}

parser parserImpl(out h1_t hdr) {
state start {
foo.apply(hdr.f1, hdr.f1);
foo.apply(hdr.f2);
transition accept;
}
}

parser p<T>(out T h);
package top<T>(p<T> p);

top(parserImpl()) main;
53 changes: 53 additions & 0 deletions testdata/p4_16_samples_outputs/issue1937-1-bmv2-first.p4
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
#include <core.p4>
#include <v1model.p4>

header h1_t {
bit<8> f1;
bit<8> f2;
}

struct headers_t {
h1_t h1;
}

struct metadata_t {
}

action foo(out bit<8> x, in bit<8> y=5) {
x = y >> 2;
}
control ingressImpl(inout headers_t hdr, inout metadata_t meta, inout standard_metadata_t stdmeta) {
apply {
foo(hdr.h1.f1, hdr.h1.f1);
foo(x = hdr.h1.f2, y = 8w5);
}
}

parser parserImpl(packet_in packet, out headers_t hdr, inout metadata_t meta, inout standard_metadata_t stdmeta) {
state start {
transition accept;
}
}

control verifyChecksum(inout headers_t hdr, inout metadata_t meta) {
apply {
}
}

control egressImpl(inout headers_t hdr, inout metadata_t meta, inout standard_metadata_t stdmeta) {
apply {
}
}

control updateChecksum(inout headers_t hdr, inout metadata_t meta) {
apply {
}
}

control deparserImpl(packet_out packet, in headers_t hdr) {
apply {
}
}

V1Switch<headers_t, metadata_t>(parserImpl(), verifyChecksum(), ingressImpl(), egressImpl(), updateChecksum(), deparserImpl()) main;

60 changes: 60 additions & 0 deletions testdata/p4_16_samples_outputs/issue1937-1-bmv2-frontend.p4
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
#include <core.p4>
#include <v1model.p4>

header h1_t {
bit<8> f1;
bit<8> f2;
}

struct headers_t {
h1_t h1;
}

struct metadata_t {
}

control ingressImpl(inout headers_t hdr, inout metadata_t meta, inout standard_metadata_t stdmeta) {
@name(".foo") action foo(out bit<8> x, in bit<8> y=5) {
x = y >> 2;
}
@name(".foo") action foo_0(out bit<8> x_1, in bit<8> y_1=5) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Having two actions with @name(".foo") annotations looks like wrong to me. If the user did this in their original source program, the compiler does not catch it as an error?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense that the compiler is taking one action, and creating two separate actions from it? Why is that useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code makes sure that each action is used in a single table. For some targets there is no action reuse anyway; if you have the same code you have to implement it twice.
This also opens the opportunity to specialize the action body for each call site. Perhaps this should be a controllable option - whether to duplicate or not. But it's not wrong, in the sense that it preserves the program semantics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not an error, you can have multiple actions with the same name; the control-plane disambiguates them through the tables that invoke them.

Copy link
Contributor

Choose a reason for hiding this comment

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

These are both invoked by the same table, so no such through-the-table disambiguation is possible here, is it?

x_1 = y_1 >> 2;
}
bit<8> tmp;
bit<8> tmp_0;
apply {
tmp_0 = hdr.h1.f1;
foo(tmp, tmp_0);
hdr.h1.f1 = tmp;
foo_0(x = hdr.h1.f2, y = 8w5);
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see how this intermediate code can compile, given that action foo_0 above has parameters named x_1 and y_1, not x and y.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a bug in the code that emitted P4, internally the names were correct. That's why the programs would compile correctly. I will add a commit to fix this in this PR.

}
}

parser parserImpl(packet_in packet, out headers_t hdr, inout metadata_t meta, inout standard_metadata_t stdmeta) {
state start {
transition accept;
}
}

control verifyChecksum(inout headers_t hdr, inout metadata_t meta) {
apply {
}
}

control egressImpl(inout headers_t hdr, inout metadata_t meta, inout standard_metadata_t stdmeta) {
apply {
}
}

control updateChecksum(inout headers_t hdr, inout metadata_t meta) {
apply {
}
}

control deparserImpl(packet_out packet, in headers_t hdr) {
apply {
}
}

V1Switch<headers_t, metadata_t>(parserImpl(), verifyChecksum(), ingressImpl(), egressImpl(), updateChecksum(), deparserImpl()) main;

90 changes: 90 additions & 0 deletions testdata/p4_16_samples_outputs/issue1937-1-bmv2-midend.p4
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
#include <core.p4>
#include <v1model.p4>

header h1_t {
bit<8> f1;
bit<8> f2;
}

struct headers_t {
h1_t h1;
}

struct metadata_t {
}

control ingressImpl(inout headers_t hdr, inout metadata_t meta, inout standard_metadata_t stdmeta) {
bit<8> tmp;
bit<8> tmp_0;
@name(".foo") action foo() {
tmp = tmp_0 >> 2;
}
@name(".foo") action foo_0() {
hdr.h1.f2 = 8w1;
}
@hidden action act() {
tmp_0 = hdr.h1.f1;
}
@hidden action act_0() {
hdr.h1.f1 = tmp;
}
@hidden table tbl_act {
actions = {
act();
}
const default_action = act();
}
@hidden table tbl_foo {
actions = {
foo();
}
const default_action = foo();
}
@hidden table tbl_act_0 {
actions = {
act_0();
}
const default_action = act_0();
}
@hidden table tbl_foo_0 {
actions = {
foo_0();
}
const default_action = foo_0();
}
apply {
tbl_act.apply();
tbl_foo.apply();
tbl_act_0.apply();
tbl_foo_0.apply();
}
}

parser parserImpl(packet_in packet, out headers_t hdr, inout metadata_t meta, inout standard_metadata_t stdmeta) {
state start {
transition accept;
}
}

control verifyChecksum(inout headers_t hdr, inout metadata_t meta) {
apply {
}
}

control egressImpl(inout headers_t hdr, inout metadata_t meta, inout standard_metadata_t stdmeta) {
apply {
}
}

control updateChecksum(inout headers_t hdr, inout metadata_t meta) {
apply {
}
}

control deparserImpl(packet_out packet, in headers_t hdr) {
apply {
}
}

V1Switch<headers_t, metadata_t>(parserImpl(), verifyChecksum(), ingressImpl(), egressImpl(), updateChecksum(), deparserImpl()) main;

Loading