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

fix extern "aapcs" fn #37814

Merged
merged 1 commit into from
Nov 19, 2016
Merged

fix extern "aapcs" fn #37814

merged 1 commit into from
Nov 19, 2016

Conversation

japaric
Copy link
Member

@japaric japaric commented Nov 16, 2016

to actually use the AAPCS calling convention

closes #37810

This is technically a [breaking-change] because it changes the ABI of
extern "aapcs" functions that (a) involve f32/f64 arguments/return
values and (b) are compiled for arm-eabihf targets from
"aapcs-vfp" (wrong) to "aapcs" (correct).

Appendix:

What these ABIs mean?

  • In the "aapcs-vfp" ABI or "hard float" calling convention: Floating
    point values are passed/returned through FPU registers (s0, s1, d0, etc.)

  • Whereas, in the "aapcs" ABI or "soft float" calling convention:
    Floating point values are passed/returned through general purpose
    registers (r0, r1, etc.)

Mixing these ABIs can cause problems if the caller assumes that the
routine is using one of these ABIs but it's actually using the other
one.


r? @alexcrichton We are going this extern "aapcs" fn thing to implement some intrinsics (floatundidf) for the eabihf targets in order to comply with LLVM's calling convention of intrinsics.

Oh, and the value of the enum came from here.

cc @TimNN @parched

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@@ -274,10 +274,10 @@ impl FnType {
C => llvm::CCallConv,
Win64 => llvm::X86_64_Win64,
SysV64 => llvm::X86_64_SysV,
Aapcs => llvm::AapcsCallConv,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't match ffi.rs it's missing Arm

@japaric
Copy link
Member Author

japaric commented Nov 16, 2016

It works:

#[no_mangle]
pub extern "aapcs" fn foo(x: f64) -> f64 {
    x
}

#[no_mangle]
pub extern "C" fn bar(x: f64) -> f64 {
    x
}
00000000 <foo>:
   0:   e24dd008        sub     sp, sp, #8
   4:   ec410b10        vmov    d0, r0, r1
   8:   ed8d0b00        vstr    d0, [sp]
   c:   eaffffff        b       10 <foo+0x10>
  10:   ed9d0b00        vldr    d0, [sp]
  14:   ec510b10        vmov    r0, r1, d0
  18:   e28dd008        add     sp, sp, #8
  1c:   e12fff1e        bx      lr

Disassembly of section .text.bar:

00000000 <bar>:
   0:   e24dd008        sub     sp, sp, #8
   4:   ed8d0b00        vstr    d0, [sp]
   8:   eaffffff        b       c <bar+0xc>
   c:   ed9d0b00        vldr    d0, [sp]
  10:   e28dd008        add     sp, sp, #8
  14:   e12fff1e        bx      lr

@japaric
Copy link
Member Author

japaric commented Nov 16, 2016

(Compiled for arm-unknown-linux-gnueabihf)

@TimNN
Copy link
Contributor

TimNN commented Nov 16, 2016

Maybe we should add a note about this (aapcs is now always soft float) to the aapcs entry in reference.md?

I'm not sure about the exact policy here, however technically this is a breaking change so should probably tagged as such ([breaking-change]).

@alexcrichton
Copy link
Member

Strictly speaking yeah it's a breaking change but in practice I imagine this will fix more code than it breaks (if it breaks anything...).

@japaric want to edit the commit message and I'll r+?

also cc @rust-lang/compiler, just to be aware

to actually use the AAPCS calling convention

closes rust-lang#37810

This is technically a [breaking-change] because it changes the ABI of
`extern "aapcs"` functions that (a) involve `f32`/`f64` arguments/return
values and (b) are compiled for arm-eabihf targets from
"aapcs-vfp" (wrong) to "aapcs" (correct).

Appendix:

What these ABIs mean?

- In the "aapcs-vfp" ABI or "hard float" calling convention: Floating
point values are passed/returned through FPU registers (s0, s1, d0, etc.)

- Whereas, in the "aapcs" ABI or "soft float" calling convention:
Floating point values are passed/returned through general purpose
registers (r0, r1, etc.)

Mixing these ABIs can cause problems if the caller assumes that the
routine is using one of these ABIs but it's actually using the other
one.
@japaric
Copy link
Member Author

japaric commented Nov 16, 2016

want to edit the commit message and I'll r+?

Done.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Nov 17, 2016

📌 Commit 456ceba has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Nov 19, 2016

⌛ Testing commit 456ceba with merge fb025b4...

bors added a commit that referenced this pull request Nov 19, 2016
fix `extern "aapcs" fn`

to actually use the AAPCS calling convention

closes #37810

This is technically a [breaking-change] because it changes the ABI of
`extern "aapcs"` functions that (a) involve `f32`/`f64` arguments/return
values and (b) are compiled for arm-eabihf targets from
"aapcs-vfp" (wrong) to "aapcs" (correct).

Appendix:

What these ABIs mean?

- In the "aapcs-vfp" ABI or "hard float" calling convention: Floating
point values are passed/returned through FPU registers (s0, s1, d0, etc.)

- Whereas, in the "aapcs" ABI or "soft float" calling convention:
Floating point values are passed/returned through general purpose
registers (r0, r1, etc.)

Mixing these ABIs can cause problems if the caller assumes that the
routine is using one of these ABIs but it's actually using the other
one.

---

r? @alexcrichton We are going this `extern "aapcs" fn` thing to implement some intrinsics (floatundidf) for the eabihf targets in order to comply with LLVM's calling convention of intrinsics.

Oh, and the value of the enum came from [here](http://llvm.org/docs/doxygen/html/namespacellvm_1_1CallingConv.html).

cc @TimNN @parched
@bors bors merged commit 456ceba into rust-lang:master Nov 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aapcs calling convention isn't passed to LLVM
6 participants