Do not print confusing warning when a parser state contains an assignment to an l-value slice

[kfcripps]

I don't think it makes sense to print a warning about an l-value being uninitialized when either the entire l-value or a slice of said l-value is written to.

On main branch, the added P4 program results in:

$ p4test tmp.p4 --dump t --loopsUnroll
tmp.p4(9): [--Wwarn=uninitialized_use] warning: s.f may be uninitialized
        s.f[3:0] = 2;
        ^^^
tmp.p4(9): [--Wwarn=ignore-prop] warning: Result of 's.f[3:0] = 4w2' is not defined: Error: Uninitialized
        s.f[3:0] = 2;
                 ^

and on this branch:

$ p4test tmp.p4 --dump t --loopsUnroll
tmp.p4(9): [--Wwarn=uninitialized_use] warning: s.f may be uninitialized
        s.f[3:0] = 2;
        ^^^

Ideally both warnings would be gotten rid of, but I don't know how to get rid of the first one, and the second one is the most confusing to me. This is at least an improvement over the previous behavior. I am open to suggestions if anyone has any ideas of how to get rid of the first warning also.

[kfcripps]

The fact that this warning is worded this way is not great - I opened #4945 to improve this and similar warnings

[fruffy]

It makes sense to print a warning if only parts of the lvalue are written to but I agree, if we are assigning to the whole slice this warning is unnecessary.

On that note, which passes require this interpreter? It is rather hard to maintain and produces lots of confusing warnings like this one.

[fruffy]

The warning itself occurs in the parserUnroll pass. I think to get rid of it you need to insert some logic to check that the slice being assigned properly. We can also fix the warning there.

[kfcripps]

It makes sense to print a warning if only parts of the lvalue are written to but I agree, if we are assigning to the whole slice this warning is unnecessary.

Can you elaborate why it makes sense to emit an "uninitialized" warning when even a partial slice is written? My motivation for this PR is that no warning should even be emitted in this case.

Why should writing to an uninitialized field slice be treated any differently than writing to an uninitialized whole field, which results in no such warning?

On that note, which passes require this interpreter? It is rather hard to maintain and produces lots of confusing warnings like this one.

My understanding is that it is used by the ParsersUnroll pass, but I am not very familiar with this pass and not sure at this moment how to improve these warnings.

The warning itself occurs in the parserUnroll pass. I think to get rid of it you need to insert some logic to check that the slice being assigned properly. We can also fix the warning there.

Are you referring to the "s.f may be uninitialized" or "Result of 's.f[3:0] = 4w2' is not defined: Error: Uninitialized" warning? The SimplifyDefUse pass is responsible for the former, and ParsersUnroll for the latter.

[fruffy]

Can you elaborate why it makes sense to emit an "uninitialized" warning when even a partial slice is written? My motivation for this PR is that no warning should even be emitted in this case.

Well, this was assuming the warning makes sense at all and there was a particular motivation why it was put there for an assignment statement. In the case of a partial write, there are still some uninitialized bits which could have unpredictable values.

My understanding is that it is used by the ParsersUnroll pass, but I am not very familiar with this pass and not sure at this moment how to improve these warnings.

I think there is no one left that understands this pass but it might make sense to change the success criterion here: https://github.com/p4lang/p4c/blob/main/midend/parserUnroll.cpp#L470

Are you referring to the "s.f may be uninitialized" or "Result of 's.f[3:0] = 4w2' is not defined: Error: Uninitialized" warning? The SimplifyDefUse pass is responsible for the former, and ParsersUnroll for the latter.

The latter, although I also assumed that this pass triggered SimplifyDefUse in some form. If that pass is throwing the warning we should fix probably fix it because it is a much more commonly used pass.

[kfcripps]

Well, this was assuming the warning makes sense at all and there was a particular motivation why it was put there for an assignment statement. In the case of a partial write, there are still some uninitialized bits which could have unpredictable values.

The point of this PR is that I don't believe this warning makes sense at all. Yes, a read of a potentially uninitialized slice should result in a warning, but a warning for a write to such slice is useless. You can see from the attached tests that the latter now results in no warning, but the former still does result in a warning.

Furthermore, I have found no evidence in the code or in the history that this warning was intentionally added. It seems more likely to be an oversight to me (the author of ExpressionEvaluator::postorder(const IR::Operation_Ternary *expression) could have forgotten that an IR::Operation_Ternary can be an IR::Slice on the LHS of an assignment).

I think there is no one left that understands this pass but it might make sense to change the success criterion here: https://github.com/p4lang/p4c/blob/main/midend/parserUnroll.cpp#L470

Is there a particular reason that modifying the success criterion here is preferable to the changes I have made to ExpressionEvaluator::postorder(const IR::Operation_Ternary *expression) in this PR?

The latter, although I also assumed that this pass triggered SimplifyDefUse in some form. If that pass is throwing the warning we should fix probably fix it because it is a much more commonly used pass.

To my knowledge ParsersUnroll and SimplifyDefUse are unrelated, and SimplifyDefUse is only called at the top-level from frontend.cpp. If you have a concrete suggestion on how to fix the similar warning in SimplifyDefUse, I will try it, but otherwise I'd prefer to merge this PR first and address this other warning in a different PR.

[fruffy]

My main concern is understanding how or what it even means in the interpreter to evaluate an lvalue in postorder here. Is that even supposed to happen? I think your solution is fine but it might be more of a band-aid.

It makes sense to handle this warning on higher-levels, e.g., in the parserUnroll pass, which is throwing an error on resolving an lvalue.

The reason I am concerned about SimplifyDefUse is that it has a different logic but is throwing the same type of error.

[fruffy]

I would throw a BUG if the value is an lval and the ternary is not a slice. To make sure we do not end up in weird states.

[kfcripps]

Done

[kfcripps]

My main concern is understanding how or what it even means in the interpreter to evaluate an lvalue in postorder here. Is that even supposed to happen? I think your solution is fine but it might be more of a band-aid.

It makes sense to handle this warning on higher-levels, e.g., in the parserUnroll pass, which is throwing an error on resolving an lvalue.

In theory, I think an error may still occur when evaluating LHS in other cases. For example, an assignment to an ArrayIndex in which the index is uninitialized (for example, a[x] = rhs; where x is uninitialized). Another example is an assignment to a PlusSlice, for example a[x+:y] = rhs, where x in uninitialized. In both cases, the LHS expression actually reads uninitialized data (unlike with LHS Slice expressions), so the warning would be valid. I don't think a SymbolicStaticError is created in either of these cases currently, and I don't know the pass well enough to know if they should result in a SymbolicStaticError, but my guess is that they should and that it probably should be propagated up to executeStatement where IR::Assignment is handled.