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

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

Closed
RalfJung opened this issue Sep 3, 2023 · 11 comments · Fixed by #115987
Closed

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

RalfJung opened this issue Sep 3, 2023 · 11 comments · Fixed by #115987
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-loongarch Target: LoongArch (LA32R, LA32S, LA64) 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 3, 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,
                   ),

RISC-V had the exact same issue, so the fix will likely also be the same.

I couldn't find an O-loongarch label.

Cc'ing the people listed on https://doc.rust-lang.org/rustc/platform-support/loongarch-linux.html
@hev @xiangzhai @zhaixiaojuan @xen0n

@RalfJung RalfJung added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness A-abi Area: Concerning the application binary interface (ABI). labels Sep 3, 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 3, 2023
@RalfJung
Copy link
Member Author

RalfJung commented Sep 3, 2023

The loongarch ABI seems to be pretty much exactly like the RISC-V ABI, so I wonder if it would make sense to share some of that code so that bugs like this only have to be fixed once.

@xen0n
Copy link
Contributor

xen0n commented Sep 3, 2023

Thanks for the report! The LoongArch ABI is indeed closely modeled after that of RISC-V, so yeah the fix should look similar. @hev should be able to follow up tomorrow.

@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
@hev
Copy link

hev commented Sep 5, 2023

@xen0n I don't think you meant to tag me. Don't know much about rust.

@xen0n
Copy link
Contributor

xen0n commented Sep 5, 2023

@xen0n I don't think you meant to tag me. Don't know much about rust.

Doh, the correct Rust/LoongArch person is @heiher who happens to have another handle of "hev". Sorry for the noise!

@heiher
Copy link
Contributor

heiher commented Sep 6, 2023

Thanks for your pointing out.

I don't think the types (i32, f32) and MaybeUninit<(i32, f32) would have the same ABI on LoongArch64. The key point of the problem is that MaybeUninit is a union, which is different from a struct:

#[repr(transparent)]
pub union MaybeUninit<T> {
    /* private fields */
}

For types (i32, f32) and #[repr(transparent)] struct X((i32, f32)), they are same ABI:

#![feature(rustc_attrs)]

type T = (i32, f32);

#[repr(transparent)]
struct X(T);

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

#[rustc_abi(debug)]
extern "C" fn test2(_x: X) {}

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: [
                               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,
                   ),

And it seems that the current ABI of MaybeUninit<(i32, f32) is the same as that of C language on LoongArch64:

struct T {
    int i;
    float f;
};

union X {
    struct T v;
};

void test1(struct T x) {}
void test2(union X y) {}

int main (int argc, char *argv[])
{
    struct T t;
    union X x;

    t.i = 6339;
    t.f = 6338.0;

    x.v.i = 6339;
    x.v.f = 6338.0;

    test1(t);
    test2(x);

    return 0;
}

The first is:

(gdb) i r r4
r4             0x18c3              6339
(gdb) i r f0
f0             {f = 0x45c61000, d = 0x3b031b0145c61000} {f = 6338, d = 1.9754611757025394e-24}

The second is:

(gdb) i r r4
r4             0x45c61000000018c3  5027723626191788227 // {float = 0x45c61000, int = 0x000018c3 }
(gdb) i r f0
f0             {f = 0x45c61000, d = 0x3b031b0145c61000} {f = 6338, d = 1.9754611757025394e-24}

@xen0n
Copy link
Contributor

xen0n commented Sep 6, 2023

@heiher: I haven't studied the LoongArch psABI as closely, but from a Rust semantics perspective: MaybeUninit<T> is "a T-sized area that may or may not hold an initialized T", so T and MaybeUninit<T> really should behave the same when that's the case -- implying the two must have the same ABI. So this is something to look into and fix after all...

@RalfJung
Copy link
Member Author

RalfJung commented Sep 6, 2023

The key point of the problem is that MaybeUninit is a union, which is different from a struct:

It's a Rust union, but it doesn't have repr(C), so whatever applies to C unions is completely irrelevant. For instance, many ABIs (e.g. x86) treat structs different from integers, and still NonZeroI32 has the same ABI as i32 -- that's because NonZeroI32 is not a repr(C) struct, so its ABI doesn't have to match that of a C struct.

We promise that MaybeUninit<T> has the same ABI as T for all T on all targets, so this is really not optional.

Your C example, when translated to Rust, needs repr(C) on all types. The ABI of those types obviously needs to match the C types. But that's completely compatible with also treating repr(transparent) types differently.

@heiher
Copy link
Contributor

heiher commented Sep 6, 2023

Thank you for your explanation. Hmm, I'm not familiar with Rust ABI, and it seems to have less relevance to psABI. OK, I will follow the RISC-V patches to fix LoongArch.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 6, 2023

It's the same problem we have on all targets: there is a target ABI that is defined in terms of C types, and now every other language has to figure out how to map their types to C types to then compute the ABI from that. Unfortunately the Rust compiler has no central infrastructure for that (there's no function to "give me the C type that describes the ABI of a given Rust type"), and so each target has to implement that mapping itself, and they are all making similar mistakes...

@heiher
Copy link
Contributor

heiher commented Sep 6, 2023

@rustbot label +O-loongarch

@rustbot rustbot added the O-loongarch Target: LoongArch (LA32R, LA32S, LA64) label Sep 6, 2023
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.
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 20, 2023
…bi, r=bjorn3

rustc_target/loongarch: 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#115509

r? `@bjorn3`
@bors bors closed this as completed in 0163768 Sep 20, 2023
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Sep 21, 2023
rustc_target/loongarch: 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/rust#115509

r? `@bjorn3`
yetist pushed a commit to yetist/rust that referenced this issue Jun 5, 2024
…ne non-ZST member

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

Fixes rust-lang#115509
yetist pushed a commit to yetist/rust that referenced this issue Jul 30, 2024
…ne non-ZST member

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

Fixes rust-lang#115509
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-loongarch Target: LoongArch (LA32R, LA32S, LA64) 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.

7 participants