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

Clarification over emits inside if statements #1338

Open
vbnogueira opened this issue Nov 21, 2024 · 3 comments
Open

Clarification over emits inside if statements #1338

vbnogueira opened this issue Nov 21, 2024 · 3 comments

Comments

@vbnogueira
Copy link

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
Copy link
Collaborator

jafingerhut commented Nov 21, 2024

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 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.

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
Copy link
Author

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants