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

rustc_target/riscv: Fix passing of transparent unions with only one non-ZST member #115499

Merged
merged 2 commits into from
Sep 19, 2023
Merged

rustc_target/riscv: Fix passing of transparent unions with only one non-ZST member #115499

merged 2 commits into from
Sep 19, 2023

Conversation

msizanoen1
Copy link
Contributor

This ensures that MaybeUninit<T> has the same ABI as T when passed through an extern "C" function.

Fixes #115481.

r? @RalfJung

@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 3, 2023
@msizanoen1
Copy link
Contributor Author

@rustbot label +A-abi +O-riscv

@rustbot rustbot added A-ABI Area: Concerning the application binary interface (ABI) O-riscv Target: RISC-V architecture labels Sep 3, 2023
RalfJung
RalfJung reviewed Sep 3, 2023
View reviewed changes
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

I can confirm that this fixes the testcase in #115372.

I just got some nits, but I would prefer if someone more knowledgeable on ABIs would take a look... Cc @bjorn3 @Lokathor

Also do we have to worry about this being an ABI-breaking change?

compiler/rustc_target/src/abi/call/riscv.rs Outdated Show resolved Hide resolved
compiler/rustc_target/src/abi/call/riscv.rs Outdated Show resolved Hide resolved
@RalfJung RalfJung mentioned this pull request Sep 3, 2023
Closed
@msizanoen1 msizanoen1 force-pushed the riscv-fix-transparent-union-abi branch from 89a24ca to efd4e62 Compare September 3, 2023 13:54
@RalfJung
Copy link
Member

RalfJung commented Sep 4, 2023

LGTM but let's get another pair of eyes on it and hopefully someone who knows our process for breaking ABI. This affects extern "C" fn after all, albeit only when non-repr(C) types are involved.

r? compiler

@rustbot rustbot assigned compiler-errors and unassigned RalfJung Sep 4, 2023
@RalfJung RalfJung mentioned this pull request Sep 6, 2023
Closed
@compiler-errors
Copy link
Member

Gonna pass to someone who knows more about ABIs. Sorry for not getting to this sooner.

r? @bjorn3

@rustbot rustbot assigned bjorn3 and unassigned compiler-errors Sep 18, 2023
@msizanoen1
rustc_target/riscv: Fix passing of transparent unions with only one n…
751ecde 
…on-ZST member

This ensures that `MaybeUninit<T>` has the same ABI as `T` when passed
through an `extern "C"` function.

Fixes rust-lang#115481.
@msizanoen1 msizanoen1 force-pushed the riscv-fix-transparent-union-abi branch from efd4e62 to 751ecde Compare September 19, 2023 10:13
@bjorn3
Copy link
Member

bjorn3 commented Sep 19, 2023

Could you add a codegen test for this to avoid regressing this in the future?

@msizanoen1
Copy link
Contributor Author

Could you add a codegen test for this to avoid regressing this in the future?

AFAIK #115372 should cover this for RISC-V and other architectures. (This is actually how the bug was found in the first place.)

@bjorn3
Copy link
Member

bjorn3 commented Sep 19, 2023

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Sep 19, 2023

📌 Commit 4d4c13b has been approved by bjorn3

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 Sep 19, 2023
@GuillaumeGomez GuillaumeGomez mentioned this pull request Sep 19, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 19, 2023
@bors
Auto merge of rust-lang#115979 - GuillaumeGomez:rollup-06ujzgh, r=Gui…
ac5ac47 
…llaumeGomez

Rollup of 6 pull requests

Successful merges:

 - rust-lang#113383 (style-guide: Add section on bugs, and resolving bugs)
 - rust-lang#115499 (rustc_target/riscv: Fix passing of transparent unions with only one non-ZST member)
 - rust-lang#115801 (Detect cycle errors hidden by opaques during monomorphization)
 - rust-lang#115947 (Custom code classes in docs warning)
 - rust-lang#115957 (fix mismatched symbols)
 - rust-lang#115958 (explain mysterious addition in float minimum/maximum)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit edd7be5 into rust-lang:master Sep 19, 2023
11 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 19, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 19, 2023
@rust-timer
Unrolled build for rust-lang#115499
8e1d82e 
Rollup merge of rust-lang#115499 - msizanoen1:riscv-fix-transparent-union-abi, r=bjorn3

rustc_target/riscv: Fix passing of transparent unions with only one non-ZST member

This ensures that `MaybeUninit<T>` has the same ABI as `T` when passed through an `extern "C"` function.

Fixes rust-lang#115481.

r? `@RalfJung`
@msizanoen1 msizanoen1 deleted the riscv-fix-transparent-union-abi branch September 21, 2023 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 left review comments

@RalfJung RalfJung RalfJung left review comments

Assignees

@bjorn3 bjorn3

Labels
A-ABI Area: Concerning the application binary interface (ABI) O-riscv Target: RISC-V architecture 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.74.0
Development

Successfully merging this pull request may close these issues.

riscv64gc-unknown-linux-gnu extern "C" ABI violates repr(transparent) on unions
6 participants
@msizanoen1 @RalfJung @compiler-errors @bjorn3 @bors @rustbot