stabilize extern_system_varargs

[RalfJung]

Based on top of #144066. This has been already FCP'd over there, but @workingjubilee has some concerns regarding "system" varargs specifically (IIUC).

[rustbot]

r? @jackh726

rustbot has assigned @jackh726.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

HIR ty lowering was modified

cc @fmease

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[jackh726]

So, uh, waiting on @workingjubilee to weigh in? What's the status here?

[RalfJung]

@workingjubilee how do you suggest we proceed here? I'd rather not delay this indefinitely...

[jackh726]

So, I don't want to delay this longer this is should be, but I also don't have the full context here on whether this should be merged. From a reviewer perspective this is fairly simple and r=me on that.

But, I'm not sure if this is going to need another lang FCP? Or at least lang signoff?

[jackh726]

Going to nominate for lang. From I can tell from the background here, @workingjubilee had questions about testing: #144066 (comment)

Is this PR covered under the FCP in #144066? It doesn't seem at all like the lang team waited for a "resolution" to the concerns raised by jubilee, so I'm curious on current thoughts.

[folkertdev]

I don't think test coverage has improved since this PR was opened? All we have, as far as I can tell from the diff here, is this codegen-llvm test

rust/tests/codegen-llvm/cffi/c-variadic-ffi.rs

Lines 29 to 35 in 4b53279

	// CHECK-LABEL: @system
	#[unsafe(no_mangle)]
	fn system(f: extern "system" fn(i32, ...)) {
	// Crucially, this is *always* the C calling convention, even on Windows.
	// CHECK: call void (i32, ...)
	f(22, 44);
	}

I suppose that comment is technically correct, but could be clearer, e.g. by including what the reference says:

https://doc.rust-lang.org/nightly/reference/items/external-blocks.html?highlight=c-unwind#r-items.extern.abi.system

unsafe extern "system" — This is equivalent to extern "C" except on Windows x86_32 where it is equivalent to "stdcall".

---

Now to be fair I don't think our testing is very thorough, we mostly just check that we give something that looks reasonable to LLVM. Whether it actually runs in practice is mostly only tested for extern "C". E.g. extern "efiapi" does not have any specific tests for c-variadics either beyond this LLVM check, and that did get stabilized earlier.

[RalfJung]

Yeah that's the usual test coverage for features like this -- we check that we generate the LLVM IR we intend to generate.

Specifically for "system", which is an ABI we made up that, when there are varargs, we map to the C ABI for LLVM (for which varagrs are already stable!), I think this is entirely reasonable. Any LLVM bug affecting "system" varargs would also affect "C" varargs.

[joshtriplett]

In terms of test coverage, it sounds like it might be helpful to have an additional test verifying on x86-32 Windows that an extern "system" varargs function specifically generates a function using C-calling-convention rather than stdcall-calling-convention?

[traviscross]

Is this PR covered under the FCP in #144066? It doesn't seem at all like the lang team waited for a "resolution" to the concerns raised by jubilee...

As a procedural matter, though the FCP in #144066 did originally cover what's here, the fact that it was merged without that puts the ball back in our court, in the same way that if one of our stabilizations is reverted, we then need to re-FCP the restabilization. In this way, our FCPs represent a "point-in-time" decision to stabilize something.

We discussed that procedural analysis in the meeting and it seemed right to people.

[traviscross]

On the matter of stabilizing this, we see that there were maybe asks for more tests to ensure correctness of implementation. If we needed to see more tests to be sure of the bounds of what we're stabilizing as a language matter, that's somewhere that we would hold the stabilization from the lang side. When it's correctness of implementation, we can defer to compiler on that.

Given that, in the meeting we were happy to go ahead and propose this for stabilization, so...

@rfcbot fcp merge

[rust-rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[nikomatsakis]

@rfcbot reviewed

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[folkertdev]

In terms of test coverage, it sounds like it might be helpful to have an additional test verifying on x86-32 Windows that an extern "system" varargs function specifically generates a function using C-calling-convention rather than stdcall-calling-convention?

The test I quoted above checks that. It asserts that in LLVM IR we see a call without any explicit calling convention, C is the default:

 #[unsafe(no_mangle)] 
 fn system(f: extern "system" fn(i32, ...)) { 
     // Crucially, this is *always* the C calling convention, even on Windows. 
     // CHECK: call void (i32, ...) 
     f(22, 44); 
 } 

As mentioned, there is not that much more we can really do.

[RalfJung]

And we specifically run that test with --target i686-pc-windows-msvc (among others). So yeah, that part we have covered.