Allow to override the return type of FFI functions #4060

ergl · 2022-03-18T18:12:40Z

When this is merged, the commit message shoud be the following:

Allow overriding the return type of FFI functions

On the sync call for March 1st, we decided to re-add the ability to specify a
return type for FFI function calls. This was previously removed by RFC 68 on the
grounds that, with FFI declarations being mandatory, specifying a return type
would be redundant.

Before RFC 68 was implemented, the return type at the call site was not allowed
to differ from the return type of the declaration. We have lifted this
limitation, which should help users write FFI bindings for C functions that
might return different types depending on how they are called.

The return type of an FFI function without an explicit return type will default
to the return type of its declaration.

ponylang-main · 2022-03-18T18:13:00Z

Hi @ergl,

The changelog - added label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 4060.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

SeanTAllen · 2022-03-18T18:17:51Z

Sweet

SeanTAllen · 2022-03-18T18:18:26Z

Are there any tutorial updates that need to go along with this?

ergl · 2022-03-18T18:23:29Z

@SeanTAllen We should probably add a note about this in the tutorial, although what's currently there is still valid.

SeanTAllen · 2022-03-18T18:26:45Z

@ergl can you handle the "sounds like it is small" tutorial PR?

ergl · 2022-03-18T18:27:35Z

@SeanTAllen Yeah, no problem.

.release-notes/4060.md

On the sync call for March 1st, we decided to re-add the ability to specify a return type for FFI function calls. This was previously removed by RFC 68 on the grounds that, with FFI declarations being mandatory, specifying a return type would be redundant. Before RFC 68 was implemented, the return type at the call site was not allowed to differ from the return type of the declaration. We have lifted this limitation, which should help users write FFI bindings for C functions that might return different types depending on how they are called. The return type of an FFI function without an explicit return type will default to the return type of its declaration.

ergl · 2022-03-20T20:47:07Z

@SeanTAllen I've reworked the release notes, and added some examples of what this change makes possible. Let me know if you think it's too long/verbose, and I can try to trim it a bit.

I'll tackle the tutorial PR tomorrow.

SeanTAllen

I like the release notes.

.release-notes/4060.md

SeanTAllen · 2022-03-21T14:20:29Z

Before merging, I'd like @jemc to sign off on this to verify that it matches his expectation of what we agreed to do for re-enabling this functionality.

jemc · 2022-03-21T15:52:09Z

test/libponyc-run/ffi-different-returns/main.pony

+use @ffi_function[I32]()
+use @pony_exitcode[None](code: I32)
+
+actor Main
+  new create(env: Env) =>
+    let res: (I32 | U8) = @ffi_function[U8]()


This actually needs to be disallowed - the differing return type needs to be ABI-compatible with all other invocations of the same function. Specifically, I think we need to enforce that all of them are the same ABI size.

This is easy for pointers and classes, because they will all be pointer-sized.

But in this case you have a 1-byte and a 4-byte variant, which are not compatible.

So that was a bug we had previously with return types for FFI?

No, I thought we were preventing this previously with a compiler check, but I could be misremembering.

Here's an example of where we check ABI compatibility for the param/arg type (in the latest version of the compiler):

ponyc/src/libponyc/codegen/gencall.c

Lines 1124 to 1129 in ca6d725

if((LLVMABISizeOfType(c->target_data, param) !=

LLVMABISizeOfType(c->target_data, arg_type)))

{

report_ffi_type_err(c, decl, ast, name);

return NULL;

}

definitely something we need to make sure we have tests for then. and we should update the release notes and tutorial to explain the limitation.

thanks for catching that. it makes sense as a limitation but i wasn't thinking of it when i reviewed.

From what I remember, right now we only apply that check across packages to verify that different declarations for the same function are compatible. For FFI calls on a single package, we assumed that the types would always match the local declaration, I think. I'll try to add the check for the return types.

After discussing this with @jemc, we agreed that this check is "good enough" for our purposes, as it will catch multiple calls to the same FFI function with incompatible return types.

It's not feasible to perform the ABI size check against the return type as specified in the FFI declaration without rearchitecting how we perform the code generation for FFI functions in a significant way.

From the point of view of the implementation the compiler will treat the first FFI call to a function as "authoritative", and all future calls to this function will be checked for ABI compatibility against it. This didn't cause a problem before because type checking was making sure that the return type for calls matched the return type of the declaration.

With just a single FFI call happening, there's nothing to check against, and as such the compiler will generate a function with the return type as specified by the call.

ergl · 2022-03-23T20:07:48Z

I'll add a note to the release notes about the ABI compatiblity. Other than that, I think this is good to merge.

