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

Varargs support for system ABI #119587

Merged
merged 1 commit into from
Jan 13, 2024
Merged

Varargs support for system ABI #119587

merged 1 commit into from
Jan 13, 2024

Conversation

beepster4096
Copy link
Contributor

This PR allows functions with the system ABI to be variadic (under the extended_varargs_abi_support feature tracked in #100189). On x86 windows, the system ABI is equivalent to C for variadic functions. On other platforms, system is already equivalent to C.

Fixes #110505

@rustbot
Copy link
Collaborator

rustbot commented Jan 4, 2024

r? @petrochenkov

(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 Jan 4, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jan 4, 2024

These commits modify compiler targets.
(See the Target Tier Policy.)

@beepster4096 beepster4096 mentioned this pull request Jan 4, 2024
Closed
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Soveu
Copy link
Contributor

Soveu commented Jan 5, 2024

That makes me think, maybe we should first adjust the abi, then check if it is possible to use varargs?
This has an advantage, that the compiler will be more "flexible", at the cost of code being probably less portable - we could add some warnings for using system with varargs, but that will be it, just a warning.

@bjorn3
Copy link
Member

bjorn3 commented Jan 5, 2024

Does windows have any function that both uses the system abi and is a vararg? How is the abi handled for that function on x86?

@beepster4096
Copy link
Contributor Author

@Soveu

maybe we should first adjust the abi, then check if it is possible to use varargs?

There's some edge cases where this would make the varargs error dependent on the target. For example, a variadic stdcall function on i686-pc-windows-msvc would fail to compile. The same function on x86_64-pc-windows-msvc would pass because stdcall would be adjusted into C. Right now, the check is independent of the target, it just relies on the ABI the function is declared with. I don't think changing that is a good idea.

@bjorn3

Does windows have any function that both uses the system abi and is a vararg?

The example from the issue, DbgPrint, actually appears to be defined with __cdecl explicitly, but I found a function RtlInitializeSidEx that is both variadic and uses __stdcall.

How is the abi handled for that function on x86?

According to https://learn.microsoft.com/en-us/cpp/cpp/stdcall, variadic functions marked with __stdcall just use __cdecl. I am going to add that link in a comment to satisfy this review comment.

@petrochenkov
Copy link
Contributor

r=me after adding cross-link comments to different places that "normalize" calling conventions (#119587 (comment)) + squashing commits.
@rustbot author

@rustbot rustbot 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 Jan 12, 2024
@beepster4096
Copy link
Contributor Author

I'm a little unsure of what exactly I am being asked to do here. It seems like the review (#119587 (comment)) says to add a comment in build_dll_import's abi logic linking to adjust_abi. Meanwhile, the plurals in the r=me prerequisite lead me to believe it means to have both link to each other. To be clear, adjust_abi and build_dll_import are the only places that normalize calling conventions I could find.

I've done only the former change (and squished) for now.
@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 13, 2024
@petrochenkov
Copy link
Contributor

I've done only the former change

Okay, that's good enough.
@bors r+

@bors
Copy link
Contributor

bors commented Jan 13, 2024

📌 Commit 41e224b has been approved by petrochenkov

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 Jan 13, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 13, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 13, 2024
@bors
Auto merge of rust-lang#119927 - matthiaskrgr:rollup-885ws57, r=matth…
c6c4abf 
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#119587 (Varargs support for system ABI)
 - rust-lang#119891 (rename `reported_signature_mismatch` to reflect its use)
 - rust-lang#119894 (Allow `~const` on associated type bounds again)
 - rust-lang#119896 (Taint `_` placeholder types in trait impl method signatures)
 - rust-lang#119898 (Remove unused `ErrorReporting` variant from overflow handling)
 - rust-lang#119902 (fix typo in `fn()` docs)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7b507db into rust-lang:master Jan 13, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 13, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 13, 2024
@rust-timer
Unrolled build for rust-lang#119587
7aabe50 
Rollup merge of rust-lang#119587 - beepster4096:system_varargs, r=petrochenkov

Varargs support for system ABI

This PR allows functions with the `system` ABI to be variadic (under the `extended_varargs_abi_support` feature tracked in rust-lang#100189). On x86 windows, the `system` ABI is equivalent to `C` for variadic functions. On other platforms, `system` is already equivalent to `C`.

Fixes rust-lang#110505
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@petrochenkov petrochenkov

Labels
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.
Projects
None yet
Milestone
1.77.0
Development

Successfully merging this pull request may close these issues.

Variadic functions don't allow "system" on non-x86 Windows
8 participants
@beepster4096 @rustbot @rust-log-analyzer @Soveu @bjorn3 @petrochenkov @bors @RalfJung