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

Forbid returning and passing tuples to FFI functions #2012

Merged
merged 1 commit into from Jul 15, 2017

Conversation

Praetonus
Copy link
Member

Passing complex objects by value in C is hard: there are many different calling conventions depending on the platform. For example, a C compiler is allowed to split the members of an object into different parameters, or to change the return value into an out parameter.

The Pony compiler currently uses a naive implementation to pass objects to FFI functions and isn't able to use the correct calling convention for every call. Ideally, we'd want to rely on a C compiler API (like LibClang) to generate the correct signatures and calls.

This change is a temporary measure to avoid spurious FFI bugs until we have the interoperability detailed above. LLVM intrinsics are still allowed to return and be passed tuples since their signatures are defined by LLVM.

@Praetonus Praetonus added the changelog - changed Automatically add "Changed" CHANGELOG entry on merge label Jul 6, 2017

if(!intrinsic && (ast_id(decl_ret_type) == TK_TUPLETYPE))
{
ast_error(opt->check.errors, decl_ret_type, "FFIs cannot return tuples");
Copy link
Member

Choose a reason for hiding this comment

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

It's just a small tweak, but for consistency with some of the other compiler messages, I'd prefer to phrase this as "an FFI function cannot return a tuple".

@Praetonus
Copy link
Member Author

After fiddling with the Clang API, I realised that setting up the interoperability might be easier than I initially thought. I'll probably have a basic implementation soon, so I'll mark this PR as DO NOT MERGE. If I encounter a major difficulty with Clang, then we can unblock this PR until it is sorted out.

@Praetonus Praetonus added the do not merge This PR should not be merged at this time label Jul 6, 2017
@Praetonus
Copy link
Member Author

Actually, the interoperability won't be easy to implement because an important functionality (converting Clang types to LLVM types) is only part of Clang's internals and wouldn't be practical to replicate in ponyc. I'm going to submit a patch to Clang to expose that interface.

@Praetonus Praetonus removed the do not merge This PR should not be merged at this time label Jul 8, 2017
@SeanTAllen
Copy link
Member

@aturley does this impact on any of our sendence language binding code?

@jemc
Copy link
Member

jemc commented Jul 12, 2017

As discussed on sync, this is ready to merge, but we wanted to give @SeanTAllen and/or @aturley a chance to raise objections first.

@SeanTAllen
Copy link
Member

@aturley is really the one who needs to weigh in...

@aturley
Copy link
Member

aturley commented Jul 12, 2017

@SeanTAllen @jemc this doesn't impact us.

@jemc
Copy link
Member

jemc commented Jul 12, 2017

Okay, then this is good to merge once @Praetonus resolves the merge conflicts.

Passing complex objects by value in C is hard: there are many different
calling conventions depending on the platform. For example, a C
compiler is allowed to split the members of an object into different
parameters, or to change the return value into an out parameter.

The Pony compiler currently uses a naive implementation to pass objects
to FFI functions and isn't able to use the correct calling convention
for every call. Ideally, we'd want to rely on a C compiler API (like
LibClang) to generate the correct signatures and calls.

This change is a temporary measure to avoid spurious FFI bugs until
we have the interoperability detailed above. LLVM intrinsics are still
allowed to return and be passed tuples since their signatures are
defined by LLVM.
@Praetonus
Copy link
Member Author

Conficts fixed, CI is passing, merging.

@Praetonus Praetonus merged commit 221169b into ponylang:master Jul 15, 2017
@Praetonus Praetonus deleted the ffi-tuples branch July 15, 2017 15:11
ponylang-main added a commit that referenced this pull request Jul 15, 2017
stefandd added a commit to stefandd/ponyc that referenced this pull request Jun 21, 2022
This removes the checks that were introduced in PR ponylang#2012 that only allow intrinsic FFI calls to use tuples.

Now, tuples can again be passed into and returned from FFI functions (with all previously identified limitations).
jemc pushed a commit to jemc/ponyc that referenced this pull request Jul 7, 2022
This removes the checks that were introduced in PR ponylang#2012 that only allow intrinsic FFI calls to use tuples.

Now, tuples can again be passed into and returned from FFI functions (with all previously identified limitations).
jemc pushed a commit that referenced this pull request Jul 7, 2022
This removes the checks that were introduced in PR #2012 that only allow intrinsic FFI calls to use tuples.

Now, tuples can again be passed into and returned from FFI functions (with all previously identified limitations).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - changed Automatically add "Changed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants