Re: [PATCH] c++: Implement C++26 P2809R3 - Trivial infinite loops are not Undefined Behavior

[Jason Merrill <jason@redhat.com>]

On 4/3/24 03:25, Jakub Jelinek wrote:
> Hi!
> 
> The following patch attempts to implement P2809R3, which has been voted
> in as a DR.
> 
> The middle-end has its behavior documented:
> '-ffinite-loops'
>       Assume that a loop with an exit will eventually take the exit and
>       not loop indefinitely.  This allows the compiler to remove loops
>       that otherwise have no side-effects, not considering eventual
>       endless looping as such.
> 
>       This option is enabled by default at '-O2' for C++ with -std=c++11
>       or higher.
> 
> So, the following patch attempts to detect trivial infinite loops and
> turn their conditions into INTEGER_CSTs so that they don't really have
> exits in the middle-end and so regardless of -ffinite-loops or
> -fno-finite-loops they are handled as infinite loops by the middle-end.
> Otherwise, if the condition would be a large expression calling various
> constexpr functions, I'd be afraid we could e.g. just inline some of them
> and not all of them and the middle-end could still see tests in the
> condition and with -ffinite-loops optimize it by assuming that such loops
> need to be finite.
> 
> The "A trivial infinite loop is a trivially empty iteration statement for
> which the converted controlling expression is a constant expression, when
> interpreted as a constant-expression ([expr.const]), and evaluates to true."
> wording isn't clear to me what it implies for manifest constant evaluation
> of the expression, especially given the
> int x = 42;
> while (std::is_constant_evaluated() || --x) ;
> example in the rationale.

The "interpreted as a constant-expression" wording was specifically 
intended (per CWG email discussion) to mean manifestly 
constant-evaluated.  I also note that the paper expects

   while (std::is_constant_evaluated() || --x) ;

to be recognized as a trivial infinite loop, which means treating the 
condition as manifestly constant-evaluated.

> The patch assumes that the condition expression aren't manifestly constant
> evaluated.  If it would be supposed to be manifestly constant evaluated,
> then I think the DR would significantly change behavior of existing programs
> and have really weird effects.  Before the DR has been voted in, I think
> void foo (int x)
> {
>    if (x == 0)
>      while (std::is_constant_evaluated())
>        ;
>    else
>      while (!std::is_constant_evaluated())
>        ;
> }
> would have well defined behavior of zero loop body iterations if x == 0 and
> undefined behavior otherwise.  If the condition expression is manifestly
> constant evaluated if it evaluates to true, and otherwise can be
> non-constant or not manifestly constant evaluated otherwise, then the
> behavior would be that for x == 0 it is well defined trvial infinite loop,
> while for x != 0 it would keep to be undefined behavior (infinite loop,
> as !std::is_constant_evaluated() is false when manifestly constant evaluated
> and if we keep the condition as is, evaluates then to true.  I think it
> would be fairly strange if both loops are infinite even when their condition
> are negated.  Similar for anything that is dependent on if consteval or
> std::is_constant_evaluated() inside of it.

Using std::is_constant_evaluated directly in a loop condition is, as the 
paper says, unlikely and "horrendous code", so I'm not concerned about 
surprising effects, though I guess we should check for it with 
maybe_warn_for_constant_evaluated.

> So, the patch below attempts to discover trivially empty iteration
> statements at cp_fold time if it is the final mce_false folding,
> attempts to maybe_constant_value with mce_false evaluate the conditions
> and replaces it with the returned value if constant non-zero.

Please refactor this code to share most of the implementation between 
the loop types.

> The testcases then try to check if the FE changed the calls in the
> conditions into constants.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> Or is it really supposed to be mce_true with the above described weird
> behavior?  If so, I think the standard at least should mention it in Annex C
> (though, where when it is a DR?).

Good question, I'll raise this with CWG.

Jason