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

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

Closed
dcharkes opened this issue Jun 19, 2020 · 20 comments · Fixed by dart-archive/ffigen#261
Labels
package:ffigen waiting-on-dart-ffi Issues which are blocked by lacking suppport in dart:ffi

Comments

@dcharkes
Copy link
Collaborator

dcharkes commented Jun 19, 2020

We should support generating some dart:ffi type for size_t, int, long, etc., when dart:ffi starts supporting this: dart-lang/sdk#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
Copy link
Collaborator Author

dart-lang/sdk#36140 (comment)

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

@dcharkes dcharkes added the waiting-on-dart-ffi Issues which are blocked by lacking suppport in dart:ffi label Aug 11, 2020
@dcharkes dcharkes changed the title Support size_t, int, and other common C types Support size_t, int, and other common C types // platform independent bindings Aug 24, 2020
@vaind
Copy link
Contributor

vaind commented Nov 9, 2020

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
Copy link
Contributor

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
Copy link
Collaborator Author

dcharkes commented Nov 9, 2020

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

@vaind
Copy link
Contributor

vaind commented Nov 10, 2020

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
Copy link
Collaborator Author

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
Copy link
Contributor

mannprerak2 commented Nov 10, 2020

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
Copy link
Contributor

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

@cmc5788
Copy link

cmc5788 commented Oct 27, 2021

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
Copy link
Collaborator Author

This issue is blocked in dart:ffi support for ABI-specific types. dart-lang/sdk#42563

We'll address it once that is supported.

@cmc5788
Copy link

cmc5788 commented Dec 17, 2021

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

@mannprerak2
Copy link
Contributor

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

@dcharkes
Copy link
Collaborator Author

dcharkes commented Dec 18, 2021

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
Copy link
Contributor

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'
  1. With this we remove size-map and deprecate typedef-map from configurations since type-map is more generic.

wdyt @dcharkes ?

@dcharkes
Copy link
Collaborator Author

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
Copy link
Contributor

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
Copy link
Contributor

mannprerak2 commented Jan 22, 2022

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

@simolus3
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:ffigen waiting-on-dart-ffi Issues which are blocked by lacking suppport in dart:ffi
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants