p4lang / p4c

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

Compiler Bug: visitor returned non-Statement type: <Vector<StatOrDecl>>(104016) !!! #887

Closed MaoJianwei closed 6 years ago

MaoJianwei commented 7 years ago

The attach is my P4-16 source code, please help me! Thank you!

mao_mpls_switch.p4.zip

mao@mao-vm:~/maoP4/testbmv2/newtry$ p4c-bm2-ss --p4v 16 --p4runtime-file maoRuntime.file --p4runtime-format text -o output.file mao_mpls_switch.p4 In file: /home/mao/maoP4/p4c/ir/visitor.cpp:457 Compiler Bug: visitor returned non-Statement type: <Vector>(104016)

mao@mao-vm:~/maoP4/testbmv2/newtry$

hanw commented 7 years ago

Here is a simplified program that reproduces the problem.

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

header mpls {
    bit<8> label;
}

struct my_packet {
  mpls[8] data;
}

struct my_metadata {}

parser MyParser(packet_in b, out my_packet p, inout my_metadata m, inout standard_metadata_t s) {
  state start {
    transition accept;
  }
}

control MyVerifyChecksum(in my_packet hdr, inout my_metadata meta) {
    apply { }
}

control MyIngress(inout my_packet p, inout my_metadata m, inout standard_metadata_t s) {
    apply { }
}

control MyEgress(inout my_packet p, inout my_metadata m, inout standard_metadata_t s) {
    apply { }
}

control MyComputeChecksum(inout my_packet p, inout my_metadata m) {
    apply { }
}

control MyDeparser(packet_out b, in my_packet p) {
    apply {
        if (p.data[0].isValid()) {
            b.emit(p.data);
        }
    }
}

V1Switch(MyParser(), MyVerifyChecksum(), MyIngress(), MyEgress(), MyComputeChecksum(), MyDeparser()) main;
hanw commented 7 years ago

Without the if statement, the error does not happen.

control MyDeparser(packet_out b, in my_packet p) {
    apply {
            b.emit(p.data);
    }
}
MaoJianwei commented 7 years ago

Yes, i known that while debuging last night, but I wish compiler will give clear introductions for syntax checking :)

jafingerhut commented 7 years ago

In this program, the 'if' is redundant because emit() does nothing if the header(s) you give it are invalid. For a header stack it should only emit the subset of headers in the stack that are valid.

That said, agreed that the compiler should not crash, and preferably give a clear error message.

MaoJianwei commented 7 years ago

Hi, jafingerhut

I don't think 'if' is redundant, because the header maoMpls may be valid.

I wonder if packet.emit() will check the validation of header_stack internally? If I emit() an invalid header_stack without the beforehand 'if' check, will the program crash? or just do nothing?

do nothing is great for us while 'if' statement is not permitted in deparser

hanw commented 7 years ago

if statement is allowed in the deparser, the error message should be fixed.

That said, the semantics of emit() function is: it will check the validity of each header in the header_stack, and only emit the valid headers. You can check section 14.1 in P4-16 spec.

mihaibudiu commented 6 years ago

This should be fixed, so I will close it.