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

dart:ffi: Support variadic C functions #38578

Closed
terrier989 opened this issue Sep 26, 2019 · 34 comments
Closed

dart:ffi: Support variadic C functions #38578

terrier989 opened this issue Sep 26, 2019 · 34 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. library-ffi

Comments

@terrier989
Copy link

If "dart:ffi" supported varargs calling conventions in X64/ARM64, developers could use Objective-C libraries by using C functions such as objc_msgSend.

Related issues:

@mit-mit mit-mit added the area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. label Sep 26, 2019
@terrier989
Copy link
Author

Package cupertino_ffi contains the necessary helpers (NSString, reference counting, etc.) for using any Objective-C library, but inability to call the variadic C function objc_msgSend is a blocker.

@mraleph
Copy link
Member

mraleph commented Oct 2, 2019

@terrier989 note that you can just bind to objc_msgSend multiple times with different signatures (with 0, 1, 2, 3, 4, 5, 6, etc additional parameters). Would not that be good enough as a workaround?

@terrier989
Copy link
Author

@mraleph
In the X64 variadic convention, register AH (RAX) is used pass an upper bound of XMM register arguments to the callee. In the X64-mac trampoline, the function pointer is stored in RAX. After studying this issue more, it seems that objc_msgSend can be safely used with any RAX.

@dcharkes dcharkes added this to Flutter MVP in Dart VM FFI via automation Oct 3, 2019
@dcharkes dcharkes moved this from Flutter MVP to 1.0 in Dart VM FFI Oct 3, 2019
@dcharkes
Copy link
Contributor

dcharkes commented Oct 3, 2019

Cool project @terrier989!

/cc @sjindel-google

@sjindel-google
Copy link
Contributor

It sounds like variadic functions are supported as well as possible, given that Dart does not have variadic functions (except for noSuchMethod).

Dart VM FFI automation moved this from 1.0 to Done Oct 3, 2019
@dcharkes
Copy link
Contributor

dcharkes commented Oct 3, 2019

given that Dart does not have variadic functions

We could support variadic functions with a List as argument in the Dart signature, if we really wanted to, but I'm not sure we should.

@sjindel-google
Copy link
Contributor

given that Dart does not have variadic functions

We could support variadic functions with a List as argument in the Dart signature, if we really wanted to, but I'm not sure we should.

We can't support that actually, because the length of the list and the type of arguments in the list needs to be fixed when we compile the trampolines.

@dcharkes
Copy link
Contributor

dcharkes commented Oct 3, 2019

given that Dart does not have variadic functions

We could support variadic functions with a List as argument in the Dart signature, if we really wanted to, but I'm not sure we should.

We can't support that actually, because the length of the list and the type of arguments in the list needs to be fixed when we compile the trampolines.

Couldn't we make a generic trampoline like we had in DBC to accomodate for that? It's just going to be relatively slow...

@terrier989
Copy link
Author

I don't see need for passing a List. This is a calling convention issue.

In X64, the trampoline used by 'dart:ffi' passes an incorrect integer in AH (8 bits of RAX) to varargs C functions. But it's an implementation detail of objc_msgSend whether the wrong integer in RAX causes problems. It probably doesn't, but a simple way to be sure could be putting some high constant into AH before 'call' instruction.

In ARM64, the standard non-varargs ABI passes the first 8 arguments in registers X0-X7, whereas the standard varargs ABI passes all arguments in the stack. However, it turns out that Apple decided to diverge from the ARM64 standard and if I interpreted the document correctly, it's possible to call a varargs function with an ordinary trampoline.

@mraleph
Copy link
Member

mraleph commented Oct 3, 2019

Fair point @terrier989 - I was under impression that varargs calling convention is the same as non-varargs, which turns out is not the case. Things would work on X64 as long as you don't need to pass any double values down to the vararg function.

@mraleph mraleph reopened this Oct 3, 2019
Dart VM FFI automation moved this from Done to Flutter MVP Oct 3, 2019
@dcharkes
Copy link
Contributor

dcharkes commented Oct 28, 2019

In order to distinguish the varargs calling convention from the non varargs calling convention in dart:ffi, we need to indicate that somewhere.

We have some options of doing this:

  1. We add a NativeVarArgsFunction next to NativeFunction, a shared super type of both and let asFunction and lookupFunction support both.
  2. We add asVarArgFunction and lookupVarArgFunction to indicate it should use the vararg calling convention. This kind of weird, because now the lookup looks up a signature which is not designated as varargs, but then is told to be varargs when doing asFunction after a lookup.
  3. We add an annotation on the signatures in asFunction and lookupFunction. (Has the same issue as above.)

They are all equivalent in the sense that they communicate the same information for the creation of trampolines.

If we think about carrying function pointers around in the program, then option 1 allows us to express which calling convention is used:

Pointer<NativeVarArgsFunction<... Function(...)>> fptr = // ...

(N.b. if there are more calling convention variations we want to take into account at some point, that might influence on how we deal with multiple calling conventions. Would we ever want to support a range of 32-bit calling conventions?)

cc @mkustermann

Edit: As discussed offline: vararg callbacks do not really make sense.

@dcharkes dcharkes removed this from Flutter MVP in Dart VM FFI Dec 16, 2020
@Sunbreak
Copy link

If variadic C functions is unsupported, could inline function generation be an option?

For example, hidctl_open could be a wrapper for open

#include <fcntl.h>

static inline int hidctl_open (const char *__path, int __oflag) {
    return open(__path, __oflag);
}

@dcharkes
Copy link
Contributor

dcharkes commented Mar 1, 2021

Why inline? Wouldn't you want to generate non-inline functions so that you can actually bind to them from Dart?

Depending on how many variadic functions you're looking at, it might be worthwhile to make a PR to package:ffigen to generate wrapper functions automatically.

@Sunbreak
Copy link

Sunbreak commented Mar 1, 2021

I'm working on https://github.com/Sunbreak/logic_conf.dart, a re-implementation of https://github.com/libusb/hidapi

The linux implementation use open(__path, __oflag,...) in <fcntl.h>: https://man7.org/linux/man-pages/man2/open.2.html

Is it possible to write the wrap code manually?

@mkustermann
Copy link
Member

For example, hidctl_open could be a wrapper for open static inline int hidctl_open (const char *__path, int __oflag) { ... }

The static makes this function not an exported symbol from the shared library. If this is declared in a .h file it is similar to helper macros which we cannot access from Dart either.

The linux implementation use open(__path, __oflag,...) in <fcntl.h>: https://man7.org/linux/man-pages/man2/open.2.html

Can you just bind to open() via a different signature in Dart (either with 2 or 3 parameters)?

The actually problem here is that open() might return -1 and set errno:

RETURN VALUE
       open(), openat(), and creat() return the new file descriptor (a nonnegative integer), or -1 if an error occurred (in which case, errno is set appropriately).

ERRORS
...
       EINTR  While blocked waiting to complete an open of a slow device (e.g., a FIFO; see fifo(7)), the call was interrupted by a signal handler; see signal(7).

So if EINTR is returned you'd want to retry. But right now there's no way to safely obtain the errno set by this call. I've opened an issue for this a while ago #38832

This applies to most syscalls.

@Sunbreak
Copy link

Sunbreak commented Mar 1, 2021

Can you just bind to open() via a different signature in Dart (either with 2 or 3 parameters)?

Could you shed some light on how to bind with 2 or 3 parameters?

Or maybe a plain declaration just works?

final int Function(Pointer<Int8> __path, int __oflag) nativeOpen2 = libfcntl
    .lookup<NativeFunction<Int32 Function(Pointer<Int8>, Int32)>>("open")
    .asFunction();

final int Function(Pointer<Int8> __path, int __oflag, int __ext) nativeOpen3 = libfcntl
    .lookup<NativeFunction<Int32 Function(Pointer<Int8>, Int32, Int32)>>("open")
    .asFunction();

@mkustermann
Copy link
Member

Or maybe a plain declaration just works?

Yes. On ia32/arm/arm64 the varargs calling convention for this signature seems to be the same. Only x64 is slightly problematic because it normally requires eax to contain the number of xmm registers to be passed. Though the open function doesn't get any doubles, so it shouldn't matter what eax contain (see also discussion above in this thread).

@Sunbreak
Copy link

Sunbreak commented Mar 2, 2021

Seems to work. Thanks a lot

https://github.com/Sunbreak/logic_conf.dart/blob/a2b18a947fec46859ad09fcd44050a305c14909d/linux/libc.h#L10

// FIMXE https://github.com/dart-lang/sdk/issues/38578
// Need manully change name from `open2` to `open` after `ffigen`
extern int open2 (const char *__file, int __oflag) __nonnull ((1));

https://github.com/Sunbreak/logic_conf.dart/blob/a2b18a947fec46859ad09fcd44050a305c14909d/lib/src/linux/libc.dart#L14-L22

  int open2(
    ffi.Pointer<ffi.Int8> __file,
    int __oflag,
  ) {
    return (_open2 ??= _dylib.lookupFunction<_c_open2, _dart_open2>('open'))(
      __file,
      __oflag,
    );
  }

@dcharkes
Copy link
Contributor

It looks like culprit is the way how variadic ioctl function is defined in dart https://github.com/timsneath/dart_console/blob/f88ab598bf896878b347013400049d0702eecd0c/lib/src/ffi/unix/ioctl.dart#L35

// int ioctl(int, unsigned long, ...);
typedef IOCtlNative = Int32 Function(Int32, Int64, Pointer<Void>);
typedef IOCtlDart = int Function(int, int, Pointer<Void>);

Such ioctl invocation ends up corrupting vm isolate structure causing a crashes.

Originally posted by @aam in #49460 (comment)

It started to fail on mac arm64 because calling convention on that platform for passing variable args for int ioctl(int fd, unsigned long request, ...); differs from what is expected from Int16 Function(Int16 fd, Int32 cmd, Pointer<WinSize> winsize);: variadic args are expected to be on the stack, but latter puts all three args in the registers.

@Sunbreak
Copy link

  • Linux arm64

    • [NO ERROR] Dart SDK version: 2.18.0-271.4.beta (beta) (Tue Jul 26 10:14:06 2022 +0200) on "linux_arm64"
    • [NO ERROR] Dart SDK version: 2.19.0-32.0.dev (dev) (Sun Jul 24 15:18:55 2022 -0700) on "linux_arm64"
  • macOS arm64

    • [ERROR] Dart SDK version: 2.19.0-32.0.dev (dev) (Sun Jul 24 15:18:55 2022 -0700) on "macos_arm64"
    • [NO ERROR] Dart SDK version: 2.18.0-271.4.beta (beta) (Tue Jul 26 10:14:06 2022 +0200) on "macos_arm64"
// int ioctl(int, unsigned long, ...);
import 'dart:ffi';
import 'dart:io';

import 'package:ffi/ffi.dart';

typedef IOCtlNative = Int32 Function(Int32, Int64, Pointer<Void>);
typedef IOCtlDart = int Function(int, int, Pointer<Void>);

final TIOCGWINSZ = Platform.isMacOS ? 0x40087468 : 0x5413;
const STDIN_FILENO = 0;
const STDOUT_FILENO = 1;
const STDERR_FILENO = 2;

// struct winsize {
//      unsigned short  ws_row;         /* rows, in characters */
//      unsigned short  ws_col;         /* columns, in characters */
//      unsigned short  ws_xpixel;      /* horizontal size, pixels */
//      unsigned short  ws_ypixel;      /* vertical size, pixels */
// };
class WinSize extends Struct {
  @Int16()
  external int ws_row;

