process bug: stabilization of extended_varargs_abi_support without FCP?

[workingjubilee]

In #116161 the extended_varargs_abi_support feature was stabilized, effectively without a stabilization report. As far as I can find, it was also without an FCP for either relevant team, neither T-compiler nor T-lang. It seems like it wasn't really discussed extensively, or at least I can't find the meat of the discussion? I am guessing people felt it was conceptually good to go ahead... but no FCP was initiated, and there was no RFC for it either.

There is a bit of confusion to me on this point because extern "system", on the (ahem) systems where it is meaningful (i.e. distinct from extern "C" in some way), is translated to extern "stdcall" which is incompatible in varargs. In the PR where extern "system" was added to extended_varargs_abi_support, it was proposed to make extern "system" transparently translate to extern "C" when varargs are involved because sometimes the MSVC toolchain will make that implicit translation: #119587 (comment)

However, in #87678 we added a future compatibility warning specifically for the fact that we realized, a bit late, that we didn't actually want to support this sort of implicit ABI translation, and preferred to have these names be, er... meaningful? We had decided we wanted to limit the use of these ABIs to cases where they had an explicit and clearly indicated meaning, so that we can e.g. give them the meanings they may need to have if they are in fact given such a meaning in the future... or at least not have pointless confusion.

Thus there are a few problems, mostly about process:

• A decision that needed an FCP (from some team) glided through without one?
• The initial feature kept feature-creeping through gradual additions without much discussion and the "has this been suitably reflected on?" checking process (a stability report) did not kick in, resulting in a decision that seems inconsistent being made without apparent discussion of that inconsistency.

[workingjubilee]

I will admit that I noticed all this because I was working out a way to map between all the ABIs that we have that effectively alias each other so that some of our tooling can emit useful errors when we catch an ABI-incompatible call in e.g. miri, as on some platforms the following is perfectly valid and the truth of the matter can only be determined via dynamic checks:

extern "system" {
    fn system_abi_function(input: i32);
}
mem::transmute::<_, extern "C" fn()>(system_abi_function)(5i32)

The "extern "system" but it's varargs" case introduced a surprising amount of complication in the implementation I was working on, which prompted me to take a closer look, along with @chorman0773, at everything. Such a complication may make it less likely that our error messages will be correct, useful, or understood, though I haven't worked through all the parts of it... we noticed the recent stabilization, were surprised, and @jieyouxu noticed this was a stabilization without an FCP, at which point I kinda dropped everything.

Anyway, I will divert any further comments on the practical end of this to #100189 or related issues.

[apiraino]

Given the context I am slowly trying to get about this topic, I think this was an evaluation error from both T-lang and T-compiler, we stabilized something that shouldn't. If reverting in #136897 won't cause now any problems, I think it would make sense to do that.

The other question is of course why was it stabilized in the first place: were the approvals given with enough context? That's probably why the FCP could have helped (getting more expert eyes). AFAICS the rough timeline was:

• Was nominated first for T-lang discussion
• T-lang approved (meeting notes)
• Then T-compiler approved (unsure if due to lack of NACKs - it was also during Eurorust conference)

[jieyouxu]

