Fix Async Generator ABI

[Swatinem]

This change was missed when making async generators implement Future directly.
It did not cause any problems in codegen so far, as GeneratorState<(), Output>
happens to have the same ABI as Poll<Output>.

[rustbot]

r? @compiler-errors

(rustbot has picked a reviewer for you, use r? to override)

[compiler-errors]

1. This needs a test

2. Doesn't this break using async {} as a generator? There's nothing keepin someone from passing async {} into a parameter accepting _: impl Generator, which is the opposite of this problem, right?

3. How did you find this? Does this make Miri angry? Or what?

[Swatinem]

1. This needs a test

Can you give a suggestion how to specifically test for the ABI here?

2. Doesn't this break using async {} as a generator? There's nothing keeping one someone from passing async {} into a parameter accepting _: impl Generator, which is the opposite of this problem, right?

async blocks / fns always went through from_generator, and now through identity_future, so they do not implement Generator directly, though this is indeed worth a test.
And this is also something to watch out for when tackling #104826.

3. How did you find this? Does this make Miri angry? Or what?

I started looking at #104828, searching for all the places that use resume_ty.

[bjorn3]

Friendly ping. This is blocking a rustup for cg_clif.

[Swatinem]

I think this is blocked on figuring out a good way to regression test against this in the future. I would happily do that as a followup though to unblock cg_clif.

With #104828 / #105250 I contiously took advantage that the compiler was very lenient and did not verify these things too strictly. This bug here was rather by accident. Ideally both should be covered by tests that avoid similar things in the future.

[bjorn3]

I'm hoping to integrate cg_clif's test suite with rust's CI soonish, which should catch any regressions. A regular test would indeed be nice though. Would a MIR dump after the generator transformation for the generator of an async function that uses .await work? That should show both the function signature and what the created MIR expects as type.

[Swatinem]

That might indeed be an option, I will experiment with that tomorrow.

[Swatinem]

The MIR type is not the problem here, and it should be coming through correctly from the generator transform:

rust/compiler/rustc_mir_transform/src/generator.rs

Lines 1301 to 1314 in 7632db0

	let (state_adt_ref, state_substs) = if is_async_kind {
	// Compute Poll<return_ty>
	let state_did = tcx.require_lang_item(LangItem::Poll, None);
	let state_adt_ref = tcx.adt_def(state_did);
	let state_substs = tcx.intern_substs(&[body.return_ty().into()]);
	(state_adt_ref, state_substs)
	} else {
	// Compute GeneratorState<yield_ty, return_ty>
	let state_did = tcx.require_lang_item(LangItem::GeneratorState, None);
	let state_adt_ref = tcx.adt_def(state_did);
	let state_substs = tcx.intern_substs(&[yield_ty.into(), body.return_ty().into()]);
	(state_adt_ref, state_substs)
	};
	let ret_ty = tcx.mk_adt(state_adt_ref, state_substs);

fn future::{closure#0}(_1: Pin<&mut [async fn body@src/main.rs:20:19: 20:21]>, _2: &mut Context<'_>) -> Poll<()> {

The problem is rather that cg_clif and rustc_const_eval are the only places where the FnAbi of call terminators is actually checked. And you can’t call the non-const poll fn.

rust/compiler/rustc_const_eval/src/interpret/terminator.rs

Line 509 in bf14e31

if !Self::check_argument_compat(&caller_fn_abi.ret, &callee_fn_abi.ret) {

I don’t see the FnAbi being checked anywhere in rustc_codegen_ssa / codegen_call_terminator.

[compiler-errors]

Hm, is check_argument_compat succeeding because even though the types are unequal, they have compatible layouts?

[compiler-errors]

Also, what is clif doing differently? Is it checking full type equality? Or does it just do layout for Poll vs GeneratorState differently so that they're unequal?

[Swatinem]

I don’t think we are hitting check_argument_compat at all, as that is part of rustc_const_eval which we are not going through.

For clif, I believe it is doing full type equality, but @bjorn3 can answer that better I’m sure.

[compiler-errors]

rustc_const_eval is used by miri, though -- but it's just checking that size and align are equal, I think.

anyways, since this is blocking @bjorn3's upgrade, then this is fine without a test. @Swatinem, could you add a comment explaining this check on line 107? i'll approve after.

[Swatinem]

Rebased and added a comment.

[bjorn3]

Yeah, cg_clif is checking full type equality. It has catched a lot of bugs while working on cg_clif itself. However even when relaxing the check to allow Poll/GeneratorState mismatches I still get a crash as the generated MIR attempts to write to field 0 of the return value which is an i32 for one type and () for the other type.

[compiler-errors]

@bors r+

[bors]

📌 Commit ecf8127 has been approved by compiler-errors

It is now in the queue for this repository.

[bjorn3]

I can confirm that all issues for cg_clif are fixed with this PR. I will update cg_clif once a nightly with this PR is released.

[cuviper]

It did not cause any problems in codegen so far,

Just for reference, this did manifest as a crash on Fedora 36 with LLVM 14:
https://bugzilla.redhat.com/show_bug.cgi?id=2168622

So far, backporting this PR does appear to fix that, thanks!

[bjorn3]

Do we have any builders with both LLVM 14 and LLVM assertions enabled? If not I think the crash was due to pointer types being mixed up. LLVM 15 uses opaque pointers and as such there is nothing to mixup.

[cuviper]

I added LLVM 14 in #107044, but that's Ubuntu's normal package. I think we only have CI builds with assertions in the bundled LLVM.