p4lang / p4c

P4_16 reference compiler
https://p4.org/
Apache License 2.0
680 stars 444 forks source link

Bogus "duplicates declaration" error #780

Closed jnfoster closed 7 years ago

jnfoster commented 7 years ago

Compiling the following P4-14 program,

header_type headers {
  fields {
    counterrevolution : 32;
    cartographers : 8;
    wadded : 8;
    terns : 32;
    miscellaneous : 8;
  }
}
header headers heartlands;
parser start {
  return ingress;
}
action add_heartlands() {
  add_header(heartlands);
}
control ingress { }

produces a mysterious error:

% p4test --p4-14 ~/Downloads/envelop.p4 
envelop.p4(1): error: struct headers: Duplicates declaration header headers
header_type headers {
^
error: 1 errors encountered, aborting compilation

If I change headers to foo, then it converts just fine:

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

header foo {
    bit<32> counterrevolution;
    bit<8>  cartographers;
    bit<8>  wadded;
    bit<32> terns;
    bit<8>  miscellaneous;
}

struct metadata {
}

struct headers {
    @name("heartlands") 
    foo heartlands;
}

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

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

control egress(inout headers hdr, inout metadata meta, inout standard_metadata_t standard_metadata) {
    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;

Hence, it seems that p4c is introducing names that may or may not be unique, and/or not mangling the names in the source program to ensure there are no clashes.

jnfoster commented 7 years ago

Further bugs:

cc10512 commented 7 years ago

Not that the compiler should crash, but for example, replacing headers with ingress works as long as you also rename your ingress control to something else. This code compiles for me:

header_type ingress {
  fields {
    counterrevolution : 32;
    cartographers : 8;
    wadded : 8;
    terns : 32;
    miscellaneous : 8;
  }
}
header ingress heartlands;
parser start {
  return theIngress;
}
action add_heartlands() {
  add_header(heartlands);
}
control theIngress { }
#include <core.p4>
#include <v1model.p4>

header ingress {
    bit<32> counterrevolution;
    bit<8>  cartographers;
    bit<8>  wadded;
    bit<32> terns;
    bit<8>  miscellaneous;
}

struct metadata {
}

struct headers {
    @name("heartlands")
    ingress heartlands;
}

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

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

control egress(inout headers hdr, inout metadata meta, inout standard_metadata_t standard_metadata) {
    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(), theIngress(), egress(), computeChecksum(), DeparserImpl()) main;
jnfoster commented 7 years ago

Here's another one

header_type h_t {
   fields { f : 8; }
}

header h_t h;

parser start {
  extract(h);
  return ingress;
}    

action nop() { }

table exact {
  reads { h.f : ternary; }
  actions { nop; }
  size : 256;
}

control ingress {
  apply(exact);
}

Here's the error message:

foo.p4(14): warning: .exact shadows exact
table exact {
^
p4c/p4include/core.p4(75)
    exact,
    ^^^^^
error: Cannot extract field apply from exact which has type match_kind
foo.p4(21): error: Could not find type of exact.apply
  apply(exact);
  ^^^^^^^^^^^^

It's also unclear why the compiler reports that .exact shadows exact and decides that exact is amatch_kindrather than a table. The declaration of.exactcomes from the#include` which is earlier in the file. So this might indicate another (different) bug besides the name clash issue.

mihaibudiu commented 7 years ago

I expect that all these issues should be fixed now; perhaps we can close.

mihaibudiu commented 7 years ago

I actually would prefer an independent verification that the issue us satisfactorily resolved, otherwise i could just close it myself.

cc10512 commented 7 years ago

Isn't that what the reviewer of your PR is supposed to do?

mihaibudiu commented 7 years ago

Often the reviewer isn't the same person who opened the issue, and may not have the same understanding of the issue. There are lots of issues waiting to be closed right now.

hanw commented 7 years ago

replace headers with standard_metadata_t triggers an error in programStructure.cpp.

libc++abi.dylib: terminating with uncaught exception of type Util::CompilerBug: In file: /Users/hanwang/p4c/frontends/p4/fromv1.0/programStructure.cpp:167
Compiler Bug: /Users/hanwang/p4c/frontends/p4/fromv1.0/programStructure.cpp:167: Null type

Everything else looks okay.

mihaibudiu commented 7 years ago

Yes, that is a separate issue #763. I hope.

hanw commented 7 years ago

I am not sure if it's the same issue as #763, but definitely a separate issue.

jnfoster commented 7 years ago

As someone who has reported many of the front-end bugs, I'd be happy if the implementor of the fix and/or reviewer verifies that it's been addressed -- that's why I always try to provide a concrete example!

It's non-trivial to checkout the fork, wait for the compiler to re-build, and check that the issue no longer manifests.

mihaibudiu commented 7 years ago

Each fix comes with a reproduction test which usually copies the example filed with the issue. So if the tests pass the issue should be fixed. The test is named like the issue, e.g. Issue780.p4