wrong code due to incompatibility between C++20 coroutines and 32-bit Windows x86 ABI

[zygoloid]

Testcase is a little large, see it on compiler explorer here.

The 32-bit Windows ABI passes objects of non-trivially-copyable class type by value on the stack.
The C++20 coroutines feature can suspend execution after an argument to a function has finished being evaluated and before the function call happens. In this testcase, that happens here:

  consume_two(co_await make(), co_await make());

This code is therefore not lowerable: the language rules don't permit us to make a copy of the result of the first argument, the ABI requires that we construct it directly into a stack slot where consume_two will find it, and the function containing that stack slot returns between the object being constructed and the call to consume_two.

After the coroutine lowering pass runs, we have this IR:

define internal fastcc void @"?my_task@@YA?AUtask@@XZ.resume"(ptr noalias nonnull align 4 dereferenceable(72) %0) #3 personality ptr @__CxxFrameHandler3 !dbg !880 {
  ; ...
  %argmem.reload.addr = getelementptr inbounds %"?my_task@@YA?AUtask@@XZ.Frame", ptr %0, i32 0, i32 6, !dbg !884
  ; ...
  call void @"?consume_two@@YAXUNoisy@@0@Z"(ptr inalloca(<{ %struct.Noisy, %struct.Noisy }>) %argmem.reload.addr), !dbg !886

... which doesn't satisfy the rules for inalloca arguments. (The verifier doesn't complain about this, but presumably it should!)

It's not clear to me what the machine code we generate in this case actually does, and I don't have a Windows machine / emulator on which to test, but I don't see how it can be generating correct code!

We should decide what we want to do here. It seems like there are a few options:

• We could reject use of C++20 coroutines on this ABI.
• We could introduce an extra copy of these function arguments in the case where the function evaluation could be interrupted by a suspension. Note that this is not conforming and would reject valid code; the testcase is valid even if Noisy is noncopyable.
• We could reorder the evaluation of function arguments on this ABI so that all suspensions are performed before any arguments are constructed. Note that this is not conforming due to the C++17 evaluation order rules, but we already don't follow those rules in some cases because they're incompatible with callee-cleanup argument passing.

MSVC appears to do something like the third option. Note that MSVC doesn't call await_resume on either awaitable until both are ready. While this behavior directly violates the C++ sequencing rules, which don't permit anything else to be interleaved between the await_suspend and await_resume calls on a coroutine, maybe that's the best we can do, and it would be compatible with MSVC.

[llvmbot]

@llvm/issue-subscribers-clang-codegen

[llvmbot]

@llvm/issue-subscribers-c-20

[llvmbot]

@llvm/issue-subscribers-coroutines

[zygoloid]

@GorNishanov @ilya-biryukov

[iains]

I wonder if this is not a symptom of a more general problem e.g.

result = co_await t + co_await t;

where the await_resume() for the coroutine has a side-effect that changes the operation of a subsequent invocation. (I have PR against the GCC impl. raised by Lewis for this specific case).

It is my current plan for GCC to ensure that, effectively we compute this like

result = co_await t ;
result += co_await t;

(and similar constraints for other cases, such as the function arguments in this PR).

edit: in the case above it assumes that the LHS and RHS are the same type (otherwise:

 temp = co_await t ;
 result = temp + co_await t ;

)

edit2: I have also over-simplified this for the sake of exposition - the proposed impl. treats the rewrite more like a GNU statement expression, since we need to handle lifetime extensions of temporaries where required to the end of the original statement.

If we say that this is invalid user code, it's not clear to me how we can diagnose it?

[ChuanqiXu9]

BTW, it looks like that coroutines on Windows is going to be out of maintenance... This is the 4th bug report in the last 4 months: #58556, #58543, #56989

I didn't look deeply into them since I don't have a windows environment too.

[h-vetinari]

[...] We could reorder the evaluation of function arguments on this ABI so that all suspensions are performed before any arguments are constructed. Note that this is not conforming due to the C++17 evaluation order rules, but we already don't follow those rules in some cases because they're incompatible with callee-cleanup argument passing.

MSVC appears to do something like [this]. Note that MSVC doesn't call await_resume on either awaitable until both are ready. While this behavior directly violates the C++ sequencing rules, which don't permit anything else to be interleaved between the await_suspend and await_resume calls on a coroutine, maybe that's the best we can do, and it would be compatible with MSVC.

Is there a CWG issue for these inconsistencies resp. outright impossibility to conform? Does anyone even care (sufficiently) about the 32-bit ABI these days for this to really matter?