dart-lang / native

Dart packages related to FFI and native assets bundling.
BSD 3-Clause "New" or "Revised" License
83 stars 27 forks source link

Support size_t, int, and other common C types // platform independent bindings #530

Closed dcharkes closed 2 years ago

dcharkes commented 3 years ago

We should support generating some dart:ffi type for size_t, int, long, etc., when dart:ffi starts supporting this: https://github.com/dart-lang/sdk/issues/36140.

Example

Comparing generated files on Linux x64 and MacOS x64 which uses size_t:

CINDEX_LINKAGE const char *clang_getFileContents(CXTranslationUnit tu,
                                                 CXFile file, size_t *size);
 typedef _c_clang_getFileContents = ffi.Pointer<ffi.Int8> Function(
   ffi.Pointer<CXTranslationUnitImpl> tu,
   ffi.Pointer<ffi.Void> file,
-  ffi.Pointer<ffi.Uint64> size,
+  ffi.Pointer<ffi.Int32> size,
 );

The lack of 'named' integer types that vary in size on different platforms currently makes ffigen only generate correct bindings for its host platform. In order to make ffigen generate platform independent bindings we should address this issue.

dcharkes commented 3 years ago

https://github.com/dart-lang/sdk/issues/36140#issuecomment-648073317

We can map the three 3 types that we already support to the right dart:ffi type.

vaind commented 3 years ago

Why does size_t currently generate as Int32? I understand UintPtr would be ideal but Isn't IntPtr better, being the correct size at least?

mannprerak2 commented 3 years ago

Ffigen will usually disregard the type name, types like Int8_t, Int16_t etc are automatically mapped by default, but not size_t. And because it's a typedef, ffigen uses the underlying type it is (which may be Int32 or Int64, depending on the system it's running on).

If size_t is consistent with IntPtr (I am not sure) then perhaps we can make ffigen use IntPtr for that.

dcharkes commented 3 years ago

This is blocked by https://github.com/dart-lang/sdk/issues/42563. When that is added to dart:ffi we can define types such as size_t and generate code for them.

vaind commented 3 years ago

This is blocked by dart-lang/sdk#42563.

I understand the complete solution is blocked by not having a proper matching type. but I still think changing to IntPtr or a 64-bit type in general (like is done for C bool, which would useffi.Int32) in the meantime would be better. I'd even go as far as calling the current generated code buggy. You can easily verify that e.g. using valgrind on a dart2native compiled dart code. If your C function is, for example sth. like write_size(size_t* out_size), than the current generated code expecting Pointer<Int32> causes write outside of allocated memory:

==101914== Thread 3 DartWorker:
==101914== Invalid write of size 8
==101914==    by 0x59B8A03: write_size (...)
...
==101914==  Address 0x6159250 is 0 bytes inside a block of size 4 alloc'd
==101914==    at 0x483A77F: malloc (vg_replace_malloc.c:307)
...
dcharkes commented 3 years ago

Using IntPtr will make sign extension and truncation do the wrong thing on 32 bit platforms. This will make package:ffigen do the wrong thing for its host platform.

In its current form, package:ffigen works for the host platform, and does not work multi platform. What you are proposing is to break it for the host platform to make it slightly less broken for multi platform.

@mannprerak2, does our current configuration allow users to overwrite what type is generated by matching the name of the C type and then selecting the name for the Dart type to be generated? (If I'm not mistaken, we can only match on name and select a size, correct?) It might be worth implementing this as a workaround for the time being such that users can opt in to that. I'll leave that decision up to you.

The final solution should be multi platform bindings, but it might be some time until we get around to dart-lang/sdk#42563.

mannprerak2 commented 3 years ago

Our current configuration has size-map which allows us to specify a size (from 1, 2, 4, 8) for native types (like short, int, long int). No such thing is available for typedefs type as of now.

Internally, however, we map typedef names (like int8_t, int16_t directly to a type by default (if use-supported-typedefs is not set to false).

So we can just expose this to the users as a config option. Something like typedef-map can be added, it can work similarly to size-map but with the user providing a type name instead of size? (like IntPtr, Int32, etc)

typedef-map:
  'size_t': 'IntPtr`
mannprerak2 commented 3 years ago

@dcharkes @vaind I've added a PR dart-lang/ffigen#119 so that users can temporarily map typedef names to a NativeType (Int(s), Float, Double, and IntPtr).

cmc5788 commented 2 years ago

Just following up on this. For our use case it would be really useful if the Dart code generated by ffigen as platform-agnostic. The workflow for providing different Dart code for each platform we support is awkward.

dcharkes commented 2 years ago

This issue is blocked in dart:ffi support for ABI-specific types. https://github.com/dart-lang/sdk/issues/42563

We'll address it once that is supported.

cmc5788 commented 2 years ago

@dcharkes -- looks like the blocking issue landed, any update on this?

mannprerak2 commented 2 years ago

Yeah, I guess we can start working on this now. Any idea when this will be available on the latest dev channel?

dcharkes commented 2 years ago

When we release a dev build after Version 2.16.0-118.0.dev.

P.S. Feel free to copy https://github.com/dart-lang/sdk/blob/main/tests/ffi/abi_specific_ints.dart into ffigen for code generation until I move them to package:ffi.

mannprerak2 commented 2 years ago

I guess I'll atleast wait for the dev release.

I've been thinking about how to expose this in ffigen. Here's what I came up with -

  1. We introduce library imports to ffigen. By default there can be 2 library imports already available - ffi and pkg_ffi. Users can also specify their own library-imports like this -
library-imports:
    my_lib: 'package:my_app/some/file.dart'
  1. Users can then specify/override an ABI to use for some type/typedef via type-map.

    type-map:
    'wchar_t':
    'lib': 'ffi'
    'type': 'Int32' 
    'unsigned int':
     'lib': 'pkg_ffi'
     'type': 'UintPtr'
  2. With this we remove size-map and deprecate typedef-map from configurations since type-map is more generic.

wdyt @dcharkes ?

dcharkes commented 2 years ago

I like it.

We want to name the library imports to avoid name clashes in generated code I presume?

It would be slightly more convenient for users if we don't prefix the imports, and the list of libraries is just a list and the type-map is a simple map.

library-imports:
  - 'package:my_app/some/file.dart'
type-map:
  'wchar_t' : 'WChar'

But I'm not sure if we can make the prefixes optional. It would work in the type map. But it would not work in the libraries (something cannot have list and map entries in yaml).

library-imports:
  - 'package:my_app/some/file.dart'
  my_other_lib : 'package:my_other_lib/some/file.dart' #invalid yaml?
type-map:
  'wchar_t' : 'WChar'
  'unsigned int':
     'lib': 'my_other_lib'
     'type': 'UintPtr'
mannprerak2 commented 2 years ago

We want to name the library imports to avoid name clashes in generated code I presume?

Primarily yes. But also for the user to specify exactly which symbol to use, in case it exists in multiple imports (not to mention that it benefits us by not having to analyse the library imports for finding the symbols available).

mannprerak2 commented 2 years ago

@cmc5788 checkout ffigen(5.0.0-dev.0), it uses ffi(1.2.0-dev.0) to generate platform specific ints.

simolus3 commented 2 years ago

Now that Dart 2.17 is out, can we use the types from dart:ffi directly? (I was trying to contribute a fix, but most of the generated libraries in test/ only generate correctly on a macOS libc so I'm out of luck)

liamappelbe commented 2 years ago

Now that Dart 2.17 is out, can we use the types from dart:ffi directly? (I was trying to contribute a fix, but most of the generated libraries in test/ only generate correctly on a macOS libc so I'm out of luck)

You mean running test/regen.dart? There's been some debate around whether those generated goldens are a good way of writing tests, and this is a pretty good argument against them. I hadn't considered that they make it hard to contribute code from non-mac devices. Now I'm thinking we should remove them, at least for tests that are platform dependent. I'll file a separate bug about this.

Sunbreak commented 2 years ago

Now that Dart 2.17 is out, can we use the types from dart:ffi directly? (I was trying to contribute a fix, but most of the generated libraries in test/ only generate correctly on a macOS libc so I'm out of luck)

A *unix environment may help, i.e. Linux desktop, WSL 2 on Windows or Multipass