p4lang / p4-spec

Apache License 2.0
178 stars 80 forks source link

Clarification over emits inside if statements #1338

Open vbnogueira opened 23 hours ago

vbnogueira commented 23 hours ago

In the deparser's apply block, are packet emits inside if statements allowed? For example, something like the following:

control IngressDeparserImpl(packet_out packet,
                            out empty_t clone_i2e_meta,
                            out empty_t resubmit_meta,
                            out empty_t normal_meta,
                            inout headers hdr,
                            in metadata meta,
                            in psa_ingress_output_metadata_t istd)
{
    apply {
        if (meta.mymd == 0) {
            packet.emit(hdr.ethernet);
        }
        packet.emit(hdr.ipv4);
    }   
}

The spec shows uses of if statements inside the deparser's apply block, so I'm assuming that is fine. However, there is nothing specifically mentioning emits.

The question arose because we tried to compile the example above using the ebpf backend, but the generated code was buggy. It compiled:

if (meta.mymd == 0) {
    packet.emit(hdr.ethernet);
}

to an empty if statement:

if (meta->mymd == 0) { 
;            }

and completely ignored the emit for the ethernet header

jafingerhut commented 23 hours ago

The language spec does not forbid if statements inside of a deparser. Neither does it require that all implementations must support them.

It would be best if an implementation that does not support them document this, and that its back end reject programs that attempt to use them.

I would recommend filing a bug in the https://github.com/p4lang/p4c repository that includes:

The most reasonable possible fixes for the EBPF back end would be one of:

(a) reject the program with an error that such if statements are not supported, or (b) support them fully and correctly

vbnogueira commented 13 hours ago

Thank you for the reply.

I would recommend filing a bug in the https://github.com/p4lang/p4c repository that includes:

  • the version of p4c source code that you built the compiler from, or if you did not, at least the output of the p4c --version command.
  • the full source code of a P4 test program that is close to as short as possible, yet exhibits the problem.
  • the full command line that you used to compile the program
  • the output that you saw, and any explanation of what you believe is wrong in that output.

Okk, opened the issue

The most reasonable possible fixes for the EBPF back end would be one of:

(a) reject the program with an error that such if statements are not supported, or (b) support them fully and correctly

In the case of (a), would it be ok to just reject if statements with emits inside, or should it just flat out reject any if statements?

jafingerhut commented 12 hours ago

Thank you for the reply.

I would recommend filing a bug in the https://github.com/p4lang/p4c repository that includes:

  • the version of p4c source code that you built the compiler from, or if you did not, at least the output of the p4c --version command.
  • the full source code of a P4 test program that is close to as short as possible, yet exhibits the problem.
  • the full command line that you used to compile the program
  • the output that you saw, and any explanation of what you believe is wrong in that output.

Okk, opened the issue

The most reasonable possible fixes for the EBPF back end would be one of: (a) reject the program with an error that such if statements are not supported, or (b) support them fully and correctly

In the case of (a), would it be ok to just reject if statements with emits inside, or should it just flat out reject any if statements?

I would recommend supporting the largest subset of possible programs that is reasonable to implement for whoever is doing the implementation, and reject all other programs.