  @Int16()
  external int ws_col;

  @Int16()
  external int ws_xpixel;

  @Int16()
  external int ws_ypixel;
}

void main() {
  final ioctl = DynamicLibrary.process().lookupFunction<IOCtlNative, IOCtlDart>('ioctl');

  final winSizePointer = calloc<WinSize>();
  final result = ioctl(STDOUT_FILENO, TIOCGWINSZ, winSizePointer.cast());
  print('result is $result');

  final winSize = winSizePointer.ref;
  print('Per ioctl, this console window has ${winSize.ws_col} cols and '
      '${winSize.ws_row} rows.');

  calloc.free(winSizePointer);
}

@dcharkes
Copy link
Contributor

When doing FFI calls to variadic functions we need a binding for a specific amount of arguments on the Dart side. We need to generate machine code ahead of time for the trampolines.

Moreover, we need a way to signify from which argument the variadic arguments start.

API design 1

We could introduce a marker for native signatures as follows:

/// `int printf(const char *format, ...)` with `int` and `double` as varargs.
typedef NativePrintfIntDouble =
    Int Function(Pointer<Char>, VarArgs<Int>, Double);

The VarArgs NativeType would be defined as:

/// Represents the start of varargs in C.
///
/// The signatures in [NativeFunction] need to specify the exact types used for
/// FFI calls.
///
/// For example take calling `printf` in C.
///
/// ```c
/// int printf(const char *format, ...);
///
/// void call_printf() {
///   int a = 4;
///   double b = 5.5;
///   const char* format = "...";
///   printf(format, a, b);
/// }
/// ```
///
/// To call `printf` directly from Dart with those two argument types, define
/// the native type as follows:
///
/// ```dart
/// /// `int printf(const char *format, ...)` with `int` and `double` as
/// /// varargs.
/// typedef NativePrintfIntDouble =
///     Int Function(Pointer<Char>, VarArgs<Int>, Double);
/// ```
///
/// Note how [VarArgs] signals where the variadic arguments start and the all
/// the arguments passed are covered.
///
/// [VarArgs] is not constructible in the Dart code and serves purely as marker
/// in type signatures.
@Since('2.19')
abstract class VarArgs<T extends NativeType> extends NativeType {}

API design 2

Alternatively, we could add a marker before the first variadic argument rather than "around" it:

/// `int printf(const char *format, ...)` with `int` and `double` as varargs.
typedef NativePrintfIntDouble =
    Int Function(Pointer<Char>, VarArgs, Int, Double);

Pro:

  • All variadic argument types are treated the same, rather than only wrapping the first.

Con:

  • The Dart and native signature do not have the same amount of arguments.

@mraleph
Copy link
Member

mraleph commented Sep 13, 2022

@dcharkes Maybe we could consider an alternative design which outlaws direct closure calls and instead relies on static dispatch, e.g.

@FfiNative<IntPtr Function(Pointer<Char>, VarArg)>
external int printf(Pointer<Char> fmt, List<Object> args);

printf("%s %d", [nativeValue1, nativeValue2]);  // ok
printf("%s %d", someArray); // error

If somebody needs closure calls we could provide a helper method in FFI for calling variadic functions:

Pointer<NativeFunction<IntPtr Function(Pointer<Char>, VarArg)>> printfPtr;
callVariadic(printfPtr, ["%s %d", nativeValue1, nativeValue2])

If we go this route then we can use local types to lower the call-site (just like it would be in C).

@dcharkes
Copy link
Contributor

dcharkes commented Sep 13, 2022

List<Object> args

I believe this will not work, because we need the native types for the arguments at compile time to generate the correct trampoline. (I don't believe every ABI always uses only stack arguments. Some ABIs use the same registers as for regular arguments.)

So with FfiNatives we would need:

@FfiNative<Int Function(Pointer<Char>, VarArgs<Int>, Double))>
external int printf(Pointer<Char> fmt, int, double);

