p4c: More questions on setInvalid

Hello. I have another clarification question on setInvalid, this is a follow-up to #2212. This issue is quite esoteric but has given me some trouble recently.

So for a program like this:

control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
    apply {
        h.h.setInvalid();
        h.h.a = 16w2;
        h.eth_hdr.eth_type = h.h.a * h.eth_hdr.eth_type;
    }
}

the LocalCopyPropagation pass collapses the values into

control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
    apply {
        h.h.setInvalid();
        h.eth_hdr.eth_type = 16w2 * h.eth_hdr.eth_type;
    }
}

h.eth_hdr is a valid header and its value is now always 16w2 * h.eth_hdr.eth_type after the pass instead of being undefined. However, since the alternative is undefined behaviour does it matter? The only case I can think of where this may affect the output is something like

control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
    apply {
        h.h.setInvalid();
        h.h.a = 1;
        h.eth_hdr.src_addr = h.h.a;
        if (h.eth_hdr.src_addr != 1) {
            h.h.setValid();
            h.h.a = 1;
        }
    }
}

which is eventually turned into

control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
    apply {
        h.h.setInvalid();
        h.h.a = 48w1;
        h.eth_hdr.src_addr = 48w1;
    }
}

because the assumption is that h.eth_hdr.src_addr is always 1. There may be undefined cases where the inequality is true, however. Or can this be treated as a case where all bets are off?

strange_set_valid.p4.txt strange_set_valid.stf.txt

About this issue

  • Original URL
  • State: open
  • Created 4 years ago
  • Comments: 19 (14 by maintainers)

Most upvoted comments

A note of explanation on the simple_switch behavior. p4c translates setValid calls into calls to the add_header primitive action, which is implemented by simple_switch. This primitive action comes from P4_14, and the specification for it reads as follows:

The indicated header instance is set valid. If the header instance was invalid, all its fields are initialized to 0, and if the header instance was already valid, it is not changed.

Ideally, we should have a set_valid in the bmv2 core primitive actions, which could implement a different behavior if needed. We could even use it for simple_switch and deprecate add_header altogether, but the compiler would need to generate the zero assignments when translating from P4_14 to P4_16 (which I do not think it does today - @mbudiu-vmw) .

The spec says that reads to a field in an invalid header or where the field has not yet been initialized may return an arbitrary value.

Writing to an invalid header may not change any of the defined state in the program – e.g., making invalid headers valid or changing fields in valid headers.

So both semantics are in line with the spec. The front-end is optimizing with a particular semantics in various places.