After introducing #4060, we forgot to re-add the code that ensured that, if a FFI function call specified different return types, those types were being marked as reachable. This created a situation where programs that introduced a new type (for example, a bare lambda) in the context of a FFI return type would crash the compiler. This was due to having missing information about the type during code generation.

ergl added the changelog - added Automatically add "Added" CHANGELOG entry on merge label Mar 18, 2022

ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Mar 18, 2022

SeanTAllen reviewed Mar 18, 2022

View reviewed changes

.release-notes/4060.md Outdated Show resolved Hide resolved

ergl force-pushed the ergl/ffi_return_types branch 2 times, most recently from 9412b9e to 711ae62 Compare March 20, 2022 20:43

ergl force-pushed the ergl/ffi_return_types branch from 711ae62 to 7247ca5 Compare March 20, 2022 20:45

SeanTAllen approved these changes Mar 20, 2022

View reviewed changes

ergl commented Mar 20, 2022

View reviewed changes

.release-notes/4060.md Outdated Show resolved Hide resolved

Update .release-notes/4060.md

6bce8bb

ergl mentioned this pull request Mar 21, 2022

Document return-type polymorphism in FFI ponylang/pony-tutorial#508

Merged

jemc reviewed Mar 21, 2022

View reviewed changes

ergl added the do not merge This PR should not be merged at this time label Mar 22, 2022

ergl added 3 commits March 23, 2022 20:27

Give more context for incompatible FFI calls

2154c38

Don't use ABI-incompatible example

ededac4

Add ABI check test

4f80b42

ergl removed the do not merge This PR should not be merged at this time label Mar 23, 2022

Update release notes [skip ci]

e079f52

jemc approved these changes Mar 23, 2022

View reviewed changes

SeanTAllen merged commit 229775e into main Mar 24, 2022

SeanTAllen deleted the ergl/ffi_return_types branch March 24, 2022 00:34

ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Mar 24, 2022

github-actions bot pushed a commit that referenced this pull request Mar 24, 2022

Updates release notes for PR #4060

a78cb7c

github-actions bot pushed a commit that referenced this pull request Mar 24, 2022

Update CHANGELOG for PR #4060

748d335

ergl mentioned this pull request Jun 21, 2022

Ensure reachability of types returned by FFI calls #4149

Merged

redvers mentioned this pull request Jan 9, 2023

Incorrect generic FFI syntax compiles #4300

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to override the return type of FFI functions #4060

Allow to override the return type of FFI functions #4060

ergl commented Mar 18, 2022 •

edited

Loading

ponylang-main commented Mar 18, 2022

SeanTAllen commented Mar 18, 2022

SeanTAllen commented Mar 18, 2022

ergl commented Mar 18, 2022

SeanTAllen commented Mar 18, 2022

ergl commented Mar 18, 2022

ergl commented Mar 20, 2022 •

edited

Loading

SeanTAllen left a comment

SeanTAllen commented Mar 21, 2022

jemc Mar 21, 2022

SeanTAllen Mar 21, 2022

jemc Mar 21, 2022

jemc Mar 21, 2022 •

edited

Loading

SeanTAllen Mar 21, 2022

ergl Mar 21, 2022

ergl Mar 23, 2022 •

edited

Loading

ergl commented Mar 23, 2022

	if((LLVMABISizeOfType(c->target_data, param) !=
	LLVMABISizeOfType(c->target_data, arg_type)))
	{
	report_ffi_type_err(c, decl, ast, name);
	return NULL;
	}

Allow to override the return type of FFI functions #4060

Allow to override the return type of FFI functions #4060

Conversation

ergl commented Mar 18, 2022 • edited Loading

ponylang-main commented Mar 18, 2022

SeanTAllen commented Mar 18, 2022

SeanTAllen commented Mar 18, 2022

ergl commented Mar 18, 2022

SeanTAllen commented Mar 18, 2022

ergl commented Mar 18, 2022

ergl commented Mar 20, 2022 • edited Loading

SeanTAllen left a comment

Choose a reason for hiding this comment

SeanTAllen commented Mar 21, 2022

jemc Mar 21, 2022

Choose a reason for hiding this comment

SeanTAllen Mar 21, 2022

Choose a reason for hiding this comment

jemc Mar 21, 2022

Choose a reason for hiding this comment

jemc Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

SeanTAllen Mar 21, 2022

Choose a reason for hiding this comment

ergl Mar 21, 2022

Choose a reason for hiding this comment

ergl Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

ergl commented Mar 23, 2022

ergl commented Mar 18, 2022 •

edited

Loading

ergl commented Mar 20, 2022 •

edited

Loading

jemc Mar 21, 2022 •

edited

Loading

ergl Mar 23, 2022 •

edited

Loading