p4lang / p4c

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

If and return/exit statements causing uninitialized_use warnings #4385

Open Trigary opened 7 months ago

Trigary commented 7 months ago

Compiling the following code:

apply {
    if (standard_metadata.ingress_global_timestamp == 123) return;
    bit<32> x = 42;
    if (standard_metadata.packet_length == 456) return;
    log_msg("test {}", {x});
}

Results in the following warning, which I believe is incorrect behaviour:

user@xyz:/workspace# p4c-bm2-ss switch.p4
switch.p4(24): [--Wwarn=uninitialized_use] warning: x may be uninitialized
        log_msg("test {}", {x});
                            ^
user@xyz:/workspace#

The warning persists if I replace exactly one return with exit, but the warning disappears if I replace both. The warning also disappears if I use a constant as the if statement condition (e.g. if (true) return;), this is why I used those standard_metadata related checks in the sample above.

p4c version: 1.2.4.2
These warnings were not present in the last p4c version I used, which I believe was 1.2.3.0 or 1.2.2.0.

I attached the full source file for your convenience: switch.zip

kfcripps commented 2 months ago

This was also introduced by e60cb8a633e305a75cb28c56dbd3d1dbccc871c8 (along with #4500, #4507, #4548).

kfcripps commented 1 month ago

e60cb8a633e305a75cb28c56dbd3d1dbccc871c8 added a second SimplifyDefUse pass, which sees the following transformed IR:

    apply {
        hasReturned = false;
        if (standard_metadata.ingress_global_timestamp == 48w123) {
            hasReturned = true;
        }
        if (hasReturned) {
            ;
        } else {
            x_0 = 32w42;
            if (standard_metadata.packet_length == 32w456) {
                hasReturned = true;
            }
        }
        if (hasReturned) {
            ;
        } else {
            log_msg<tuple<bit<32>>>("test {}", { x_0 });
        }
    }

whereas the first pass sees:

    apply {
        if (standard_metadata.ingress_global_timestamp == 48w123) {
            return;
        }
        x_0 = 32w42;
        if (standard_metadata.packet_length == 32w456) {
            return;
        }
        log_msg<tuple<bit<32>>>("test {}", { x_0 });
    }

The IR looks correct, but SimplifyDefUse, which emits the warning, is probably not able to determine that x_0 is guaranteed to be initialized when hasReturned is false.