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

Only allow PassMode::Direct for aggregates on wasm when using the C ABI #133931

Merged
merged 1 commit into from
Dec 6, 2024
Merged

Only allow PassMode::Direct for aggregates on wasm when using the C ABI #133931

merged 1 commit into from
Dec 6, 2024

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Dec 5, 2024

For the Rust ABI we don't have any ABI compat reasons to allow PassMode::Direct for aggregates.

@rustbot
Copy link
Collaborator

rustbot commented Dec 5, 2024

r? @nnethercote

rustbot has assigned @nnethercote.
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. labels Dec 5, 2024
@bjorn3 bjorn3 mentioned this pull request Dec 5, 2024
Open
nnethercote
nnethercote reviewed Dec 5, 2024
View reviewed changes
compiler/rustc_ty_utils/src/abi.rs Outdated

match spec_abi {
// The unadjusted ABI is ill specified, but unfortunately we need it for
// calling certain LLVM intrinsics.
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this comment up to be part of the big comment above?

nnethercote
nnethercote reviewed Dec 5, 2024
View reviewed changes
compiler/rustc_ty_utils/src/abi.rs
// calling certain LLVM intrinsics.
ExternAbi::Unadjusted => {}
ExternAbi::PtxKernel => {}
ExternAbi::C { unwind: _ }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the C-abi-only aspect also be mentioned in the comment above?

Copy link
Member

Choose a reason for hiding this comment

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

It could be something like

Suggested change
ExternAbi::C { unwind: _ }
// wasm targets currently implement a dysfunctional C ABI
ExternAbi::C { unwind: _ }

@nnethercote
Copy link
Contributor

I don't know anything about this stuff, but the change is so minor it seems ok. r=me with the slight comment improvements suggested.

@workingjubilee
Copy link
Member

workingjubilee commented Dec 5, 2024 •
edited
Loading

PassMode::Direct is primarily intended for "in-register" arguments, it should primarily be used with Scalar and Vector types, but wasm's C ABI is currently a set of mistakes so PassMode::Direct can happen with aggregates.

There might be an edge case I'm not thinking of right now but it doesn't really make sense for almost any ABI to use PassMode::Direct for aggregates. For Unadjusted the only reason is because we aren't actually performing a real function call.

@bjorn3 bjorn3 force-pushed the even_stricter_fn_abi_sanity_checking branch from 6ed4038 to 117e7de Compare December 6, 2024 08:25
@bjorn3
Only allow PassMode::Direct for aggregates on wasm when using the C ABI
8922d30 
For the Rust ABI we don't have any ABI compat reasons to allow
PassMode::Direct for aggregates.
@bjorn3 bjorn3 force-pushed the even_stricter_fn_abi_sanity_checking branch from 117e7de to 8922d30 Compare December 6, 2024 08:26
@bjorn3
Copy link
Member Author

bjorn3 commented Dec 6, 2024

@bors r=nnethercote

@bors
Copy link
Contributor

bors commented Dec 6, 2024

📌 Commit 8922d30 has been approved by nnethercote

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 Dec 6, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 6, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 6, 2024
@bors
Auto merge of rust-lang#133950 - matthiaskrgr:rollup-b7g5p73, r=matth…
bc145ce 
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#130777 (rust_for_linux: -Zreg-struct-return commandline flag for X86 (rust-lang#116973))
 - rust-lang#133211 (Extend Miri to correctly pass mutable pointers through FFI)
 - rust-lang#133790 (Improve documentation for Vec::extend_from_within)
 - rust-lang#133930 (rustbook: update to use new mdbook-trpl package from The Book)
 - rust-lang#133931 (Only allow PassMode::Direct for aggregates on wasm when using the C ABI)

r? `@ghost`
`@rustbot` modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 6, 2024
@rust-timer
Unrolled build for rust-lang#133931
23860c6 
Rollup merge of rust-lang#133931 - bjorn3:even_stricter_fn_abi_sanity_checking, r=nnethercote

Only allow PassMode::Direct for aggregates on wasm when using the C ABI

For the Rust ABI we don't have any ABI compat reasons to allow PassMode::Direct for aggregates.
@bors bors merged commit 875df6c into rust-lang:master Dec 6, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 6, 2024
@bjorn3 bjorn3 deleted the even_stricter_fn_abi_sanity_checking branch December 6, 2024 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@workingjubilee workingjubilee workingjubilee left review comments

@nnethercote nnethercote nnethercote left review comments

Assignees

@nnethercote nnethercote

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.85.0
Development

Successfully merging this pull request may close these issues.
5 participants
@bjorn3 @rustbot @nnethercote @workingjubilee @bors