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 question on uninitialized local headers #2383

Closed
fruffy opened this issue May 18, 2020 · 1 comment · Fixed by #2385
Closed

Clarification question on uninitialized local headers #2383

fruffy opened this issue May 18, 2020 · 1 comment · Fixed by #2385
Assignees
Labels
bug This behavior is unintended and should be fixed. fixed This topic is considered to be fixed.

Comments

@fruffy
Copy link
Collaborator

fruffy commented May 18, 2020

After the recent discussions and spec merges I have a (hopefully) last question on header initialization. Are these constructs equivalent?

ethernet_t tmp;
tmp = { 48w1, 48w1, 16w1 };
ethernet_t tmp;
tmp.dst_addr = 48w1;
tmp.src_addr = 48w1;
tmp.eth_type = 16w1;

My initial guess was no, since the in the first example the header ist set to valid, whereas in the second I assume the header stays invalid. Otherwise any assignment to an invalid header would set it to valid again.

Based on my assumptions I have observed potentially incorrect behavior in the "CopyStructures" pass. The example program this time is actually something a sane person might write:

ethernet_t do_function() {
    return  {1, 1, 1};
}
control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
    apply {
        h.eth_hdr = do_function();
    }
}

With BMV2::SimpleSwitchMidEnd_19_CopyStructures

control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
    bool hasReturned;
    ethernet_t retval;
    apply {
        {
            hasReturned = false;
            hasReturned = true;
            retval = { 48w1, 48w1, 16w1 };
            h.eth_hdr = retval;
        }
    }
}

is converted to

control ingress(inout Headers h, inout Meta m, inout standard_metadata_t sm) {
    bool hasReturned;
    ethernet_t retval;
    apply {
        {
            hasReturned = false;
            hasReturned = true;
            {
                retval.dst_addr = 48w1;
                retval.src_addr = 48w1;
                retval.eth_type = 16w1;
            }
            h.eth_hdr = retval;
        }
    }
}

Which lacks the setValid call. Maybe there is already a plan to fix this with the setHeaders pass?.

header_function_cast.p4.txt
header_function_cast.stf.txt

@jafingerhut
Copy link
Contributor

My understanding is that your assumption described early in the comment is correct:

The first code snippet will make the header tmp valid, as a result of the assignment tmp = { 48w1, 48w1, 16w1 };, and at the end all 3 fields of the header are guaranteed to have the indicated values.

The second code snippet will guarantee that tmp is initialized to invalid, and the later assignments to the fields of tmp will leave the header still invalid, and reading any of the fields after those assignments will not guarantee any particular read value, as those fields are still undefined.

@mihaibudiu mihaibudiu added bug This behavior is unintended and should be fixed. fixed This topic is considered to be fixed. labels May 19, 2020
@mihaibudiu mihaibudiu self-assigned this May 19, 2020
anasyrmia added a commit to anasyrmia/p4c-1 that referenced this issue Aug 18, 2020
Frontend pass setHeaders shifted after InlineFunctions pass, so that return values of functions could be processed as right operands of assignment statements, where left operand is header type or a header field. The idea was to remove the redundant setValid() calls in some outputs. The solution was tested on code given in issue 2383, and tests that had a duplicated setValid() call.

With this change, there is no need for checking for header types when handling structures or lists in copyStructures pass in midend, because setValid() call is handled in frontend. There are no changes at the end of midend in test p4lang#2383 after implementing these changes, and tests with redundant setValid() calls have different outputs. For an example, in test issue2261.p4, there is no redundant setValid() call after midend anymore:

    @hidden action issue2261l22() {
        tmp_struct_0_eth_hdr.setValid();
        tmp_struct_0_eth_hdr.dst_addr = 48w0;
        tmp_struct_0_eth_hdr.src_addr = 48w0;
        tmp_struct_0_eth_hdr.eth_type = 16w0;
        h.eth_hdr.eth_type = 16w0;
    }
anasyrmia added a commit to anasyrmia/p4c-1 that referenced this issue Aug 21, 2020
Frontend pass setHeaders shifted after InlineFunctions pass, so that return values of functions could be processed as right operands of assignment statements, where left operand is header type or a header field. The idea was to remove the redundant setValid() calls in some outputs. The solution was tested on code given in issue 2383, and tests that had a duplicated setValid() call.

With this change, there is no need for checking for header types when handling structures or lists in copyStructures pass in midend, because setValid() call is handled in frontend. There are no changes at the end of midend in test p4lang#2383 after implementing these changes, and tests with redundant setValid() calls have different outputs. For an example, in test issue2261.p4, there is no redundant setValid() call after midend anymore:

    @hidden action issue2261l22() {
        tmp_struct_0_eth_hdr.setValid();
        tmp_struct_0_eth_hdr.dst_addr = 48w0;
        tmp_struct_0_eth_hdr.src_addr = 48w0;
        tmp_struct_0_eth_hdr.eth_type = 16w0;
        h.eth_hdr.eth_type = 16w0;
    }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This behavior is unintended and should be fixed. fixed This topic is considered to be fixed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants