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

[ffi] Support passing and returning composites (structs) by value. #36730

Closed
mraleph opened this issue Apr 24, 2019 · 11 comments
Closed

[ffi] Support passing and returning composites (structs) by value. #36730

mraleph opened this issue Apr 24, 2019 · 11 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi

Comments

@mraleph
Copy link
Member

mraleph commented Apr 24, 2019

I wanted to writing bindings for libclang (so that I can generate bindings from C/C++ header files automatically), but I discovered that libclang API uses structs passed and returned by value.

typedef struct {
  unsigned int_data[4];
  void *ptr_data;
} CXToken;

typedef struct {
  const void *data;
  unsigned private_flags;
} CXString;

CINDEX_LINKAGE CXString clang_getTokenSpelling(CXTranslationUnit, CXToken);

So I can't really bind to this API. There are some interesting design problems to resolve: e.g. when a structure is returned by value who manages the memory of that struct and how do we prevent people from making a mistake of passing address of that struct to some other method.

@mraleph
Copy link
Member Author

mraleph commented Apr 24, 2019

/cc @sjindel-google @dcharkes I think this should probably be part of MVP - but I could not find an issue for this.

@devoncarew devoncarew added the area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. label Apr 24, 2019
@sjindel-google sjindel-google added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. and removed area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. labels Apr 24, 2019
@sjindel-google
Copy link
Contributor

You could bind to the API btw, with some very simple C wrappers.

@dcharkes
Copy link
Contributor

dcharkes commented Jun 7, 2019

Why did we move this back to Flutter MVP?

@dcharkes
Copy link
Contributor

We'll do the api change in MVP #37229, but not the by-value-passing.

@sjindel-google sjindel-google removed their assignment Jun 12, 2019
dart-bot pushed a commit that referenced this issue Feb 5, 2020
Introduces NativeRepresentation and NativeLocation for the compilation of FFI.

NativeRepresentations are able to express all representations (or types) of the native ABIs we bind to with FFI, this is more representations that than that are used in Dart itself.
NativeLocations are able to express all locations of the native ABIs we bind to with FFI, this is more types of locations than that are used for the Dart calling convention.
See the documentation in the respective files.

These NativeLocations and NativeRepresentations are computed by the NativeCallingConvention and consumed by the Marshaller and Assemblers.

This reenginering is required for go/dart-ffi-by-value, hardfp (Arm S and D fpu registers), and iOS 64 bit (non-word-aligned stack arguments).

In addition, by using the NativeRepresentations we also get slightly reduced code size:

* The tracking of sizes is improved, so less sign/zero-extension operations are required.
* UnboxedWidthExtenderInstr is fully removed, the size extension is done inside the native moves, coalescing moves and size extension when possible.
* BitCastInstr is only used when really needed. This reduces code-size on arm32 softfp.

This fixes the iOS arm64 calling convention, manually tested with flutter/flutter#46078 and https://dart-review.googlesource.com/c/sdk/+/131074.

Fixes: #39637
Issue: #36309
Issue: #36730

Change-Id: I8878bc0f314277bab4ca22f417c6295ecc017720
Cq-Include-Trybots: luci.dart.try:vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64-try,app-kernel-linux-debug-x64-try,vm-kernel-linux-debug-ia32-try,vm-kernel-win-debug-x64-try,vm-kernel-win-debug-ia32-try,vm-kernel-precomp-linux-debug-x64-try,vm-dartkb-linux-release-x64-abi-try,vm-kernel-precomp-android-release-arm64-try,vm-kernel-asan-linux-release-x64-try,vm-kernel-linux-release-simarm-try,vm-kernel-linux-release-simarm64-try,vm-kernel-precomp-android-release-arm_x64-try,vm-kernel-precomp-obfuscate-linux-release-x64-try,dart-sdk-linux-try,analyzer-analysis-server-linux-try,analyzer-linux-release-try,front-end-linux-release-x64-try,vm-kernel-precomp-win-release-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/129081
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>
dart-bot pushed a commit that referenced this issue Mar 5, 2020
Issue: #40767
Issue: #36730
Change-Id: I725bb7ffc9325308dbccb668e683fbd9a7d7b565
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/137791
Reviewed-by: Martin Kustermann <kustermann@google.com>
dart-bot pushed a commit that referenced this issue Mar 5, 2020
This regresses Pointer load and store loops because LoadUntagged is never hoisted out of loops. But with support for passing TypedData, in a follow up CL, we cannot assume that only Pointers are passed in the future.

It also changes the FfiCallInstr context to hold the Pointer, rather than the address, as call sites can be unoptimized and we cannot handle an untagged address there.

Issue: #40767
Issue: #36730
Change-Id: Icc716d79eb9eb2b5aac4f03dbf6c622a6825ffdc
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/137793
Reviewed-by: Martin Kustermann <kustermann@google.com>
dart-bot pushed a commit that referenced this issue Mar 5, 2020
This does not make FFI Structs with TypedData yet, but makes the loads/stores support using TypedData.

Issue: #40767
Issue: #36730

Change-Id: I5d03e324c98c79c07955d70db86ab2e2d8dc4ec2
Cq-Include-Trybots: luci.dart.try:vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64-try,app-kernel-linux-debug-x64-try,vm-kernel-linux-debug-ia32-try,vm-kernel-win-debug-x64-try,vm-kernel-win-debug-ia32-try,vm-kernel-precomp-linux-debug-x64-try,vm-dartkb-linux-release-x64-abi-try,vm-kernel-precomp-android-release-arm64-try,vm-kernel-asan-linux-release-x64-try,vm-kernel-linux-release-simarm-try,vm-kernel-linux-release-simarm64-try,vm-kernel-precomp-android-release-arm_x64-try,vm-kernel-precomp-obfuscate-linux-release-x64-try,dart-sdk-linux-try,analyzer-analysis-server-linux-try,analyzer-linux-release-try,front-end-linux-release-x64-try,vm-kernel-precomp-win-release-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/137305
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
@mannprerak2
Copy link

mannprerak2 commented Mar 22, 2020

I noticed something strange
I created a header like this

struct Data
{
    int i;
};

void printData(struct Data);
struct Data createData(int);

But If I write bindings like this (using Pointer instead of passing struct by value)

typedef printData_func = ffi.Void Function(ffi.Pointer<Data>);
typedef PrintData = void Function(ffi.Pointer<Data>);

typedef create_struct_func = ffi.Pointer<Data> Function(ffi.Int32);
typedef CreateStruct = ffi.Pointer<Data> Function(int);
// we can even simply use ffi.Pointer instead of ffi.Pointer<Data>

The bindings are working perfectly (tested on ubuntu 19)
We just can't do ptr.ref to access the Struct.

Complete code can be found here

Can someone shed some light into this? (is it safe to do so), and
If there are any potential memory leak issues involved?

@dcharkes
Copy link
Contributor

The Data struct with a single int field is treated as a single int field in the Linux ABI.

You're storing the value of the int field as the address of the Pointer, most likely ptr.address will give you the value of the int field.

This will not work for structs that are larger than the architecture size (larger than 8 bytes on 64 bit architectures), and this will not work for structs containing floats.

dart-bot pushed a commit that referenced this issue Mar 26, 2020
Required so that we can access its slots in the compiler.

Issue: #36730
Change-Id: Ib14aa73f5cb6b713c682693f95841773b1b4e5d9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/141121
Reviewed-by: Aske Simon Christensen <askesc@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>
dart-bot pushed a commit that referenced this issue Sep 17, 2020
R8 is used for passing the address when returning structs by value
in the native ABI.

Separated out from https://dart-review.googlesource.com/c/sdk/+/140290

Issue: #36730

Change-Id: I35187d35b8cd1b9fe6852faf0cd02a5f51e9f358
Cq-Include-Trybots: luci.dart.try:vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64-try,vm-precomp-ffi-qemu-linux-release-arm-try,vm-kernel-linux-debug-ia32-try,vm-kernel-linux-debug-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/163204
Reviewed-by: Tess Strickland <sstrickl@google.com>
Commit-Queue: Tess Strickland <sstrickl@google.com>
Auto-Submit: Daco Harkes <dacoharkes@google.com>
dart-bot pushed a commit that referenced this issue Oct 12, 2020
Separated out from:
https://dart-review.googlesource.com/c/sdk/+/140290/18.
Issue: #36730.

