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.

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

Stabilize extended_varargs_abi_support #116161

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Soveu
Copy link
Contributor

@Soveu Soveu commented Sep 25, 2023

I think that is everything? If there is any documentation regarding extern and/or varargs to correct, let me know, some quick greps suggest that there might be none.

Tracking issue: #100189

@rustbot
Copy link
Collaborator

rustbot commented Sep 25, 2023

r? @wesleywiser

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

@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. labels Sep 25, 2023
@Soveu
Copy link
Contributor Author

Soveu commented Sep 25, 2023

@rustbot label F-extended_varargs_abi_support

@rustbot rustbot added the F-extended_varargs_abi_support `#![feature(extended_varargs_abi_support)]` label Sep 25, 2023
@rust-log-analyzer

This comment has been minimized.

@@ -160,6 +160,9 @@ declare_features! (
(accepted, explicit_generic_args_with_impl_trait, "1.63.0", Some(83701), None),
/// Allows arbitrary expressions in key-value attributes at parse time.
(accepted, extended_key_value_attributes, "1.54.0", Some(78835), None),
/// Allows using `efiapi`, `aapcs`, `sysv64` and `win64` as calling
/// convention for functions with varargs.
(accepted, extended_varargs_abi_support, "1.65.0", Some(100189), None),
Copy link
Contributor

Choose a reason for hiding this comment

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

use CURRENT_RUSTC_VERSION instead of a version number here

@tgross35
Copy link
Contributor

tgross35 commented Oct 5, 2023

Tracking issue: #100189

@bors
Copy link
Contributor

bors commented Oct 16, 2023

☔ The latest upstream changes (presumably #116550) made this pull request unmergeable. Please resolve the merge conflicts.

@Soveu
Copy link
Contributor Author

Soveu commented Nov 21, 2023

@wesleywiser

@bors
Copy link
Contributor

bors commented Dec 11, 2023

☔ The latest upstream changes (presumably #118823) made this pull request unmergeable. Please resolve the merge conflicts.

@apiraino
Copy link
Contributor

r? compiler

@rustbot rustbot assigned Nadrieril and unassigned wesleywiser Feb 22, 2024
@Nadrieril
Copy link
Member

I'm on vacation for a few days :)
r? compiler

@rustbot rustbot assigned cjgillot and unassigned Nadrieril Feb 22, 2024
@cjgillot cjgillot added I-lang-nominated Nominated for discussion during a lang team meeting. I-compiler-nominated Nominated for discussion during a compiler team meeting. labels Mar 3, 2024
@cjgillot
Copy link
Contributor

cjgillot commented Mar 3, 2024

Not completely sure which team should be nominated for stabilization, so labeling both in doubt.

@pnkfelix
Copy link
Member

pnkfelix commented Mar 7, 2024

Just to add on to @cjgillot 's comment above: @wesleywiser and I could not remember earlier today whether T-lang wants to own FCP'ing changes like this that are restricted to extending the set of calling-conventions (i.e. the conv in extern "conv" fn foo(...)), which is largely a detail about what platforms one is interoperating with, and not about changing the expressiveness of the Rust language as a whole in the abstract.

(My own gut reaction is that T-compiler is a more natural owner for this than T-lang, but I wasn't certain and so it seems best to let the nomination stand and let the two teams duke it out.)

@apiraino
Copy link
Contributor

Removing the T-compiler nomination until after T-lang discussed this. Feel free to re-add T-compiler!

@rustbot label -I-compiler-nominated

@rustbot rustbot removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Mar 14, 2024
@rustbot rustbot removed I-lang-nominated Nominated for discussion during a lang team meeting. I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy. labels Sep 25, 2024
@traviscross traviscross added the I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy. label Sep 25, 2024
@Soveu
Copy link
Contributor Author

Soveu commented Sep 26, 2024

Question: Does this include -unwind variants for each of the ABIs or not?

Well, yes. I haven't thought about it initially (as I was mostly focused on efiapi, non-unwind win64 and sysv64), but currently Abi::supports_varargs looks like this:

    pub fn supports_varargs(self) -> bool {
        // * C and Cdecl obviously support varargs.
        // * C can be based on Aapcs, SysV64 or Win64, so they must support varargs.
        // * EfiApi is based on Win64 or C, so it also supports it.
        // * System falls back to C for functions with varargs.
        //
        // * Stdcall does not, because it would be impossible for the callee to clean
        //   up the arguments. (callee doesn't know how many arguments are there)
        // * Same for Fastcall, Vectorcall and Thiscall.
        // * Other calling conventions are related to hardware or the compiler itself.
        match self {
            Self::C { .. }
            | Self::Cdecl { .. }
            | Self::System { .. }
            | Self::Aapcs { .. }
            | Self::Win64 { .. }
            | Self::SysV64 { .. }
            | Self::EfiApi => true,
            _ => false,
        }
    }

So whether there is Abi::Win64 { unwind: true } or Abi::Win64 { unwind: false }, it should treat them the same.

Should I rebase the change or will there be some exceptions for -unwind calling conventions?

@Soveu
Copy link
Contributor Author

Soveu commented Sep 26, 2024

Removing the T-compiler nomination until after T-lang discussed this. Feel free to re-add T-compiler!

@rustbot label -I-compiler-nominated

@rustbot label +I-compiler-nominated

@rustbot rustbot added the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Sep 26, 2024
@apiraino
Copy link
Contributor

apiraino commented Oct 3, 2024

T-compiler looked again at this proposal during the triage meeting and is fine with it.

@rustbot label -I-compiler-nominated

@rustbot rustbot removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Oct 3, 2024
@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 11, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 11, 2024

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust.git master
$ git push --force-with-lease

The following commits are merge commits:

@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 11, 2024
@Soveu
Copy link
Contributor Author

Soveu commented Oct 11, 2024

lgtm :)
@rustbot label +S-waiting-on-review

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 11, 2024
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Oct 12, 2024

☔ The latest upstream changes (presumably #131448) made this pull request unmergeable. Please resolve the merge conflicts.

@Soveu Soveu force-pushed the varargs2 branch 2 times, most recently from 4951cec to 050c888 Compare October 13, 2024 20:08
@Soveu
Copy link
Contributor Author

Soveu commented Oct 13, 2024

uhh, I did not notice that I changed some submodules by accident

@bors
Copy link
Contributor

bors commented Oct 23, 2024

☔ The latest upstream changes (presumably #132027) made this pull request unmergeable. Please resolve the merge conflicts.

@cjgillot
Copy link
Contributor

r=me after rebase

@cjgillot cjgillot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 27, 2024
@apiraino apiraino removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-extended_varargs_abi_support `#![feature(extended_varargs_abi_support)]` I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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
Development

Successfully merging this pull request may close these issues.