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 mips64 targets does not respect repr(transparent) #115404

Open
RalfJung opened this issue Aug 31, 2023 · 5 comments
Open

extern "C" fn on mips64 targets does not respect repr(transparent) #115404

RalfJung opened this issue Aug 31, 2023 · 5 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-MIPS Target: MIPS processors P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

This is the MIPS version of #115336: on the target mips64-unknown-linux-gnuabi64, all arrays and some tuple types like (i32, f32, i64, f64) have a different ABI when being wrapped in repr(transparent) wrappers, and hence this fails the test I added in #115372.

For instance for [u8; 2], the PassMode becomes

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

But for Wrapper<[u8; 2]> it becomes

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

This also affects mips64el-unknown-linux-gnuabi64, mips64-openwrt-linux-musl, mips64-unknown-linux-muslabi64, mips64el-unknown-linux-muslabi64
Cc @Itus-Shield (listed as maintainer for the openwrt target)

@RalfJung RalfJung added O-MIPS Target: MIPS processors I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness labels Aug 31, 2023
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 31, 2023
@Noratrieb Noratrieb added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. 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 31, 2023
@RalfJung
Copy link
Member Author

As per #115238, all affected targets are tier 3 targets.

@apiraino
Copy link
Contributor

apiraino commented Sep 4, 2023

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-low

@rustbot rustbot added P-low Low priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Sep 4, 2023
@RalfJung
Copy link
Member Author

RalfJung commented Sep 6, 2023

Here's another example of a repr(transparent) violation on mips64, orthogonal to the first:

struct T(f64, f64, f64);
struct Wrap<T>(T);

#[rustc_abi(assert_eq)]
type Test = (extern "C" fn(T), extern "C" fn(Wrap<T>));

T gets passed in 3 float registers, but Wrap<T> is passed in 3 integer registers. The problem is that this search here doesn't recurse into fields of nested structs.

(I don't know what the C ABI says for this target, i.e. whether it is truly only double fields that appear directly in the struct type that should be passed in registers, and any doubles in nested fields-of-fields really should be ignored. So there might also be an ABI incompatibility with C here.)

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.
@RalfJung
Copy link
Member Author

RalfJung commented Sep 8, 2023

I'm also seeing this target use PassMode::Cast for unsized types... that makes no sense, right?

@bjorn3
Copy link
Member

bjorn3 commented Sep 8, 2023

Yeah, unsized args should always be passed as PassMode::Indirect with on_stack=false.

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.
@workingjubilee workingjubilee added the A-FFI Area: Foreign function interface (FFI) label Nov 1, 2024
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-MIPS Target: MIPS processors P-low Low 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

6 participants