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

riscv64gc-unknown-linux-gnu extern "C" ABI violates repr(transparent) on unions #115481

Closed
RalfJung opened this issue Sep 2, 2023 · 14 comments · Fixed by #115499
Closed

riscv64gc-unknown-linux-gnu extern "C" ABI violates repr(transparent) on unions #115481

RalfJung opened this issue Sep 2, 2023 · 14 comments · Fixed by #115499
Labels
A-ABI Area: Concerning the application binary interface (ABI) I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-riscv Target: RISC-V architecture P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Sep 2, 2023

The types (i32, f32) and MaybeUninit<T> do not have the same ABI, as demonstrated by this testcase:

#![feature(rustc_attrs)]

type T = (i32, f32);

#[rustc_abi(debug)]
extern "C" fn test1(_x: T) {}

#[rustc_abi(debug)]
extern "C" fn test2(_x: std::mem::MaybeUninit<T>) {}

fn main() {}

The first is

                   mode: Cast(
                       CastTarget {
                           prefix: [
                               Some(
                                   Reg {
                                       kind: Integer,
                                       size: Size(4 bytes),
                                   },
                               ),
                               None,
                               None,
                               None,
                               None,
                               None,
                               None,
                               None,
                           ],
                           rest: Uniform {
                               unit: Reg {
                                   kind: Float,
                                   size: Size(4 bytes),
                               },
                               total: Size(4 bytes),
                           },
                           attrs: ArgAttributes {
                               regular: (empty),
                               arg_ext: None,
                               pointee_size: Size(0 bytes),
                               pointee_align: None,
                           },
                       },
                       false,
                   ),

The second is

                   mode: Cast(
                       CastTarget {
                           prefix: [
                               None,
                               None,
                               None,
                               None,
                               None,
                               None,
                               None,
                               None,
                           ],
                           rest: Uniform {
                               unit: Reg {
                                   kind: Integer,
                                   size: Size(8 bytes),
                               },
                               total: Size(8 bytes),
                           },
                           attrs: ArgAttributes {
                               regular: (empty),
                               arg_ext: None,
                               pointee_size: Size(0 bytes),
                               pointee_align: None,
                           },
                       },
                       false,
                   ),

Not sure whom to ping for RISC-V issues.

@RalfJung RalfJung added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-riscv Target: RISC-V architecture A-ABI Area: Concerning the application binary interface (ABI) labels Sep 2, 2023
@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Sep 2, 2023
@RalfJung
Copy link
Member Author

RalfJung commented Sep 2, 2023

I think the fix will have to be to treat repr(transparent) unions specially somewhere around here

FieldsShape::Union(_) => {
if !arg_layout.is_zst() {
return Err(CannotUseFpConv);
}
}

@RalfJung
Copy link
Member Author

RalfJung commented Sep 3, 2023

Cc @msizanoen1

@msizanoen1
Copy link
Contributor

msizanoen1 commented Sep 3, 2023

@RalfJung I'm afraid that the semantics of #[repr(transparent)] on unions are not clearly defined enough to figure out exactly how they should be specially treated (which variant should be used? etc.). Can you ping other people who have more knowledge on this? (If repr(transparent) on unions turns out to be incompatible with ABIs that have struct content destructuring semantics that depends on the types of the field in the struct (e.g. the RISC-V C psABI) then we definitely have a bigger problem.)

@RalfJung
Copy link
Member Author

RalfJung commented Sep 3, 2023

I have no idea who would know anything about RISC-V calling conventions. 🤷 and no other ABI is failing this test.

Basically, when the ABI description says "unions do X", then this should only apply to repr(C) and repr(Rust) unions. A repr(transparent) union must be treated exactly like its non-ZST field.

msizanoen1 added a commit to msizanoen1/rust that referenced this issue Sep 3, 2023
…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.
@Noratrieb Noratrieb added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 4, 2023
@apiraino
Copy link
Contributor

apiraino commented Sep 4, 2023

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

@rustbot rustbot added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Sep 4, 2023
@tgross35
Copy link
Contributor

tgross35 commented Sep 6, 2023

I haven't seen the Rust representation before - is the example saying that T is splitting the values into float and integer registers and passing them, whereas MaybeUninit<T> is treating the whole thing as an bigger opaque integer?

If so then based on section 18.2 here, I think the first one would be correct, assuming Rust doesn't ever make its tuples opaque https://riscv.org/wp-content/uploads/2015/01/riscv-calling.pdf

@programmerjake helped me with something RISC-V at some point so may know where to look. Or deferring to clang https://clang.godbolt.org/z/rrv5x3eYx

@RalfJung
Copy link
Member Author

RalfJung commented Sep 6, 2023

I honestly don't know what that PassMode::Cast means, it is sadly mostly undocumented. But I know it must be the same for the types to be ABI-compatible.

#115499 has been opened to fix this issue.

bors added a commit to rust-lang-ci/rust that referenced this issue Sep 7, 2023
add rustc_abi(assert_eq) to test some guaranteed or at least highly expected ABI compatibility guarantees

This new repr(transparent) test is super useful, it would have found rust-lang#115336 and found rust-lang#115404, rust-lang#115481, rust-lang#115509.
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 8, 2023
add rustc_abi(assert_eq) to test some guaranteed or at least highly expected ABI compatibility guarantees

This new repr(transparent) test is super useful, it would have found rust-lang#115336 and found rust-lang#115404, rust-lang#115481, rust-lang#115509.
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 8, 2023
add rustc_abi(assert_eq) to test some guaranteed or at least highly expected ABI compatibility guarantees

This new repr(transparent) test is super useful, it would have found rust-lang#115336 and found rust-lang#115404, rust-lang#115481, rust-lang#115509.
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Sep 12, 2023
add rustc_abi(assert_eq) to test some guaranteed or at least highly expected ABI compatibility guarantees

This new repr(transparent) test is super useful, it would have found rust-lang/rust#115336 and found rust-lang/rust#115404, rust-lang/rust#115481, rust-lang/rust#115509.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Sep 19, 2023
…nion-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`
@bors bors closed this as completed in 751ecde Sep 19, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Sep 19, 2023
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`
@tgross35
Copy link
Contributor

@RalfJung now that this is fixed, is there a CI test to catch regressions?

@RalfJung
Copy link
Member Author

RalfJung commented Sep 19, 2023

There is a test in tests/ui/abi/compatibility.rs but we don't run tests for riscv, since it's a tier 2 target.

@RalfJung
Copy link
Member Author

Also see #115609 for another ABI issue that affects riscv (and more).

@tgross35
Copy link
Contributor

tgross35 commented Sep 19, 2023

Are tier 2 tests run once before release or anything like that? It seems like at least a subset of tests (this one) are important enough that we should raise a red flag if we brake them somehow on any targets. But that doesn't fit well into the tiering structure...

@RalfJung
Copy link
Member Author

RalfJung commented Sep 20, 2023

I'm not aware of any process like that. Part of the point of being tier 2 is not to burden all contributors with keeping the target working, is it? If we do want to accept that burden, we could consider running something like ./x.py test codegen && ./x.py test ui --test-args abi for tier 2 targets.

But having this discussion in a closed issue makes little sense. Do you want to open a new issue or Zulip thread for discussing tier 2 target test coverage?

@RalfJung
Copy link
Member Author

Inspired by #116004 I found a way to have a test for this: #116030.

@tgross35
Copy link
Contributor

Awesome! Thanks for the update here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ABI Area: Concerning the application binary interface (ABI) I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-riscv Target: RISC-V architecture P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants