Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert "Stabilize extended_varargs_abi_support" #136897

Merged
merged 2 commits into from
Feb 12, 2025
Merged

Revert "Stabilize extended_varargs_abi_support" #136897

merged 2 commits into from
Feb 12, 2025

Conversation

workingjubilee
Copy link
Member

@workingjubilee workingjubilee commented Feb 12, 2025 •
edited
Loading

I cannot find an FCP for this, despite it being a stabilization PR which normally means we do an FCP of some kind? It would seem reasonable for either compiler or lang to have FCPed it? I am thus opening a revert PR, which mostly-cleanly applies, so that we can later actually land this properly with a stability report and FCP.

@rustbot
Copy link
Collaborator

rustbot commented Feb 12, 2025

r? @estebank

rustbot has assigned @estebank.
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 rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 12, 2025
@workingjubilee
Copy link
Member Author

workingjubilee commented Feb 12, 2025 •
edited
Loading

Note that even if this is accepted I intend to almost immediately relaunch the stabilization process here for the set of ABIs for which it is simply and obviously consistent to do this for... which is basically everything except extern "system". I would not recommend backporting such a diff to our current beta, however...

@workingjubilee workingjubilee added beta-nominated Nominated for backporting to the compiler in the beta channel. I-compiler-nominated Nominated for discussion during a compiler team meeting. T-lang Relevant to the language team, which will review and decide on the PR/issue. I-lang-nominated Nominated for discussion during a lang team meeting. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 12, 2025
@workingjubilee workingjubilee mentioned this pull request Feb 12, 2025
Open
3 tasks
WaffleLapkin
WaffleLapkin approved these changes Feb 12, 2025
View reviewed changes
Copy link
Member

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with/without nitpick

compiler/rustc_feature/src/accepted.rs
@@ -197,9 +197,6 @@ declare_features! (
(accepted, expr_fragment_specifier_2024, "1.83.0", Some(123742)),
/// Allows arbitrary expressions in key-value attributes at parse time.
(accepted, extended_key_value_attributes, "1.54.0", Some(78835)),
/// Allows using `efiapi`, `aapcs`, `sysv64` and `win64` as calling
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fun how this doesn't even mention "system"...

@workingjubilee @WaffleLapkin
library: amend revert of extended_varargs_abi_support for beta diff
cafa646 
And leave a comment on the unusual `cfg_attr`

Co-authored-by: waffle <waffle.lapkin@gmail.com>
@WaffleLapkin
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 12, 2025

📌 Commit cafa646 has been approved by WaffleLapkin

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 12, 2025
@WaffleLapkin
Copy link
Member

@bors p=5

this needs to be beta backported before Friday, so let's try to merge this as soon as possible

@bors
Copy link
Contributor

bors commented Feb 12, 2025

⌛ Testing commit cafa646 with merge 021fb9c...

@apiraino apiraino mentioned this pull request Feb 12, 2025
Closed
@jieyouxu jieyouxu added the F-extended_varargs_abi_support `#![feature(extended_varargs_abi_support)]` label Feb 12, 2025
@RalfJung
Copy link
Member

RalfJung commented Feb 12, 2025 •
edited
Loading

Note that even if this is accepted I intend to almost immediately relaunch the stabilization process here for the set of ABIs for which it is simply and obviously consistent to do this for... which is basically everything except extern "system". I would not recommend backporting such a diff to our current beta, however...

Maybe "system" should just return false in fn supports_varargs ?

@ChrisDenton
Copy link
Member

That would work as quick fix but ideally it should return the same as whatever calling convention system is aliasing, no? As should C in theory, albeit I think all C calling conventions must support varags.

@bors
Copy link
Contributor

bors commented Feb 12, 2025

☀️ Test successful - checks-actions
Approved by: WaffleLapkin
Pushing 021fb9c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 12, 2025
@bors bors merged commit 021fb9c into rust-lang:master Feb 12, 2025
7 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Feb 12, 2025
@WaffleLapkin
Copy link
Member

Is anyone willing to prepare a beta backport?

This was referenced Feb 12, 2025
Open
Open
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (021fb9c): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 2.5%, secondary 1.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 2.5% [2.0%, 2.9%] 2
Regressions ❌
(secondary)		 2.5% [2.1%, 3.2%] 8
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -4.0% [-4.0%, -4.0%] 1
All ❌✅ (primary) 2.5% [2.0%, 2.9%] 2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 790.14s -> 789.14s (-0.13%)
Artifact size: 347.71 MiB -> 347.71 MiB (0.00%)

@Mark-Simulacrum Mark-Simulacrum added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Feb 12, 2025
@Mark-Simulacrum
Copy link
Member

I'll unilaterally beta accept this - reverting a stabilization feels like obviously required to backport.

@cuviper
Copy link
Member

cuviper commented Feb 12, 2025

Note, we should include the reference revert too: rust-lang/reference#1734

@WaffleLapkin WaffleLapkin mentioned this pull request Feb 12, 2025
Closed
@workingjubilee workingjubilee deleted the revert-unfcped-stab branch February 12, 2025 17:28
@cuviper cuviper mentioned this pull request Feb 13, 2025
Merged
@cuviper cuviper modified the milestones: 1.86.0, 1.85.0 Feb 13, 2025
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 13, 2025
This was referenced Feb 13, 2025
Merged
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 13, 2025
@bors
Auto merge of rust-lang#136980 - cuviper:beta-next, r=cuviper
461de74 
[beta] backports

- Pattern Migration 2024: try to suggest eliding redundant binding modifiers rust-lang#136577, rust-lang#136857
- chore: update rustc-hash 2.1.0 to 2.1.1 rust-lang#136605
- Make `AsyncFnOnce`, `AsyncFnMut`, `AsyncFn` non-`#[fundamental]` rust-lang#136724
- fix ensure_monomorphic_enough rust-lang#136839
- Revert "Stabilize `extended_varargs_abi_support`" rust-lang#136897, rust-lang#136934

r? cuviper
@apiraino
Copy link
Contributor

I'll remove the nominations since this was the sensible thing to do (T-lang left a comment here)

@rustbot label -I-compiler-nominated -I-lang-nominated

@rustbot rustbot removed I-compiler-nominated Nominated for discussion during a compiler team meeting. I-lang-nominated Nominated for discussion during a lang team meeting. labels Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WaffleLapkin WaffleLapkin WaffleLapkin approved these changes

Assignees

@estebank estebank

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. F-extended_varargs_abi_support `#![feature(extended_varargs_abi_support)]` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.85.0
Development

Successfully merging this pull request may close these issues.