Change-Id: I71a749ffe946c74033f7f047486f32746e93d79f
Cq-Include-Trybots: luci.dart.try:vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64-try,vm-precomp-ffi-qemu-linux-release-arm-try,vm-kernel-linux-debug-ia32-try,vm-kernel-linux-debug-x64-try,vm-kernel-mac-debug-x64-try,vm-kernel-win-debug-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/166849
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
dart-bot pushed a commit that referenced this issue Oct 16, 2020
Changes the kernel representation of structs in two ways.
1. `_addressOf` field of `Struct` gets type `Object` because it will be
   either `TypedData` or `Pointer` in structs by value CL.
2. Subtypes of `Struct` get a pragma `vm:ffi:struct-fields` which
   contains a const list of the native types of the fields of the struct
   which will be read in the VM to do compute the locations of structs
   in the target ABI.

Split off from https://dart-review.googlesource.com/c/sdk/+/140290/23
to make that CL smaller. That CL will no longer have changes to the
kernel representation of FFI code after this CL lands separately.

These changes are not consumed in the VM in this CL, but they are tested
by the expect files.

Issue: #36730.

Change-Id: I5d3babd5be07f78c6d2bd80bbc1fd492c51bc01f
Cq-Include-Trybots: luci.dart.try:front-end-linux-release-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/167280
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
dart-bot pushed a commit that referenced this issue Oct 26, 2020
The 3 and 7 byte tests in
https://dart-review.googlesource.com/c/sdk/+/168829
had their sizes rounded up to the next power of two due to alignment of
fields inside the structs.

In order to properly exercise behavior on win32, win64 and iOS, this CL
changes those structs to only have uint8 fields to prevent alignment
influencing the sizes.

Old tests are renamed to exercise the size rounding due to alignment.

This also swaps around the two 9byte structs to have consistent naming
with the new 3 and 7-byte tests.

structs_by_value_tests_confguration.dart is the only non-generated file.

The tests are tested in the dependent CL. Tests are ignored in status
file and are un-ignored in dependent CL.

Tests for #36730.

Change-Id: If47cfcaebe56f43d50265f0d6e2749c56fe7b195
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/169101
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Tess Strickland <sstrickl@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
tekknolagi pushed a commit to tekknolagi/dart-assembler that referenced this issue Nov 3, 2020
Introduces NativeRepresentation and NativeLocation for the compilation of FFI.

NativeRepresentations are able to express all representations (or types) of the native ABIs we bind to with FFI, this is more representations that than that are used in Dart itself.
NativeLocations are able to express all locations of the native ABIs we bind to with FFI, this is more types of locations than that are used for the Dart calling convention.
See the documentation in the respective files.

These NativeLocations and NativeRepresentations are computed by the NativeCallingConvention and consumed by the Marshaller and Assemblers.

This reenginering is required for go/dart-ffi-by-value, hardfp (Arm S and D fpu registers), and iOS 64 bit (non-word-aligned stack arguments).

In addition, by using the NativeRepresentations we also get slightly reduced code size:

* The tracking of sizes is improved, so less sign/zero-extension operations are required.
* UnboxedWidthExtenderInstr is fully removed, the size extension is done inside the native moves, coalescing moves and size extension when possible.
* BitCastInstr is only used when really needed. This reduces code-size on arm32 softfp.

This fixes the iOS arm64 calling convention, manually tested with flutter/flutter#46078 and https://dart-review.googlesource.com/c/sdk/+/131074.

Fixes: dart-lang#39637
Issue: dart-lang#36309
Issue: dart-lang#36730

Change-Id: I8878bc0f314277bab4ca22f417c6295ecc017720
Cq-Include-Trybots: luci.dart.try:vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64-try,app-kernel-linux-debug-x64-try,vm-kernel-linux-debug-ia32-try,vm-kernel-win-debug-x64-try,vm-kernel-win-debug-ia32-try,vm-kernel-precomp-linux-debug-x64-try,vm-dartkb-linux-release-x64-abi-try,vm-kernel-precomp-android-release-arm64-try,vm-kernel-asan-linux-release-x64-try,vm-kernel-linux-release-simarm-try,vm-kernel-linux-release-simarm64-try,vm-kernel-precomp-android-release-arm_x64-try,vm-kernel-precomp-obfuscate-linux-release-x64-try,dart-sdk-linux-try,analyzer-analysis-server-linux-try,analyzer-linux-release-try,front-end-linux-release-x64-try,vm-kernel-precomp-win-release-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/129081
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>
tekknolagi pushed a commit to tekknolagi/dart-assembler that referenced this issue Nov 3, 2020
Separated out from:
https://dart-review.googlesource.com/c/sdk/+/140290/18.
Issue: dart-lang#36730.

Change-Id: I71a749ffe946c74033f7f047486f32746e93d79f
Cq-Include-Trybots: luci.dart.try:vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64-try,vm-precomp-ffi-qemu-linux-release-arm-try,vm-kernel-linux-debug-ia32-try,vm-kernel-linux-debug-x64-try,vm-kernel-mac-debug-x64-try,vm-kernel-win-debug-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/166849
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
dart-bot pushed a commit that referenced this issue Nov 5, 2020
Split off from https://dart-review.googlesource.com/c/sdk/+/140290.
We start using the stack after the C call, so RSP needs to be restored.

Issue #36730.

TEST=All existing FFI tests on windows.

Change-Id: Idcaff9007c1b6f42b95288e39470b3d6cca2ef56
Cq-Include-Trybots: luci.dart.try:vm-kernel-win-debug-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/170425
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>
dart-bot pushed a commit that referenced this issue Nov 6, 2020
A smoke test to exercise alignment of structs with various field types
to see how they are aligned on the stack. Only contains small structs
because most ABIs only pass structs less than 16 bytes on the stack.

Especially tests iOS64, where `AlignOf(struct{float32;float32;}) == 4`
but `AlignOf(struct{int32;int32;}) == 8` due to the special treatment
of homogenous floats in the arm64 ABI combined with iOS decision to not
align everything on the stack to at least word size.

Test tested on iOS64 through Flutter.

structs_by_value_tests_configuration.dart is the only non-generated
file.

The tests are tested in the dependent CL. Tests are ignored in status
file and are un-ignored in dependent CL.

Tests for #36730.

Change-Id: I6ec4523208db740b8ea3f8a4ab1e5717f1088467
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/170691
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Auto-Submit: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
dart-bot pushed a commit that referenced this issue Nov 18, 2020
Split off https://dart-review.googlesource.com/c/sdk/+/140290 to make
that CL smaller.

This CL adds support for struct types in the ffi compiler frontend in
`runtime/vm/compiler/ffi/native_type.h`.

The code in this CL is unit tested:

1. From Dart source to `NativeCompoundType` are tested as VM test.
TEST=runtime/vm/compiler/ffi/native_type_vm_test.cc

2. The size and alignments for structs are tested with expect files.
TEST=runtime/vm/compiler/ffi/native_type_test.cc

The code in this CL has been end-to-end tested in the CL it is split off
from. And will be end-to-end tested when that CL also lands.

Issue: #36730

Change-Id: Ia7134e30daf028fea3ed97319ac7f5d0dbcc710f
Cq-Include-Trybots: luci.dart.try:vm-precomp-ffi-qemu-linux-release-arm-try,vm-ffi-android-debug-arm64-try,vm-ffi-android-debug-arm-try,vm-kernel-nnbd-win-debug-x64-try,vm-kernel-mac-debug-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/172648
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Clement Skau <cskau@google.com>
dart-bot pushed a commit that referenced this issue Nov 19, 2020
Split off https://dart-review.googlesource.com/c/sdk/+/140290 to make
that CL smaller.

This CL adds support for passing struct arguments in the native calling
convention calculation in
`runtime/vm/compiler/ffi/native_calling_convention.cc`.

The code in this CL is unit tested with expect files. The unit tests are
designed to cover the majority of corner cases in the ABIs without
resorting to very many unit tests.

TEST=runtime/vm/compiler/ffi/native_calling_convention_test.cc

The code in this CL has been end-to-end tested in the CL it is split off
from. And will be end-to-end tested when that CL also lands.

Issue: #36730

Change-Id: I5b3c3786122c5f856f33181f898fbd8db782cff3
Cq-Include-Trybots: luci.dart.try:vm-precomp-ffi-qemu-linux-release-arm-try,vm-ffi-android-debug-arm64-try,vm-ffi-android-debug-arm-try,vm-kernel-nnbd-win-debug-x64-try,vm-kernel-mac-debug-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/172763
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Clement Skau <cskau@google.com>
@marcofeltmann
Copy link

Is there any documentation how this can be working now?

I'm having a Go library that returns a tuple (double, double) like so:

//export provideCurrentPosition
func provideCurrentPosition() (float64, float64) {
…
}

The go build -buildmode c-shared creates this header file for it:

/* Return type for provideCurrentPosition */
struct provideCurrentPosition_return {
        GoFloat64 r0;
        GoFloat64 r1;
};

extern struct provideCurrentPosition_return provideCurrentPosition();

I cannot modify this behavior (in fact I could since it is FOSS…) and I'm not very interested in cluttering the Go library in favor for the C interop of dart.

@dcharkes
Copy link
Contributor

dcharkes commented Jan 5, 2021

@marcofeltmann this is currently only available on the master branch (which you'll have to build manually) or on the dev channel (starting from 2.12.0-150.0.dev). Edit: we have released 2.12.0-162.0.dev https://dart.dev/tools/sdk/archive.

Next beta and/or stable releases will include this feature.

@TimWhiting
Copy link

TimWhiting commented Jan 10, 2021

Hi, I've been waiting for this feature to wrap some libraries that I've been interested in. It's looking good so far. However, one issue that I'm running into is that the API I'm wrapping returns structs by value, but always takes in those same structs by reference. Is there an easy way to create a pointer and then copy the bytes from a NativeType to the ref of the pointer? Or can I get a pointer to the NativeType itself?

Pointer<T> copyToPointer<T extends NativeType>(T input){
  final p = allocate<T>();
  p.ref = input;
// ^ error here, setter ref is not defined
  return p;
}

I know I can move the fields to the pointer one by one, but this is a huge library, and I don't want to have to do it for every type individually. (I'm using ffigen to generate the bindings, and it is currently over 32000 lines, with more headers that I'll probably eventually need to generate bindings for as well). If this isn't something that can/should be done in the SDK, I can try to submit a pull request to the ffigen repo with some setting to automatically generate functions to create a pointer and copy the fields for each of the structs that are generated.

Edit:
Looking at the utf8 conversion, it would be easier to be able to make a generic function like this if a NativeType could be cast to a Uint8List.

Pointer<T> copyToPointer<T extends NativeType>(T input){
  final p = allocate<T>();
  final pref = p.cast<Int8>().asTypedList(sizeOf<T>());
  pref.setAll(0, input.asInt8List());
  //                   ^ An object of type T has no methods on it (can't cast to anything useful)
  return p;
}

Edit 2:
I might have been trying to use the API wrong, turns out I might not need to do this for all API calls. But I think there is still a chance I might need to do something like this for some of the API.

@dcharkes
Copy link
Contributor

dcharkes commented Jan 11, 2021

However, one issue that I'm running into is that the API I'm wrapping returns structs by value, but always takes in those same structs by reference.

@TimWhiting It would be quite curious if the C API would not be consistent in returning and passing structs by value. If that API returns structs by value, but expects them to be passed in by pointer, it puts the burden of resource management on the user of that API. Is the API public? If it turns out to be a common pattern we might want to try to make the use case simpler. If this is a problem, please open a new issue.

@TimWhiting
Copy link

Yes the API is public:
Here is the C client library and docs
And the C++ API docs

As an example this c++ file
contains the following

// Creating a pointer
node_options_.reset(new rcl_node_options_t);
// Assigning a return by value result to the underlying memory of the pointer
*node_options_ = rcl_node_get_default_options();

Also this file:

InitOptions::InitOptions(const rcl_init_options_t & init_options)
: init_options_(new rcl_init_options_t)
{
// assigning a return by value to a pointer
  *init_options_ = rcl_get_zero_initialized_init_options();
  rcl_ret_t ret = rcl_init_options_copy(&init_options, init_options_.get());
  if (RCL_RET_OK != ret) {
    rclcpp::exceptions::throw_from_rcl_error(ret, "failed to copy rcl init options");
  }
}

I'm not sure how many structs I would need to do this with, but it's common enough for me to wonder how prevalent.
It's a C library that is designed with the intent that other languages should be using it. I don't know why they made the design decision they did, but I assume they knew that most languages would require a c wrapper that gives the language a pointer to the value rather than the actual value and thus parameters tend to be pointers but not return values? For the most part these are long-lived objects.

I can submit a new issue for this, and I'll try to quantify better how common this pattern is at least in this library.

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, and the AOT and JIT backends. library-ffi
Projects
None yet
Development

No branches or pull requests

7 participants