Edit: A native signature type to signifying where var-args start works with both FfiNatives and DynamicLibrary. (I agree that we should move in the direction of more FfiNatives.)

@mraleph
Copy link
Member

mraleph commented Sep 13, 2022

@dcharkes Oh right. For integers and doubles we need the right size. Maybe then:

@FfiNative<IntPtr Function(Pointer<Char>, VarArg)>
external int printf<NativeType /* extends Record */>(Pointer<Char> fmt, List<Object> args);

Users are expected to do:

printf<(Int32, Double)>("%d %lf", [a, b])

Maybe we could go further and say that the second argument does not have to be a constant array, but rather a tuple of a compatible Dart type?

printf<(Int32, Double)>("%d %lf", (a, b)); // ok

(int, double) v;
printf<(Int32, Double)>("%d %lf", v); // ok too

@dcharkes
Copy link
Contributor

@dcharkes Oh right. For integers and doubles we need the right size. Maybe then:

@FfiNative<IntPtr Function(Pointer<Char>, VarArg)>
external int printf<NativeType /* extends Record */>(Pointer<Char> fmt, List<Object> args);

Users are expected to do:

printf<(Int32, Double)>("%d %lf", [a, b])

That could work, records essentially give us a way to specify arbitrary types without a function type. :-)

Based on the ABI logic I've seen, we would still need to compile a trampoline for each different call-site signature. (Which requires some refactoring of the current implementation.)

There are still some downsides though:

  1. Manual type checking between the record and type arguments.
  2. asFunction cannot support this, as we don't know the call sites of where the closure flows through.
    a. Wrapping in a closure as you suggested works for native assets, but if someone wants to write actual dynamic code with dlopen on a desktop machine we don't support variadic args at all. (I believe you or @mkustermann mentioned some time back that keeping DynamicLibrary in dart:ffi instead of complete removing it would be a good thing. If so, we should probably support the same feature set.)
  3. If we have multiple call sites of a variadic argument with the same native types, they need to be repeated every time. (Though with my earlier design having different native types will lead to multiple definitions which will likely be postfixed with the native type.)

Pros:

  1. Single definition. 👌

Maybe we could go further and say that the second argument does not have to be a constant array, but rather a tuple of a compatible Dart type?

I don't understand. The arguments never have to be constants right? Only the type arguments denoting the native types have to be. The example you give is normal tuple syntax right, so they are both an expression not a constant.

@mraleph
Copy link
Member

mraleph commented Sep 13, 2022

I don't understand. The arguments never have to be constants right? Only the type arguments denoting the native types have to be. The example you give is normal tuple syntax right, so they are both an expression not a constant.

Sorry, I've chosen a wrong word. What I meant to say is that for List based APIs we could require that vararg list is a list literal at the call site this would allow us to emit necessary unboxing operations at the call-site itself (essentially inlining the trampoline). This however requires us to know that we are calling a vararg function - hence reliance on FfiNative (declarative) binding.

The record based invocation has more freedom, because we know number and types of arguments from the record type itself. Though to simplify the implementation we probably still want to outlaw indirect calls to vararg functions and instead require people to use a special helper to invoke such functions indirectly.

@dcharkes
Copy link
Contributor

We can use record types:

/// `int printf(const char *format, ...)` with `int` and `double` as varargs.
typedef NativePrintfIntDouble =
    Int Function(Pointer<Char>, VarArgs<(Int, Double)>);

@dcharkes
Copy link
Contributor

dcharkes commented Sep 29, 2022

Potentially blocked by

We need at least to be able to traverse the types in the VM.

/// `int printf(const char *format, ...)` with `int` and `double` as varargs.
@FfiNative<Int Function(Pointer<Char>, VarArgs<(Int, Double)>)>
external int printf(Pointer<Char>, int, double);
DynamicLibrary d;
final f = d.lookupFunction<Int Function(Pointer<Char>, VarArgs<(Int, Double)>),
                           int Function(Pointer<Char>, int, double)>();

