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

aarch64-apple-ios: Returning structs by value in extern "C" functions uses wrong calling convention #24154

Closed
jgallagher opened this issue Apr 7, 2015 · 9 comments · Fixed by #24181
Labels
O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state

Comments

@jgallagher
Copy link

Creating an extern C function like this:

#[repr(C)]
pub struct Point {
    x: f32,
    y: f32,
}

#[no_mangle]
pub extern "C" fn get_point() -> Point {
    return Point{x: 10.0, y: 20.0}
}

and compiling using cargo build --target aarch64-apple-ios --release, the assembly produced for get_point is:

_get_point:
0000000000000000                movz    x0, #16800, lsl #48
0000000000000004                movk    x0, #16672, lsl #16
0000000000000008                ret

My ARM assembly is a little rusty, but I believe this is expecting the caller to pass in the address of a Point in x0, and then the function fills it in.

However, this C code, which should be equivalent:

struct point {
    float x;
    float y;
};

struct point get_point(void) {
    return (struct point){10.0, 20.0};
}

compiles to the following assembly in Xcode 6.2 targeting arm64:

0x1000fec74:  fmov   s0, #1.000000e+01
0x1000fec78:  fmov   s1, #2.000000e+01
0x1000fec7c:  ret

which is returning the two struct fields in the SIMD registers.

Is this an issue of how aarch64 is defined or configured, maybe?

@jgallagher
Copy link
Author

Reading the calling conventions for AArch64, I believe the Xcode version is correct and Rust is incorrect. According to section 5.4 rule C.2, since Point is an HFA (Homogenous Floating-point Aggregate, section 4.3.5.1), its fields should be passed in SIMD registers. According to the first bullet in section 5.5, a returned Point should follow the same convention.

@jgallagher
Copy link
Author

Sorry, one last note. It looks like writing Rust code to take a struct by value has the same problem. This rust function:

#[no_mangle]
pub extern "C" fn sum_point(p: Point) -> f32 {
    p.x + p.y
}

compiles to this assembly:

000000000000000c                fmov    s0, w0
0000000000000010                lsr     x8, x0, #32
0000000000000014                fmov    s1, w8
0000000000000018                fadd    s0, s0, s1
000000000000001c                ret

but the equivalent C function compiled in Xcode produces:

0x10001ac4c:  fadd   s0, s0, s1
0x10001ac50:  ret

@steveklabnik steveklabnik added the O-ios Operating system: iOS label Apr 7, 2015
jgallagher pushed a commit to jgallagher/rust that referenced this issue Apr 8, 2015
jgallagher pushed a commit to jgallagher/rust that referenced this issue Apr 8, 2015
@vhbit
Copy link
Contributor

vhbit commented Apr 12, 2015

@steveklabnik I'd say 'A-iOS' is incorrect label as this is related to Aarch64 in general and can happen on Android too.

@pnkfelix
Copy link
Member

@vhbit (to be fair i suspect @steveklabnik was working from the description in the title)

@pnkfelix pnkfelix added O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state and removed O-ios Operating system: iOS labels Apr 12, 2015
@vhbit
Copy link
Contributor

vhbit commented Apr 12, 2015

@steveklabnik, may I ask you also to cc me on issues which you mark with iOS label - till today I didn't know it exists at all as I don't track issues.

@vhbit
Copy link
Contributor

vhbit commented Apr 12, 2015

@pnkfelix I didn't mean to offense anyone, I apologize if that sounded like that - my intention was just to clarify issue

@steveklabnik
Copy link
Member

Yeah, I just try to ensure each issue has at least one label, but I'm not always perfect with them.

@vhbit I can try to remember :)

@pnkfelix
Copy link
Member

@steveklabnik do you know if there is a way to utilize the github's API so that someone could register to receive notices of such labels?

@steveklabnik
Copy link
Member

Not off the top of my head. https://developer.github.com/v3/issues/labels/ could probably make it work, I'd bet.

bors added a commit that referenced this issue Apr 16, 2015
I doubt this PR is ready to merge as-is, for a couple reasons:

* There are no tests for this change. I'm not sure how to add tests for this change, as it modifies the C ABI for a cross-compilation target. Anecdotally, I have an iOS library I've been working on, and before this change, it crashes running on an arm64 device due to bad calling conventions (a simplified example is in #24154), and after this change, it runs correctly.
* This is my first foray into LLVM. I did my best to reimplement what Clang does for AArch64 codegen (https://github.com/llvm-mirror/clang/blob/master/lib/CodeGen/TargetInfo.cpp), particularly in `ABIInfo::isHomogeneousAggregate`, `AArch64ABIInfo::isHomogeneousAggregateBaseType`, and `AArch64ABIInfo::isHomogeneousAggregateSmallEnough`, but I'm not confident I got a complete translation, particularly because Clang includes a lot of checks that I don't believe are necessary for rustc.

Fixes #24154.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants