Incorrect "variable is uninitialized when used here" when LHS of compound assignment is initialized by RHS (C++17)

[mvduin]

Since C++17 the right side of a compound-assignment expression (including any side effects) is sequenced before the left side is evaluated (and therefore before the assignment), which means the following code should be well-defined when compiled with -std=c++17 or later:

// returns x + y + carry from (x + y)
unsigned foo( unsigned x, unsigned y ) {
    unsigned z;
    z += __builtin_uadd_overflow( x, y, &z );
    return z;
}

However clang (with -O2 -Wall -std=c++17) emits the following warning:

<source>:4:5: warning: variable 'z' is uninitialized when used here [-Wuninitialized]
    z += __builtin_uadd_overflow( x, y, &z );
    ^
<source>:3:15: note: initialize the variable 'z' to silence this warning
    unsigned z;
              ^
               = 0

The issue is not specific to __builtin_uadd_overflow, replacing the assignment statement by the equivalent z += (z = x + y) < x; results in the same warning.

It appears that the generated code is still correct, but obviously I'd get rather nervous when the compiler emits a warning that implies it thinks the code has undefined behaviour.

[llvmbot]

@llvm/issue-subscribers-c-17

Since C++17 the right side of a compound-assignment expression (including any side effects) is sequenced before the left side is evaluated (and therefore before the assignment), which means the following code should be well-defined when compiled with `-std=c++17` or later: ```c++ // returns x + y + carry from (x + y) unsigned foo( unsigned x, unsigned y ) { unsigned z; z += __builtin_uadd_overflow( x, y, &z ); return z; } ``` However clang (with `-O2 -Wall -std=c++17`) emits the following warning: ``` <source>:4:5: warning: variable 'z' is uninitialized when used here [-Wuninitialized] z += __builtin_uadd_overflow( x, y, &z ); ^ <source>:3:15: note: initialize the variable 'z' to silence this warning unsigned z; ^ = 0 ``` The issue is not specific to `__builtin_uadd_overflow`, replacing the assignment statement by the equivalent `z += (z = x + y) < x;` results in the same warning. It appears that the generated code is still correct, but obviously I'd get rather nervous when the compiler emits a warning that implies it thinks the code has undefined behaviour.

[shafik]

I think the issue is more obvious if you look at it like this:

z = z + (z = x + y) ;

then both gcc and clang produce a diagnostic: https://godbolt.org/z/6P77r1has

see expr.ass p6

[mvduin]

That's because your example is invalid, z + (z = ...) is undefined behaviour since the two sides of + are not sequenced relative to each other.

[mvduin]

The relevant part to quote here is expr.ass p1: "In all cases, the assignment is sequenced after the value computation of the right and left operands, and before the value computation of the assignment expression. The right operand is sequenced before the left operand."

and since "right operand" is short for "value computation AND side effects of right operand" (intro.execution p8) we get in particular the following sequencing requirement:

side effects of right operand < value computation of left operand < assignment

which is what makes z += (z = ...) valid

[shafik]

The relevant part to quote here is expr.ass p1: "In all cases, the assignment is sequenced after the value computation of the right and left operands, and before the value computation of the assignment expression. The right operand is sequenced before the left operand."

and since "right operand" is short for "value computation AND side effects of right operand" (intro.execution p8) we get in particular the following sequencing requirement:

side effects of right operand < value computation of left operand < assignment

which is what makes z += (z = ...) valid

As I pointed out [expr.ass]p6 says:

The behavior of an expression of the form E1 op= E2 is equivalent to E1 = E1 op E2 except that E1 is evaluated only once. [Note 2: The object designated by E1 is accessed twice. — end note]

E1 is evaluated once but read twice. So I believe the correct interpretation is that E1 and E2 are not sequenced wrt each other. Relying on these subtle distinctions usually means it is better to refactor to remove any doubts and be done with it.

[mvduin]

Interestingly that note is relatively new, it's not in the C++17 final, although I don't think that note is important here. You can't access the object designated by E1 until E1 has been evaluated, and that evaluation is sequenced after E2 according to expr.ass p1. The evaluation of E1 that makes z = z + (z = ...) invalid is exactly the one that has been elided in z += (z = ...).

I'll readily admit it's probably best not to write code that depends on this, but I still think it would be a good thing for the compiler to err on the side of sequencing strictly. There's good reason C++17 increased the strictness of evaluation order, since people realized that accidental reliance on (previously undefined) evaluation order was actually fairly widespread.