I suspect it was a misunderstanding that everyone thought T-lang approved (or rather, FCP'd somewhere, including T-lang ppl themselves). There was also a later T-compiler triage meeting general "in favor" vibe check, which I suspect added to the misunderstanding.

[apiraino]

@workingjubilee to be sure I understand: the technical discussion about the revert should happen in #136897 and this issue is to discuss a procedure failure (or misunderstanding), correct? Asking because the former one could probably be quickly approved and pushed forward in the short time we have before the release. OTOH the discussion about procedures maybe takes more time (ref: the recent discussion about a better, cross-team, process for stabilizations)

[jieyouxu]

@apiraino The revert PR (#136897) is already in the queue but will need T-compiler (or T-lang) approval for beta-backport¹ (in fact, it's being tested right now), this issue is specifically to discuss the process problem (and maybe how we can make this less likely to happen in the future).

Footnotes

1. This should be very easy because it never went through FCP ↩

[jieyouxu]

Relevant WIP rustc-dev-guide PR on stabilization procedures: rust-lang/rustc-dev-guide#2219

[RalfJung]

However, in #87678 we added a future compatibility warning specifically for the fact that we realized, a bit late, that we didn't actually want to support this sort of implicit ABI translation, and preferred to have these names be, er... meaningful?

FWIW we only mostly, but not entirely, ended up implementing this: stdcall, fastcall, and vectorcall are accepted on all Windows targets, even though they only make sense on some of them. See here for the long comment explaining that. These then fall back to be equivalent to C on targets where they do not make sense.

[WaffleLapkin]

I think one big problem with the process that happened is that there was no stabilization report, so everyone approved their idea of the feature, rather than the actual feature as-is-implemented. Although the lack of FCP is also a separate problem.

[workingjubilee]

I just noticed the dates?

• The stabilization PR was opened on September 2023: Stabilize extended_varargs_abi_support #116161
• The PR that landed the "weird" case was opened in January 2024: Varargs support for system ABI #119587

And the author of the stabilization PR seemed to be aware of the PR, but the change to make the feature significantly more "powerful" (in terms of what impacts it might have) didn't get noted down...???

[RalfJung]

I would argue #119587 is the one that should have been reverted. But anyway, reverting the stabilization is definitely the safer choice.

[workingjubilee]

Well, I have beef with the stabilization too, because I agree with Waffle. I feel like the hack here is definitely "submit a stabilization PR without including anything resembling a stabilization report, allowing everyone to project their own idea of what it means onto it, and thereby get effectively anything approved".

This is funny/sad to me because anyone who writes a reasonably thorough stabilization report, even if they also make it concise and understandable, almost always gets a little more discussion of it... and thus delays. Sometimes just a day or two, but even then, whatever needs to happen to unclog that delay can be surprisingly frustrating. That makes it feel like writing a correct stability report is punished, while doing nothing is seemingly rewarded?

It's not like there was much to talk about here, at least for the original feature. The stability report for that feature could have been like a paragraph. With extern "system" it gets a lot weirder.

[chorman0773]

Yes, at the very least, IMO we should follow the proper process here. And who knows - maybe someone will uncover something else spicy in this matter that deserves at least discussion. So reverting the stabilization is probably the right call here. At the very least, to give us some breathing room to get a proper look over of a stabilization report.

[RalfJung]

Ah true, the lack of an FCP definitely means we should revert.

@workingjubilee The stabilization PR took two months from submission to merging. That doesn't look to me like it was "rewarded" or moved particularly fast.

[workingjubilee]

oh, well, that's true, I guess I'm assuming the taking forever is almost a constant, thus the "punishment" is a relatively mild "forever + 1" :^)

[traviscross]

@rustbot labels -I-lang-nominated

We discussed this in the lang call on 2025-02-12. Obviously we were in support of the revert.

On the process matter, historically, sometimes we have just waved things through when they seemed perfunctory and especially if we also had the full team together.¹ Even by September, when this was waved though under those and some other unique circumstances, we were doing less of that when it involved any kind of stabilization, and we've since moved toward doing that even less often.

Clearly something more in the way of a stabilization report might have helped here too.

Anyway, exceptional though this case was in many ways, you'll get no argument from us that this could have gone better, and we expect that changes we've already made since September will help here.

Something that came up in our discussion was that it'd be good if we could put together ping groups for special areas of expertise, not otherwise covered by our subteams, so that we have something to reach for on matters related to those areas. We'll think about how to best follow up on that.

Footnotes

1. Think, e.g., of the routine kind of judgment calls we make in deciding whether something is actually a bug fix or whether it falls near enough to something we've already approved. ↩

[RalfJung]

T-lang #116161 (comment) (I have no logs of the discussion)

@workingjubilee managed to find the meeting notes for that: https://hackmd.io/foOkwHNPRA2YFi4CMxEcoA

[apiraino]

Pasting here some thoughts during the discussion on Zulip
(relevant comments from here).

• The lack of a stabilization report or an FCP for it was overlooked, so this is a bug that slipped in (comment)
• Then some assumptions were given (t-compiler r+ was given because T-lang approval made it look like they decided on a stabilization report)
• Nikos' stabilization template could help keeping track of implementation history and important design decisions (I think relevant Zulip thread, see stabilization template, docs rustc-dev-guide#2219)
• Pinging relevant groups could also be an idea (suggested in comment)

I think the TL;DR could be: next time let's all pay more attention and ensure to crosslink documentation (report, FCP, ...) needed to rubberstamp a stabilization.

@rustbot label -I-compiler-nominated

[jieyouxu]

Nikos' stabilization template could help keeping track of implementation history and important design decisions (I think relevant Zulip thread, unsure where work is happening though)

rust-lang/rustc-dev-guide#2219

[jieyouxu]

rubberstamp a stabilization

I think Niko's stabilization template is exactly to help avoid making stabilizations seem like a rubberstamp process, since without sufficient context, it's not feasible for stabilization reviewers to try to dig through all relevant discussions/context/history/decisions themselves, especially if they aren't closely involved in the feature development.