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

Properly handle indirect arguments for external C functions #11189

Merged
merged 1 commit into from
Sep 13, 2021

Conversation

ggiraldez
Copy link
Contributor

Fixes #9533

From the description of #9533 and linked issues I gather the C ABI argument passing was ported from Rust, which has the concept of indirect argument passing. Reading the ABI specs for ARM64 (see section 5.4.2, B-3) and for Windows 64 x86, and analyzing the LLVM IR code generated by Clang, it's clear that an indirect argument should be passed as a pointer to caller-allocated memory. This is different than passing an argument on the stack, which seems to be the default in Linux and Darwin on x86: this is handled by LLVM when the argument is annotated with byval, and the generated assembly code pushes the value on the stack instead of passing it on a register.

This PR fixes two problems, one on the caller and one on the callee:

  • On the caller, when an argument should be passed indirectly, it allocates stack memory to hold a copy and the pointer to the allocated memory is passed instead.
  • On the callee, when receiving an indirect argument, no special cast/copy is attempted as the code can modify the memory on the passed pointer freely.

The change in fun.cr is pretty ugly, but I don't currently know what are all the cases it should cover and it's not immediately obvious from the code. The method create_local_copy_of_arg is doing way too much already IMHO.

- When making a call to a C function with an indirect argument, make a caller
  allocated copy and substitute the argument with a pointer to the copy
- Do not attempt to cast an indirect argument on the receiving side
- Enable pending spec in AArch64 and Windows 64
Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

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

This is great, thank you! ❤️

@beta-ziliani beta-ziliani added this to the 1.2.0 milestone Sep 9, 2021
@oprypin
Copy link
Member

oprypin commented Sep 9, 2021

Ooooh yes it works

Peek.2021-09-09.23-04.mp4

@straight-shoota straight-shoota merged commit 3b6e62c into crystal-lang:master Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

C ABI: accepting a struct in a callback works wrong (indirect vs byval)
7 participants