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

Missing mechanism to pass/receive struct values into/from FFI C functions #183

Open
stefandd opened this issue Sep 6, 2020 · 9 comments

Comments

@stefandd
Copy link
Contributor

stefandd commented Sep 6, 2020

The commit ponylang/ponyc@221169b removed, on the basis of some C ABI-dependent unexpected behaviors/bugs, the only mechanism to pass value structs into FFI functions or to receive them. In the commit, a proposal was made to use libclang to access the correct call signatures and the removal of tuples for FFI functions was intended to be an interim measure until such a change was made: "... 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."

A little more than 3 years later no solution is in sight, now somewhat putting into question the decision to entirely remove the ability to wrap C functions that expect or return struct values. This severely limits the ability to wrap large C libraries with fairly simple value structs such as sfVector2f below for which the tuple solution might have worked fine! There are hacks one can use to make this work for this specific struct or structs of good-natured size.

Given the usefulness of a mechanism to pass or receive value structs in FFI scenarios and given the lack of progress that would have justified the removal of the tuple mechanism from the language althogether, I propose to either renable the passing and receiving of tuples (revert the above commit) or to provide a mechanism by which chunks of raw byte data can be generated, cast into Pony primitives, and passed into or received from FFI C functions. Unfortunately, even the hack workarounds shown below only work if by chance the struct size matches the size of a Pony primitive like U32, U64 etc. The struct sfVector3f with three floats cannot be worked around this way since there is no U96 or I96 in Pony.

----------------------------- Detailed example

Take, for example, the following C library wrapped in Pony as "debugstruct.lib"

typedef struct
{
    float x;
    float y;
} sfVector2f;

void PrintStruct(sfVector2f s) {
    printf("%f, %f\n", s.x, s.y);
}

unsigned long long sfVector2f_to_U64(sfVector2f* s) {
    return *((unsigned long long*)(s));
}

This currently can't be made to print the sfVector2f struct with Pony code such as:

use "lib:debugstruct"
use @PrintStruct[None](vec : SFVector2f) // does not work because it passes by reference
//use @PrintStruct[None](x : F32, y : F32)  // this does not work as work-around

struct SFVector2f
    let posx : F32
    let posy : F32

    new create(x : F32, y : F32) =>
        posx = x
        posy = y

actor Main
    let _env : Env

    new create(env : Env) =>
        _env = env
        let vec : SFVector2f = SFVector2f(1.0, 2.0)
        @PrintStruct[None](vec) // compiles but does not work
        // @PrintStruct[None](vec.x, vec.y) // using the second definition above it also compiles but does not work

For this specific 64-bit struct, this can be made work by a hack that involves both, another C function that casts the two float-struct into a unsigned long long in C, and misinforming Pony about the signature of the C function.

use "lib:debugstruct"
use @PrintStruct[None](arg : U64)
use @sfVector2f_to_U64[U64](arg : SFVector2f)

struct SFVector2f
    let posx : F32
    let posy : F32

    new create(x : F32, y : F32) =>
        posx = x
        posy = y

actor Main
    let _env : Env
    let vec : SFVector2f = SFVector2f(1.0, 2.0)
    
    new create(env : Env) =>
        _env = env
        let x : U64 = @sfVector2f_to_U64(vec)
        @PrintStruct(x)

This will print 1.000000, 2.000000. Another way to accomplish this hack would be to something like

type SFVector2fRaw is NullablePointer[SFVector2f]
		...
	    var tmp : U64 = 0
	    @memcpy(addressof tmp, SFVector2fRaw(vec), USize(8))
	    @PrintStruct(tmp)
@SeanTAllen SeanTAllen transferred this issue from ponylang/ponyc Sep 6, 2020
@jemc
Copy link
Member

jemc commented Sep 8, 2020

Just to link back to the discussion around this that happened in chat, here's the relevant Zulip thread: https://ponylang.zulipchat.com/#narrow/stream/189985-beginner-help/topic/FFI.20receiving.20a.20struct.20(value).20crashes.2E

A summary of my main thoughts on this issue, as expressed there:

  • The best, most robust solution would be to integrate the compiler with libclang and give it access to the C headers so that it can reproduce the exactly correct C ABI (leveraging clang's extensive knowledge of various platforms and edge cases). This would be a nontrivial effort, but would allow a lot of other useful FFI features and is considered to be generally desirable if we can make it happen cleanly.

  • In my opinion, the best short-term solution to unblock the OP would be to partially revert the change that disallows this - disallow it by default for safety, but allow users like the OP to explicitly enable it as a compilation flag, recognizing that it may not be safe/correct for some C calls or on some platforms, allowing the user to take responsibility for the potential correctness issues that result.

  • I don't think that the workarounds allowing the language user to handle byte packing/unpacking themselves are a satisfactory kind of solution - it has the same problems that the ponyc compiler has in not knowing whether in the correct call convention the argument is truly represented as a struct or not. In cases where it is not, there would be the same issues the compiler had. So in these workarounds we'd be asking the user to jump through a bunch of additional hoops for no benefit - the same problems would be there as if we had merely done the processing in ponyc as before.

@SeanTAllen
Copy link
Member

During sync. Another point that was discussed as an option was that we could do code generation of the boilerplate associated with functionality as it stands.

@SeanTAllen
Copy link
Member

Note that @Praetonus mentions ponylang/ponyc#2012 (comment) that an interface for clang needed to be exposed and we were doing this because it wasn't exposed. It might be possible that it has since been exposed.

The last comment is "I'm going to submit a patch to Clang to expose that interface.". I have no idea what happened with that.

@SeanTAllen
Copy link
Member

We would love it if someone would look into interfacing with clang and report back on the feasibility.

@stefandd
Copy link
Contributor Author

stefandd commented Sep 9, 2020

@SeanTAllen: any thoughts coming out of the sync re: the short-term solution proposed by @jemc since that would not require the things to maybe have happened over at Clang?

@SeanTAllen
Copy link
Member

I'm personally more in favor of code generation rather than putting something that is broken back in. However, the something that might have happened, if it happened could be the best (and a fairly quick) path forward.

More information is needed and whoever undertakes writing an RFC would ideally look into and weigh the pros and cons of each approach and present them in the RFC.

@SeanTAllen
Copy link
Member

If anyone is interested in the nuance of the conversation so far that happened at the sync, you can download the recording at https://sync-recordings.ponylang.io/r/2020_09_08.m4a.

Getting that nuance would be a good idea for anyone who is interested in writing an RFC for this.

@SeanTAllen
Copy link
Member

We discussed this briefly at sync and believe that between last week's recording (for nuance) and what is on this issue should be enough for someone to draft an RFC for this. If you are interested in exploring the issues here in, go for it.

@SeanTAllen
Copy link
Member

We discussed this again today at sync and @ergl is looking into this more and will be working on an RFC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants