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

extern "C" fn on sparc64 targets does not respect repr(transparent) #115336

Open
RalfJung opened this issue Aug 29, 2023 · 11 comments
Open

extern "C" fn on sparc64 targets does not respect repr(transparent) #115336

RalfJung opened this issue Aug 29, 2023 · 11 comments
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-FFI Area: Foreign function interface (FFI) I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-SPARC Target: SPARC processors P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

I tried the following code:

#![feature(rustc_attrs)]

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

#[repr(transparent)]
struct Wrap<T>(T);

#[rustc_abi(debug)]
extern "C" fn test2(_x: Wrap<[u8; 0]>) {}

When running this with --target sparc64-unknown-linux-gnu (using Miri), the first function reports an argument pass mode of

                   mode: Indirect {
                       attrs: ArgAttributes {
                           regular: NoAlias | NoCapture | NonNull | NoUndef,
                           arg_ext: None,
                           pointee_size: Size(0 bytes),
                           pointee_align: Some(
                               Align(1 bytes),
                           ),
                       },
                       extra_attrs: None,
                       on_stack: false,
                   },

but the 2nd function reports

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

I'm not an expert in interpreting PassMode, but Indirect and Cast are not compatible, are they?

Cc @bjorn3 @eddyb @psumbera

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 29, 2023
@RalfJung
Copy link
Member Author

RalfJung commented Aug 29, 2023

While this particular case might be fixed by having the ABI adjustment logic explicitly unwrap all repr(transparent) wrappers, that will not work in more complicated cases where it is unclear which argument is being wrapped:

struct Zst;

#[repr(transparent)]
struct Wrap(Zst, [u8; 0]);

A Wrap must be ABI-compatible with both Zst and [u8; 0] and that seems not possible?

Maybe we have to say that empty arrays do not count as 1-ZST...

@Noratrieb Noratrieb added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. O-SPARC Target: SPARC processors A-ABI Area: Concerning the application binary interface (ABI) and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 29, 2023
@RalfJung
Copy link
Member Author

Arguably this is a soundness issue, since unsafe code could rely on repr(transparent) behaving as advertised.

@RalfJung RalfJung added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Aug 29, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Aug 29, 2023
@RalfJung
Copy link
Member Author

This affects all arrays, not just empty arrays: [u8; 8] has pass mode

                   mode: Indirect {
                       attrs: ArgAttributes {
                           regular: NoAlias | NoCapture | NonNull | NoUndef,
                           arg_ext: None,
                           pointee_size: Size(8 bytes),
                           pointee_align: Some(
                               Align(1 bytes),
                           ),
                       },
                       extra_attrs: None,
                       on_stack: false,
                   },

but Wrap<[u8; 8]> has

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

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Aug 30, 2023
@thejpster
Copy link
Contributor

Does this also affect 32-bit SPARCV7 in sparc-unknown-none-elf?

@RalfJung
Copy link
Member Author

RalfJung commented Aug 30, 2023

No, this is specific to sparc64. (Well there might be other ABIs, I didn't check all of them. This PR should tell us once it reaches CI. But this here for sparc non-64 targets is completely harmless.)

@RalfJung
Copy link
Member Author

RalfJung commented Aug 30, 2023

The only way I see to fix this is to say that the actual presence of repr(transparent) affects ABI computation, and before checking the field shape a function like unfold_transparent defined in this PR needs to be applied. After all, repr(transparent) structs do not have to be compatible with their C counterparts, only repr(C) structs do.

... but that won't be enough for the ZST issue, since we still have the problem that

#[repr(transparent)]
struct Wrap((), [u8; 0]);

must be ABI-compatible with both of its fields.

@RalfJung RalfJung changed the title extern "C" fn on sparc64-unknown-linux-gnu does not respect repr(transparent) extern "C" fn on sparc64-unknown-linux-gnu and mips64-unknown-linux-gnuabi64 does not respect repr(transparent) Aug 31, 2023
@RalfJung RalfJung changed the title extern "C" fn on sparc64-unknown-linux-gnu and mips64-unknown-linux-gnuabi64 does not respect repr(transparent) extern "C" fn on sparc64-unknown-linux-gnu does not respect repr(transparent) Aug 31, 2023
@RalfJung RalfJung changed the title extern "C" fn on sparc64-unknown-linux-gnu does not respect repr(transparent) extern "C" fn on sparc64 targets does not respect repr(transparent) Aug 31, 2023
@RalfJung
Copy link
Member Author

RalfJung commented Aug 31, 2023

MIPS64 is also affected: #115404.

And the non-zero-sized-array case also affects the sparc64-unknown-netbsd, sparc64-unknown-openbsd targets. I reached out to their maintainers via email.

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.
@psumbera
Copy link
Contributor

psumbera commented Dec 7, 2023

@RalfJung sorry for stupid question. What exactly does mean "using Miri"? I would like to reproduce it on Linux...

@bjorn3
Copy link
Member

bjorn3 commented Dec 7, 2023

Running cargo miri run --target sparc64-unknown-linux-gnu after installing the miri component (rustup component add miri, needs nightly rustc)

@RalfJung
Copy link
Member Author

RalfJung commented Dec 7, 2023

You don't need to do this any more though, the test in tests/ui/abi/compatibility.rs now works on other targets even without Miri. You can see that some tests are disabled on sparc, e.g.

#[cfg(not(any(target_arch = "sparc64")))]
test_abi_compatible!(zst_array, Zst, [u8; 0]);

After enabling them, ./x.py test ui --test-args abi/compatibility will fail. (You can run this command on any host, no sparc CPU required.)

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) A-FFI Area: Foreign function interface (FFI) I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-SPARC Target: SPARC processors P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants