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] Inline arrays in Structs #35763

Closed
dcharkes opened this issue Jan 25, 2019 · 18 comments
Closed

[dart:ffi] Inline arrays in Structs #35763

dcharkes opened this issue Jan 25, 2019 · 18 comments
Assignees
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-ffi P2 A bug or feature request we're likely to work on

Comments

@dcharkes
Copy link
Contributor

Helpers for converting between ffi.Pointers and arrays.

Maybe use external typed data for efficient conversions.

@dcharkes dcharkes added this to the Dart VM FFI Flutter MVP milestone Jan 25, 2019
@dcharkes dcharkes self-assigned this Jan 25, 2019
@dcharkes dcharkes added this to To do in Dart VM FFI via automation Jan 25, 2019
@kevmoo kevmoo added the area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. label Jan 25, 2019
@dcharkes dcharkes moved this from 1.0 to Flutter MVP in Dart VM FFI Feb 26, 2019
@mit-mit mit-mit removed this from the Dart VM FFI Flutter MVP milestone Mar 11, 2019
@dcharkes dcharkes moved this from Flutter MVP to 1.0 in Dart VM FFI Mar 11, 2019
@djpnewton
Copy link
Contributor

also support for fixed size arrays.

its quite common to have a struct with fixed sized arrays to pass around in C

@dcharkes dcharkes removed their assignment Jul 19, 2019
@dcharkes
Copy link
Contributor Author

dcharkes commented Oct 25, 2019

We have asTypedList for exposing the native memory as a Dart array.

For structs with inline arrays:

struct MyStruct {
  uint8_t arr[10];
  int64_t arr2[3];
};

We need a dart syntax of representing an inline array.

We could opt for exposing this as a pointer, which provides [] and []= access.

class MyStruct extends Struct {
  @InlineArray(10)
  Pointer<Uint8> arr;

  @InlineArray(3)
  Pointer<Int64> arr2;
}

Using a Pointer as the representing type does not allow us to distinguish pointers and inline arrays by the Dart type. However, this is not really a problem since C does not support passing fixed-size arrays by value in calls.

cc @mkustermann

edit: We do not want to expose it as a pointer, rather as a CArray which represents a fixed-size inlined array.

class MS extends Struct {}

MS foo(MS ms); // C signature for passing struct by value.
@size(4)CArray<MS> foo(@size(4)CArray<MS> mss); // C signature for passing inline array of structs.
Pointer<MS> foo(Pointer<MS> ms); // C signature for passing pointer.

class MS2 extends Struct {
  MS ms; // Nested struct.
}
class MS3 extends Struct {
  @size(4)
  CArray<MS> mss; // Nested array of structs.
}
class MS4 extends Struct {
  Pointer<MS> mss; // Pointer to struct.
}

CArray would be a view on top of the underlying pointer (kind of like struct).

If we expose the nested structs and the inline array as getter/setter pair, the setters would do a memcopy of the whole underlying structure.

(As sidenote, we should also introduce memcopy on structs:

Pointer<MS> p1, p2;
p1.value = p2.ref; // Copies over all memory.

)

@kakyoism
Copy link

kakyoism commented Mar 30, 2020

@dcharkes

I got this error when trying your solution to getting static arrays on Dart side

Code

class StaticArray extends Struct {
  @InlineArray(10)
  Pointer<Int32> array;
  @Int32()
  int len;
}

Result

$ dart structs.dart 
structs.dart:48:4: Error: Method not found: 'InlineArray'.
  @InlineArray(10)
   ^^^^^^^^^^^
structs.dart:129:15: Error: A function expression can't have a name.
  staticArray.forEach(idx, elem) => print("array[$idx]: $elem");
              ^^^^^^^
structs.dart:129:15: Error: Expected an identifier, but got 'forEach'.
  staticArray.forEach(idx, elem) => print("array[$idx]: $elem");
              ^^^^^^^

Does it mean that this is just a proposal?
What would be the canonical way of getting array data from FFI C pointers?
I'm referring to my latest question.

Thanks in advance!

@dcharkes
Copy link
Contributor Author

dcharkes commented Mar 30, 2020

Yes, for now it's just a proposal.

See the workaround made by @timsneath here for now.

I'll get around to implementing this in the future, I'm working on other dart:ffi features at the moment.

@kakyoism
Copy link

kakyoism commented Mar 31, 2020

@dcharkes Thanks! FFI has been great so far.
On a related matter, the struct example never frees the malloc'ated memory. Does this mean that the memory will be GC'ed on Dart side?

This would affect how I use the array from a returned pointer.

@timsneath
Copy link
Contributor

You need to use free() to release the memory. 
it'll happen automatically when the process exits, of course. 

@kakyoism
Copy link

kakyoism commented Mar 31, 2020

@timsneath Thanks. I looked at the static array workaround but it seems difficult to scale when my array is say 500-1000 elements big.

Is it that the realistic workaround is to always use malloc'ated arrays?

@artob
Copy link

artob commented May 21, 2020

See the workaround made by @timsneath here for now.

I'll get around to implementing this in the future, I'm working on other dart:ffi features at the moment.

@dcharkes As I alluded to in the conversation in #36140, the lack of support for inline arrays in structs is a blocker for me in creating OpenXR bindings for Dart and Flutter.

The proposed workaround is infeasible for the structs I need to map, which include the following:

struct XrActionCreateInfo {
  XrStructureType type;
  const void* next;
  char actionName[64];
  XrActionType actionType;
  uint32_t countSubactionPaths;
  const XrPath* subactionPaths;
  char localizedActionName[128];
};

struct XrApplicationInfo {
  char applicationName[128];
  uint32_t applicationVersion;
  char engineName[128];
  uint32_t engineVersion;
  XrVersion apiVersion;
};

struct XrEventDataBuffer {
  XrStructureType type;
  const void* next;
  uint8_t varying[4000];
};

@bitsydarel
Copy link

bitsydarel commented May 21, 2020

As a workaround, would not creating a pointer to uint8 or utf8 with size matching the required, good enough?

For example:
class XrEventDataBuffer extends Struct {
....
Pointer varying;

factory XrEventDataBuffer.create(..., Uint8List varying) {
     final Pointer<Uint8> data = allocate<Uint8>(count: varying.length);
     // since pointer is indexable you can just
     // do a for loop and fill the pointer
     fillpointerWithIntlist(data, varying);
     
    // then initialize the struct
 }

}

Sorry for missing the auto formatting, I’m on the phone.

@dcharkes
Copy link
Contributor Author

What about multi dimensional inline arrays, what should the API be for multidimensional arrays?
(ty @mannprerak2!)

@timsneath
Copy link
Contributor

@artob extending a little from the idea @bitsydarel approached, you might check out https://github.com/timsneath/win32/blob/bluetooth/lib/src/structs.dart#L1941 for an example of wrapping a similar Win32 struct that seems like a viable workaround for you. (In this scenario, I have a 250 character Utf-16 string that I need to wrap).

@artob
Copy link

artob commented Jul 5, 2020

@artob extending a little from the idea @bitsydarel approached, you might check out [...] an example of wrapping a similar Win32 struct that seems like a viable workaround for you.

@timsneath It's a good workaround for the BLUETOOTH_DEVICE_INFO struct you have there, but it won't quite suffice for my use case: the OpenXR API has large character arrays inline and intermixed with other struct fields, not only as the last trailing field in the struct.

So, the suggested approach seems like it could be a workaround for XrEventDataBuffer with its 4000-byte trailing array, but defining XrActionCreateInfo and XrApplicationInfo would still be a pain in the butt given the 64/128-byte inline arrays. (And there are more structs like these.) Any suggests for the likes of those?

As a workaround, would not creating a pointer to uint8 or utf8 with size matching the required, good enough?

@bitsydarel After @timsneath's code example I understand better what you were proposing for XrEventDataBuffer. Thanks, seems a viable approach for that struct 👍

@timsneath
Copy link
Contributor

It's not the prettiest code I've ever written, but wouldn't something like this work?
https://gist.github.com/timsneath/983764458690f89a5c8231b4ff2e2e64

Here we use the same technique as for the BLUETOOTH_DEVICE_INFO struct of using properties to index into the created struct, and treating as the struct just as a binary blob for the purposes of allocating space in the heap.

You could extend this still further by providing setters where appropriate, and adding clean String properties to convert from a C-style string into a Dart string, with guards to ensure a 128-byte maxlength.

Curious to know if this works for you?

@dcharkes
Copy link
Contributor Author

For inline arrays of primitive values https://pub.dev/packages/ffigen has an option to generate 'n' fields and provide indexed access (with array-workaround: true).

@dcharkes dcharkes removed this from 1.0 in Dart VM FFI Dec 16, 2020
@mit-mit mit-mit added this to the January Beta Release (2.12) milestone Jan 7, 2021
@franklinyow franklinyow added the P2 A bug or feature request we're likely to work on label Jan 12, 2021
dart-bot pushed a commit that referenced this issue Feb 9, 2021
This CL does not add inline arrays to `dart:ffi` yet, only to the mock
SDK in the analyzer for testing the analyzer. This means we can review
and land it separately.

Closes: #44747
Bug: #35763

Change-Id: I7df6a61ea4cfa522afd12194dd2f3573eb79b3ef
Cq-Include-Trybots: luci.dart.try:analyzer-analysis-server-linux-try,analyzer-linux-release-try,analyzer-nnbd-linux-release-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/183684
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
dart-bot pushed a commit that referenced this issue Feb 9, 2021
So that it can be invoked from `CArray` which has a backing store
that is either TypedData or Pointer.

Bug: #35763

TEST=tests/ffi(_2)/*

Change-Id: I30bb0c1e848f2ac4f47919009106d5428ed66453
Cq-Include-Trybots: luci.dart.try:vm-kernel-linux-debug-x64-try,vm-kernel-precomp-nnbd-linux-debug-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/183683
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Aske Simon Christensen <askesc@google.com>
dart-bot pushed a commit that referenced this issue Feb 10, 2021
The various ABIs lay out structs with inline arrays the same way as
they do with only nested structs and primitive values.
Most notably, homogenous structs (arm and arm64) and structs spread
over a CPU and FPU register in x64 Linux/MacOS will even be laid out
this way if irregular size nested arrays are involved.
These cases are covered in the unit tests.

This CL introduces the ByteRange to ease the ContainsOnlyFloats
calculation for x64.

Bug: #35763

tools/build.py run_ffi_unit_tests && tools/test.py ffi_unit
TEST=runtime/vm/compiler/ffi/native_calling_convention_test.cc
TEST=runtime/vm/compiler/ffi/native_type_test.cc

tools/test.py ffi ffi_2
TEST=tests/ffi(_2)/(.*)by_value_(*.)_test.dart

Change-Id: I4bbcbffd47eb8901a87db64e62aa5cbe67d03e18
Cq-Include-Trybots: luci.dart.try:vm-precomp-ffi-qemu-linux-release-arm-try,vm-kernel-win-debug-ia32-try,vm-kernel-win-debug-x64-try,vm-kernel-precomp-nnbd-linux-debug-x64-try,vm-kernel-linux-debug-ia32-try,vm-kernel-mac-debug-x64-try,vm-ffi-android-debug-arm64-try,vm-ffi-android-debug-arm-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/183682
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Clement Skau <cskau@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
dart-bot pushed a commit that referenced this issue Feb 11, 2021
This CL changes `@pragma('vm:ffi:struct-fields', [...])` to
`@pragma('vm:ffi:struct-fields', _FfiStructLayout([...]))` which makes
it easier to add more data in subsequent CLs.

Extends `FindPragma` to allow returning multiple matched pragma's, so
that we can filter them. (In this case to avoid matching user-defined
pragma's that do not have an instance of the private class.)

Separated out from https://dart-review.googlesource.com/c/sdk/+/183640
because of the extra constant in existing expectation files.

Bug: #35763
Bug: #38158

TEST=tests/ffi(_2)/*_by_value_*_test.dart

Change-Id: Idef9f82e9b53c2a32dffabcec19669eae550fe2f
Cq-Include-Trybots: luci.dart.try:front-end-nnbd-mac-release-x64-try,front-end-linux-release-x64-try,vm-precomp-ffi-qemu-linux-release-arm-try,vm-kernel-nnbd-linux-debug-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/184181
Reviewed-by: Martin Kustermann <kustermann@google.com>
Reviewed-by: Clement Skau <cskau@google.com>
dart-bot pushed a commit that referenced this issue Feb 11, 2021
Introduces the `NativeTypeCfe` type hierarchy for cleaner calculation
of sizes and offsets of native types and cleaner code generation.

The transformation ensures we're only visiting the structs in
topological order, which means the struct type can always look up its
(indirectly) nested structs in the `structCache`.

This simplifies adding inline arrays
(https://dart-review.googlesource.com/c/sdk/+/183640), packed structs,
and unions to this transformation.

`typedDataBaseOffset` is moved to the `FfiTransformer` because the
dependent CL uses it in the `FfiUseSiteTransformer`.

Bug: #35763
Bug: #38158
Bug: #38491

TEST=tests/ffi(_2)/(.*)by_value(.*)_test.dart

Change-Id: I345e02c48844ca795f9137a5addd5ba89992e1c9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/184421
Reviewed-by: Dmitry Stefantsov <dmitryas@google.com>
Reviewed-by: Clement Skau <cskau@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>
@dcharkes
Copy link
Contributor Author

dcharkes commented Feb 16, 2021

Update^2 design:

class MyStruct extends Struct {
  @Array(8)
  external Array<Uint8> inlineArray;

  @Array(2, 2, 2)
  external Array<Uint8> threeDimensionalInlineArray;

  @Array.multi([2, 2, 2, 2, 2, 2, 2, 2])
  external Array<Uint8> eightDimensionalInlineArray;
}

source #45023 (comment)

@dcharkes
Copy link
Contributor Author

This will be available on the first dev-release after Version 2.13.0-73.0.dev.

@Sunbreak
Copy link

Is it possible to have the address of the Array<Int8>, so that we could convert it asTypedList?

@dcharkes
Copy link
Contributor Author

Is it possible to have the address of the Array<Int8>, so that we could convert it asTypedList?

We're tracking this in #45508.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-ffi P2 A bug or feature request we're likely to work on
Projects
None yet
Development

No branches or pull requests