@dcharkes
Copy link
Contributor

Remove support for single positional element records. They don't have any current use and are a syntactic wart. If we later add support for spreading argument lists and single element positional records become useful, we can re-add them then.

https://github.com/dart-lang/language/blob/master/accepted/future-releases/records/records-feature-specification.md

@munificent this forces us to use

class VarArgs<T extends Object> ...

instead of

class VarArgs<T extends Record> ...

Not really a big issue IMO, but it requires some more special casing in the documentation and implementation.

@mkustermann
Copy link
Member

Since records are supported now (if experiments flag is enabled) it seems this issue is unblocked now, right?

One interesting thing to think about is how this would work for bindings generation from .h files: It wouldn't know what specific var-args signature to use when generating method(s) for a vararg method. Either it could generate a lookup of pointer and require users to fp.asFunction or one would need to specify in the configuration.

@dcharkes
Copy link
Contributor

dcharkes commented Jan 4, 2023

Since records are supported now (if experiments flag is enabled) it seems this issue is unblocked now, right?

Correct, and I happen to have implemented this over the holidays: https://dart-review.googlesource.com/c/sdk/+/276921
I still need to clean it up and split it up before sending it out for review.

@munificent
Copy link
Member

We went back and forth on single-field records. They were removed at one point, but we added them back in and the feature does support them now, along with a Record supertype.

@dcharkes
Copy link
Contributor

We went back and forth on single-field records. They were removed at one point, but we added them back in and the feature does support them now, along with a Record supertype.

Do we also support unary record types though?

tests/ffi/function_callbacks_varargs_generated_test.dart:109:63: Error: Expected ')' before this.
typedef VariadicAt1Int64x2Type = Int64 Function(Int64, VarArgs<(Int64)>);
                                                              ^

I do not get similar errors for nullary or multi-field records.

@munificent
Copy link
Member

Do we also support unary record types though?

Yes. To be consistent with unary record expressions (and to potentially allow parenthesized types if we later get more complex type expressions), we require unary record types to also have a trailing comma:

typedef VariadicAt1Int64x2Type = Int64 Function(Int64, VarArgs<(Int64,)>);

(I don't know if the implementations support it yet, but that's the correct syntax.)

copybara-service bot pushed a commit that referenced this issue Jan 20, 2023
On ARM64 macos and ios, when varargs are used, the first vararg blocks
all cpu and fpu registers.

On Windows x64, when varargs are used, floating point arguments are
passed _both_ in the integer and double register.

The Windows logic requires a new kind of native location:
`BothNativeLocations`, which signals that a value needs to be copied
to both locations before an FFI call, and can be copied from any of
the two locations when getting an FFI callback.
TEST=runtime/vm/compiler/ffi/unit_tests/variadic_double/x64_win.expect

Note that integer arguments already block out the corresponding xmm
registers on Windows x64.

On System-V, an upper bound of the number of XMM registers used must
be passed in AL. (Not reflected in the unit tests here, but will be in
the dependent CL.)

On ARM (32 bit), using varargs forces the calling convention to be in
softfp mode even on hardfp supported devices.

On RISC-V, the FPU registers are blocked when using varargs.

TEST=runtime/vm/compiler/ffi/native_calling_convention_test.cc
Test outputs in: runtime/vm/compiler/ffi/unit_tests/variadic_*
Run test with `tools/test.py ffi_unit`.

Bug: #38578
Change-Id: Ic568f8156c1c28ac3d6a2144805edf8caaa0169c
Cq-Include-Trybots: luci.dart.try:vm-precomp-ffi-qemu-linux-release-arm-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/278342
Reviewed-by: Ryan Macnak <rmacnak@google.com>
copybara-service bot pushed a commit that referenced this issue Jan 20, 2023
Bug: #38578
Change-Id: I4b98ac3d23a9e009a15af6cf165481fa80ec2d7d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/278343
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. library-ffi
Projects
None yet
Development

No branches or pull requests

8 participants