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

Ffigen doesn't generate typedefs compatible with NativeCallable's APIs #431

Closed
Piero512 opened this issue Sep 3, 2023 · 9 comments · Fixed by dart-archive/ffigen#621
Closed
Assignees
Labels
package:ffigen type-enhancement A request for a change that isn't a bug

Comments

@Piero512
Copy link

Piero512 commented Sep 3, 2023

Hello!

Today I was trying out the new NativeCallable APIs and stumbled upon on a kinda annoying issue.

NativeCallable T generic parameter has this problem with ffigen that it requires specifying a function that only uses dart:ffi types, but there isn't currently a way to get one of these, since the generated typedefs usually come from pointer functions.

I think the NativeCallable API should not need to be changed, but say, a static function that removes the pointer and nativefunction from the signature would be useful? Although I do believe that the API should have bound T type to extend NativeFunction.

I haven't checked if ffigen's config section functions -> expose-typedefs would generate the required typedefs, but if it does, it should be documented in the NativeCallable class too.

@Piero512
Copy link
Author

Piero512 commented Sep 3, 2023

Hello!

So I checked ffigen's config functions -> expose-typedefs summary and it still would not generated the required typedefs

The usecase I'm currently using to test this is using the whisper.h file with the following ffigen.yaml

output: 'lib/src/whisper_bindings.dart'
headers:
  entry-points:
    - 'whisper.h'
  include-directives:
    - '**whisper.h'
name: 'WhisperBindings'
structs:
  dependency-only: opaque
functions:
  include:
    - '.*'
  symbol-address:
    include:
      - '.*'
  expose-typedefs:
    include:
      - 'whisper_.*_callback'
description: 'Bindings to OpenAI Whisper model in cpp' 
comments:
  style: any
  length: full

I would think that maybe a similar option but in the typedefs section of the config would probably work for this issue.

@mkustermann
Copy link
Member

@liamappelbe could you have a look at this?

@liamappelbe
Copy link
Contributor

@Piero512 I don't really understand what you're trying to do or what the problem is. Can you give a code example?

@Piero512
Copy link
Author

Piero512 commented Sep 5, 2023

Hello!

I don't know exactly how to summarize it, but the way I ran into this issue is the following.

I am in the progress on writing bindings to the aforementioned header file and while iterating, I decided that I wanted to retrieve the segments as soon as they get discovered. The library exposes users callbacks in the struct whisper_full_params

    struct whisper_full_params {
        int n_threads;
        int n_max_text_ctx;     // max tokens to use from past text as prompt for the decoder
        int offset_ms;          // start offset in ms
        int duration_ms;        // audio duration to process in ms
        // Rest of the header ommited for brevity
        // called for every newly generated text segment
        whisper_new_segment_callback new_segment_callback;
        void * new_segment_callback_user_data;


        // called on each progress update
        whisper_progress_callback progress_callback;
        void * progress_callback_user_data;
}

and I thought that the NativeCallable class could do this work, since (unfortunately) this lib calls those callbacks from worker threads managed by itself.
The issues I ran into while writing that code were the following:
a) The NativeCallback.listener factory doesn't do type inference correctly, and you have to explicitly type them (probably worth another issue).
b) The NativeCallback T generic parameter requires that the T generic parameter be a Function that only has dart:ffi members, which none of the typedefs generated by ffigen are.

  • functions -> expose-typedefs = only exposes functions, not typedefs.
  • the Dart typedef generated by ffigen for the C typedef is for a function pointer:
typedef whisper_progress_callback = ffi.Pointer<
    ffi.NativeFunction<
        ffi.Void Function(
            ffi.Pointer<whisper_context> ctx,
            ffi.Pointer<whisper_state> state,
            ffi.Int progress,
            ffi.Pointer<ffi.Void> user_data)>>;

but the type required for NativeCallback to generate that pointer in the nativeFunction getter is the following.

typedef whisper_progress_callback_nc = ffi.Void 
     Function(
           ffi.Pointer<whisper_context>, 
           ffi.Pointer<whisper_state>,
           ffi.Int progress, 
          ffi.Pointer<ffi.Void>
);

So I would like a solution where I don't have to manually write the typedef for that callback. Since FFI is not dynamic, I would guess the way to go would be to add a new setting to ffigen to generate the required typedefs so we can use this API more ergonomically. But any other solution is fine.
I hope this helps: @liamappelbe
PD: The T parameter bounds is not even inline with the _lookup function that ffigen generates: DynamicLibrary.lookup uses a <T extends NativeType> parameter.

@liamappelbe liamappelbe transferred this issue from dart-lang/sdk Sep 5, 2023
@liamappelbe liamappelbe added the type-enhancement A request for a change that isn't a bug label Sep 5, 2023
@liamappelbe
Copy link
Contributor

Ok, sounds like this is primarily a feature request for ffigen, so I've transferred the issue. The request is for ffigen to detect typedefs of function pointers, and define a second typedef that unwraps the Pointer<NativeFunction<T>> and just typedefs the T (which will be some Function type). Optionally, put this functionality behind a flag.

@Piero512 For issue (a), where the listener constructor isn't doing the type inference you expect, can you file a separate issue with code for this (back on the dart-lang/sdk repo)? Is this something that worked under Pointer.fromFunction, or is this a new feature request? I'm trying to get NativeCallable to fully replace all Pointer.fromFunction use cases. So if this inference works for Pointer.fromFunction then I'd consider it a regression if it doesn't also work for NativeCallable.

@Piero512
Copy link
Author

Piero512 commented Sep 6, 2023

Hopefully I find some free time this week to create a minimum reproducible example for issue A, but then, if those typedefs would be generated, I am sure the issue A would become moot.

@dcharkes
Copy link
Collaborator

dcharkes commented Sep 18, 2023

@Piero512 @liamappelbe RE:dart-archive/ffigen#621 (comment), do you have same examples of the use of the typedefs for both the pointers and the native functions? (In order to decide how common they are and what names to use.)

Maybe @mannprerak2 also has something to say about this, as he added the typedefs earlier.

typedef CXInclusionVisitor = ffi.Pointer<ffi.NativeFunction<CXInclusionVisitor_function>>;

typedef CXInclusionVisitor_function = ffi.Void Function(
    CXFile included_file,
    ffi.Pointer<CXSourceLocation> inclusion_stack,
    ffi.UnsignedInt include_len,
    CXClientData client_data);

vs (breaking change)

typedef CXInclusionVisitor_pointer = ffi.Pointer<ffi.NativeFunction<CXInclusionVisitor>>;

typedef CXInclusionVisitor = ffi.Void Function(
    CXFile included_file,
    ffi.Pointer<CXSourceLocation> inclusion_stack,
    ffi.UnsignedInt include_len,
    CXClientData client_data);

@mannprerak2
Copy link
Contributor

No input from my side, when we added typedefs, all the types were inline since I wasn't aware of such usecase.

As per the naming, maybe we should keep the name used by C intact, since C is exposing the typedef to a function pointer itself, but then we are further decomposing it to another typedef. This approach is also non breaking.

@dcharkes
Copy link
Collaborator

Okay, maybe we should just land as is then.

liamappelbe referenced this issue in dart-archive/ffigen Sep 19, 2023
* Fix #614

* Update test expectations

* Hacky fix

* Refactor

* Fix analysis

* Fix analysis

* Fix analysis
@liamappelbe liamappelbe transferred this issue from dart-archive/ffigen Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:ffigen type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants