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

[vm/ffi] Support unions #38491

Closed
dcharkes opened this issue Sep 20, 2019 · 7 comments
Closed

[vm/ffi] Support unions #38491

dcharkes opened this issue Sep 20, 2019 · 7 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-ffi

Comments

@dcharkes
Copy link
Contributor

The FFI currently only supports structs, we should add support for unions as well.

@dcharkes dcharkes added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-ffi labels Sep 20, 2019
@dcharkes dcharkes added this to Flutter MVP in Dart VM FFI via automation Sep 20, 2019
@dcharkes
Copy link
Contributor Author

Untagged unions should be fairly easy to add in a manner similar to structs:

class MyUnion extends Union {
  @Int8()
  int a;

  Pointer<MyStruct> b;

  @Float()
  double c;
}

For them to be useful, we might want to add nested structs (#37271) and inline fixed-size arrays (#35763) first though.

cc @mkustermann

@Hexer10
Copy link

Hexer10 commented Dec 25, 2019

Any update for this feature? Right now I'd like to implement the SendInput function from the winapi, which requires an INPUT array structured as such

typedef struct tagINPUT {
  DWORD type;
  union {
    MOUSEINPUT    mi;
    KEYBDINPUT    ki;
    HARDWAREINPUT hi;
  } DUMMYUNIONNAME;
} INPUT, *PINPUT, *LPINPUT;

and seems like right now this is not possible.

@dcharkes
Copy link
Contributor Author

dcharkes commented Dec 26, 2019

No update yet.

For the example you provided, you would actually need a union nested in a struct, which requires the nested structs/unions feature in addition (#37271).

As workaround, you can use nesting of structs workaround mentioned in that issue, and use manual casting for the different union values. It's not pretty but it should get you going.

class TagInputMouseInput extends Struct {
  @Int32()
  int type;

  @Int32()
  int dx;

  @Int32()
  int dy;

  // rest of the MOUSEINPUT fields
}

class TagInputKeybdInput extends Struct {
  @Int32()
  int type;

  // the KEYBDINPUT fields
}

class TagInputHardwareInput extends //...

Pointer<TagMouseInput> i;
if(i.type == KeybdInput){
  Pointer<TagInputKeybdInput> p = i.cast();
  // use it as keyboard input.
}
else if //...

@dcharkes dcharkes removed this from Flutter MVP in Dart VM FFI Dec 16, 2020
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>
dart-bot pushed a commit that referenced this issue Apr 8, 2021
This Splits the NativeCompoundType into NativeStructType and
NativeUnionType.

The calling conventions themselves did not have to be updated at all,
they rely on the 'queries' on NativeTypes.

Some interesting corner cases in the native calling conventions are:
1. Arm64 and arm hardfp still regard unions as homogeneous floats, even
   if the union members are different sizes.
2. Unions can be larger than their members if their largest member has
   a smaller alignment than a smaller member.
3. When a specific range in a struct contains floating points in one
   member, but integers in another, the integers take precedence.

Bug: #38491

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
These tests are exercised on vm-precomp-ffi-qemu-linux-release-arm-try.

Change-Id: Ib97769457d1ad0fce47601e9e4a3008bd837167f
Cq-Include-Trybots: luci.dart.try:vm-precomp-ffi-qemu-linux-release-arm-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/194422
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Aske Simon Christensen <askesc@google.com>
@dcharkes
Copy link
Contributor Author

dcharkes commented Apr 9, 2021

We've got two options for structuring the similar Struct and Union APIs:

  1. Give them a shared super class Compound extends NativeType.
  2. Have them next to each other and both extends NativeType.

Option 1:

/// The supertype of all FFI compound types.
///
/// FFI struct types should extend [Struct] and FFI union types should extend
/// [Union]. For more information see the documentation on these classes.
abstract class Compound extends NativeType {
  @pragma("vm:entry-point")
  final Object _typedDataBase;

  Compound._() : _typedDataBase = nullptr;

  Compound._fromTypedDataBase(this._typedDataBase);
}

/// The supertype of all FFI union types.
///
/// FFI union types should extend this class and declare fields corresponding
/// to the underlying native union.
///
/// Field declarations in a [Union] subclass declaration are automatically
/// given a setter and getter implementation which accesses the native union's
/// field in memory.
///
/// All field declarations in a [Union] subclass declaration must either have
/// type [int] or [float] and be annotated with a [NativeType] representing the
/// native type, or must be of type [Pointer]. For example:
///
/// ```c
/// typedef union {
///  int a;
///  float b;
///  void* c;
/// } my_union;
/// ```
///
/// ```dart
/// class MyUnion extends Union {
///   @Int32()
///   external int a;
///
///   @Float()
///   external double b;
///
///   external Pointer<Void> c;
/// }
/// ```
///
/// All field declarations in a [Union] subclass declaration must be marked
/// `external`. You cannot create instances of the class, only have it point to
/// existing native memory, so there is no memory in which to store non-native
/// fields. External fields also cannot be initialized by constructors since no
/// Dart object is being created.
///
/// Instances of a subclass of [Union] have reference semantics and are backed
/// by native memory. The may allocated via allocation or loaded from a
/// [Pointer], but cannot be created by a generative constructor.
abstract class Union extends Compound {
  Union() : super._();

  Union._fromTypedDataBase(Object typedDataBase)
      : super._fromTypedDataBase(typedDataBase);
}

Pros:

  • In our error messages we can refer to 'Compound' rather than 'Struct' or 'Union'.
  • In our implementation we can refer to the shared typedDataBase field.

Cons:

  • People might be less familiar with Compound than Struct and Union.
  • Compound is taken from c++ terminology, and includes more than structs and unions, including pointers.
    • The C standard always uses structure or union in the spec.

Note that we can't actually unify StructPointer and UnionPointer extensions:

/// Extension on [Pointer] specialized for the type argument [Struct].
extension StructPointer<T extends Struct> on Pointer<T> {
  /// Creates a reference to access the fields of this struct backed by native
  /// memory at [address].
  ///
  /// The [address] must be aligned according to the struct alignment rules of
  /// the platform.
  ///
  /// This extension method must be invoked with a compile-time constant [T].
  external T get ref;

  /// Creates a reference to access the fields of this struct backed by native
  /// memory at `address + sizeOf<T>() * index`.
  ///
  /// The [address] must be aligned according to the struct alignment rules of
  /// the platform.
  ///
  /// This extension method must be invoked with a compile-time constant [T].
  external T operator [](int index);
}

Users might have used the StructPointer to disambiguate, so making a shared CompoundPointer would be a breaking change.

Option 2:

/// The supertype of all FFI union types.
///
/// FFI union types should extend this class and declare fields corresponding
/// to the underlying native union.
///
/// Field declarations in a [Union] subclass declaration are automatically
/// given a setter and getter implementation which accesses the native union's
/// field in memory.
///
/// All field declarations in a [Union] subclass declaration must either have
/// type [int] or [float] and be annotated with a [NativeType] representing the
/// native type, or must be of type [Pointer]. For example:
///
/// ```c
/// typedef union {
///  int a;
///  float b;
///  void* c;
/// } my_union;
/// ```
///
/// ```dart
/// class MyUnion extends Union {
///   @Int32()
///   external int a;
///
///   @Float()
///   external double b;
///
///   external Pointer<Void> c;
/// }
/// ```
///
/// All field declarations in a [Union] subclass declaration must be marked
/// `external`. You cannot create instances of the class, only have it point to
/// existing native memory, so there is no memory in which to store non-native
/// fields. External fields also cannot be initialized by constructors since no
/// Dart object is being created.
///
/// Instances of a subclass of [Union] have reference semantics and are backed
/// by native memory. The may allocated via allocation or loaded from a
/// [Pointer], but cannot be created by a generative constructor.
abstract class Union extends NativeType {
  @pragma("vm:entry-point")
  final Object _typedDataBase;

  /// Construct a reference to the [nullptr].
  ///
  /// Use [UnionPointer]'s `.ref` to gain references to native memory backed
  /// unions.
  Union() : _typedDataBase = nullptr;

  Union._fromTypedDataBase(this._typedDataBase);
}

Pros:

  • Consistent with C standard naming.
  • People have most likely more familiarity with the terms Struct and Union.

Cons:

  • More verbose error messages (not a shared super type to refer to).
  • More special casing in the implementation (not a super big deal).

Option 2 prototype: https://dart-review.googlesource.com/c/sdk/+/194420/5
(Option 1 prototype only on my local machine.)

Anyone with more arguments for and against? Other options? Any preferences? @lrhn @mkustermann @mraleph

@dcharkes
Copy link
Contributor Author

dcharkes commented Apr 9, 2021

Brief summary of offline discussion @sstrickl @askeksa-google @mraleph:

  • In our API and error messages use 'struct' or 'union' as that's what the C spec does and what the C grammar does.
  • In our implementation have a shared _Compound superclass for conceptual sharing and implementation sharing. (Use 'compound' over 'composite', because composite has a different meaning in the C standard.)

dart-bot pushed a commit that referenced this issue Apr 12, 2021
This CL does not add unions 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.

Also, this CL fixes some tests to not use nullable fields in Structs.

Bug: #38491

Change-Id: Ia972f31475859aa57e012adf39b636919995b183
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/194788
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
dart-bot pushed a commit that referenced this issue Apr 13, 2021
Introduces the class `_Compound extends NativeType`, for sharing between
`Struct` and `Union`.

Does minimal changes to CFE and IL construction. Follow up refactor CLs
rename everything.

Bug: #38491

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

Change-Id: I276ceb9249c20bd331c2f8b6ef64e35acb525e9c
Cq-Include-Trybots: luci.dart.try:vm-precomp-ffi-qemu-linux-release-arm-try,vm-kernel-precomp-linux-debug-x64-try,vm-kernel-reload-linux-debug-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/194765
Reviewed-by: Aske Simon Christensen <askesc@google.com>
dart-bot pushed a commit that referenced this issue Apr 13, 2021
In preparation of having `Union` besides `Struct`, renames all mentions
of `Struct` to `Compound`.

Bug: #38491

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

Change-Id: I47124a95d67c5afc3da9b5f5c08d0be47908f2e0
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/194423
Reviewed-by: Aske Simon Christensen <askesc@google.com>
dart-bot pushed a commit that referenced this issue Apr 13, 2021
In preparation of having `Union` besides `Struct`, renames all mentions
of `Struct` to `Compound`.

Also, changes the type checking to have `allowXYZ` named arguments
always default to false, and remove redundant arguments.

Bug: #38491

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

Change-Id: Ie5f7cf4189dc315f896e3b3933ff0e0580ac540f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/194424
Commit-Queue: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Aske Simon Christensen <askesc@google.com>
@alexlapa
Copy link

@dcharkes ,

I'm getting a strange compilation error when using unions on current Flutter master channel, which is:

Flutter is already up to date on channel master
Flutter 2.3.0-2.0.pre.116 • channel master •
Framework • revision b7dd9acbed (54 minutes ago) • 2021-05-11 13:49:02 -0400
Engine • revision 5f9eb39f7d
Tools • Dart 2.14.0 (build 2.14.0-74.0.dev)
Error log
Crash when compiling null,
at character offset null:
RangeError (index): Invalid value: Not in inclusive range 0..2: 3
#0      List.[] (dart:core-patch/array.dart:110:52)
#1      _FfiDefinitionTransformer._replaceFields.<anonymous closure> (package:vm/transformations/ffi_definitions.dart:546:70)
#2      MapMixin.map (dart:collection/maps.dart:170:28)
#3      _FfiDefinitionTransformer._replaceFields (package:vm/transformations/ffi_definitions.dart:546:12)
#4      _FfiDefinitionTransformer.visitClassInTopologicalOrder (package:vm/transformations/ffi_definitions.dart:198:28)
#5      _FfiDefinitionTransformer.manualVisitInTopologicalOrder.<anonymous closure> (package:vm/transformations/ffi_definitions.dart:158:9)
#6      List.forEach (dart:core-patch/growable_array.dart:403:8)
#7      _FfiDefinitionTransformer.manualVisitInTopologicalOrder (package:vm/transformations/ffi_definitions.dart:134:25)
#8      transformLibraries (package:vm/transformations/ffi_definitions.dart:89:15)
#9      VmTarget.performModularTransformationsOnLibraries (package:vm/target/vm.dart:162:34)
#10     KernelTarget.runBuildTransformations (package:front_end/src/fasta/kernel/kernel_target.dart:1236:19)
#11     KernelTarget.buildComponent.<anonymous closure> (package:front_end/src/fasta/kernel/kernel_target.dart:372:7)
<asynchronous suspension>
#12     withCrashReporting (package:front_end/src/fasta/crash.dart:121:12)
<asynchronous suspension>
#13     generateKernelInternal.<anonymous closure> (package:front_end/src/kernel_generator_impl.dart:164:19)
<asynchronous suspension>
#14     withCrashReporting (package:front_end/src/fasta/crash.dart:121:12)
<asynchronous suspension>
#15     generateKernel.<anonymous closure> (package:front_end/src/kernel_generator_impl.dart:53:12)
<asynchronous suspension>
#16     generateKernel (package:front_end/src/kernel_generator_impl.dart:52:10)
<asynchronous suspension>
#17     kernelForModule (package:front_end/src/api_prototype/kernel_generator.dart:99:11)
<asynchronous suspension>
#18     SingleShotCompilerWrapper.compileInternal (file:///b/s/w/ir/cache/builder/src/third_party/dart/pkg/vm/bin/kernel_service.dart:404:11)
<asynchronous suspension>
#19     Compiler.compile.<anonymous closure> (file:///b/s/w/ir/cache/builder/src/third_party/dart/pkg/vm/bin/kernel_service.dart:218:45)
<asynchronous suspension>
#20     _processLoadRequest (file:///b/s/w/ir/cache/builder/src/third_party/dart/pkg/vm/bin/kernel_service.dart:885:37)
<asynchronous suspension>


#0      List.[] (dart:core-patch/array.dart:110:52)
#1      _FfiDefinitionTransformer._replaceFields.<anonymous closure> (package:vm/transformations/ffi_definitions.dart:546:70)
#2      MapMixin.map (dart:collection/maps.dart:170:28)
#3      _FfiDefinitionTransformer._replaceFields (package:vm/transformations/ffi_definitions.dart:546:12)
#4      _FfiDefinitionTransformer.visitClassInTopologicalOrder (package:vm/transformations/ffi_definitions.dart:198:28)
#5      _FfiDefinitionTransformer.manualVisitInTopologicalOrder.<anonymous closure> (package:vm/transformations/ffi_definitions.dart:158:9)
#6      List.forEach (dart:core-patch/growable_array.dart:403:8)
#7      _FfiDefinitionTransformer.manualVisitInTopologicalOrder (package:vm/transformations/ffi_definitions.dart:134:25)
#8      transformLibraries (package:vm/transformations/ffi_definitions.dart:89:15)
#9      VmTarget.performModularTransformationsOnLibraries (package:vm/target/vm.dart:162:34)
#10     KernelTarget.runBuildTransformations (package:front_end/src/fasta/kernel/kernel_target.dart:1236:19)
#11     KernelTarget.buildComponent.<anonymous closure> (package:front_end/src/fasta/kernel/kernel_target.dart:372:7)
<asynchronous suspension>
#12     withCrashReporting (package:front_end/src/fasta/crash.dart:121:12)
<asynchronous suspension>
#13     generateKernelInternal.<anonymous closure> (package:front_end/src/kernel_generator_impl.dart:164:19)
<asynchronous suspension>
#14     withCrashReporting (package:front_end/src/fasta/crash.dart:121:12)
<asynchronous suspension>
#15     generateKernel.<anonymous closure> (package:front_end/src/kernel_generator_impl.dart:53:12)
<asynchronous suspension>
#16     generateKernel (package:front_end/src/kernel_generator_impl.dart:52:10)
<asynchronous suspension>
#17     kernelForModule (package:front_end/src/api_prototype/kernel_generator.dart:99:11)
<asynchronous suspension>
#18     SingleShotCompilerWrapper.compileInternal (file:///b/s/w/ir/cache/builder/src/third_party/dart/pkg/vm/bin/kernel_service.dart:404:11)
<asynchronous suspension>
#19     Compiler.compile.<anonymous closure> (file:///b/s/w/ir/cache/builder/src/third_party/dart/pkg/vm/bin/kernel_service.dart:218:45)
<asynchronous suspension>
#20     _processLoadRequest (file:///b/s/w/ir/cache/builder/src/third_party/dart/pkg/vm/bin/kernel_service.dart:885:37)
<asynchronous suspension>

This error happens whenever my union has more than three fields. E.g. :

class DartValueFields extends Union {
  Pointer p1;
  Pointer p2;
  Pointer p3;
  Pointer p4;
}

Is this a known limitation or should i open an issue in dart-lang/ffi?

@dcharkes
Copy link
Contributor Author

dcharkes commented May 12, 2021

@alexlapa bugs in dart:ffi can be reported here. Bugs with package:ffi can be reported in dart-lang/ffi. I've opened a new issue to investigate this. Thanks for reporting!

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
Projects
None yet
Development

No branches or pull requests

3 participants