From 40f7a11d89242f9246f538b6d5a8af5da97dbc72 Mon Sep 17 00:00:00 2001 From: Daco Harkes Date: Mon, 18 May 2020 12:22:50 +0000 Subject: [PATCH] [vm/ffi] NNBD use external fields for structs This enables adding the `external` keyword to struct fields, which enables use in nnbd (weak mode). Closes: https://github.com/dart-lang/sdk/issues/40247 External fields are not implemented in the analyzer yet: https://github.com/dart-lang/sdk/issues/41940 Change-Id: I9d88acbdabf73ca63a6ad3d549930aa3c97cb53f Cq-Include-Trybots: luci.dart.try: analyzer-nnbd-linux-release-try,dart2js-nnbd-linux-x64-chrome-try,ddc-nnbd-linux-release-chrome-try,front-end-nnbd-linux-release-x64-try,vm-kernel-nnbd-linux-debug-x64-try,vm-kernel-nnbd-linux-release-x64-try,vm-kernel-precomp-nnbd-linux-release-x64-try Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/148242 Commit-Queue: Daco Harkes Reviewed-by: Alexander Markov --- .../lib/transformations/ffi_definitions.dart | 221 ++++++++++++------ tests/ffi/coordinate.dart | 6 +- tests/ffi/coordinate_bare.dart | 6 +- tests/ffi/extension_methods_test.dart | 2 +- tests/ffi/very_large_struct.dart | 30 +-- tests/ffi/vmspecific_static_checks_test.dart | 20 +- 6 files changed, 184 insertions(+), 101 deletions(-) diff --git a/pkg/vm/lib/transformations/ffi_definitions.dart b/pkg/vm/lib/transformations/ffi_definitions.dart index cecc5a02bb5..1c11b716d3a 100644 --- a/pkg/vm/lib/transformations/ffi_definitions.dart +++ b/pkg/vm/lib/transformations/ffi_definitions.dart @@ -156,9 +156,9 @@ class _FfiDefinitionTransformer extends FfiTransformer { } } - bool _isPointerType(Field field) { + bool _isPointerType(DartType type) { return env.isSubtypeOf( - field.type, + type, InterfaceType(pointerClass, Nullability.legacy, [ InterfaceType(nativeTypesClasses[NativeType.kNativeType.index], Nullability.legacy) @@ -166,18 +166,52 @@ class _FfiDefinitionTransformer extends FfiTransformer { SubtypeCheckMode.ignoringNullabilities); } + /// Returns members of [node] that correspond to struct fields. + /// + /// Note that getters and setters that originate from an external field have + /// the same `fileOffset`, we always returns getters first. + List _structFieldMembers(Class node) { + final externalGetterSetters = [...node.procedures] + ..retainWhere((p) => p.isExternal && (p.isGetter || p.isSetter)); + final structMembers = [...node.fields, ...externalGetterSetters] + ..sort((m1, m2) { + if (m1.fileOffset == m2.fileOffset) { + // Getter and setter have same offset, getter comes first. + return (m1 as Procedure).isGetter ? -1 : 1; + } + return m1.fileOffset - m2.fileOffset; + }); + return structMembers; + } + + DartType _structFieldMemberType(Member member) { + if (member is Field) { + return member.type; + } + final Procedure p = member; + if (p.isGetter) { + return p.function.returnType; + } + return p.function.positionalParameters.single.type; + } + bool _checkFieldAnnotations(Class node) { bool success = true; - for (final Field f in node.fields) { - if (f.initializer is! NullLiteral) { - diagnosticReporter.report( - templateFfiFieldInitializer.withArguments(f.name.name), - f.fileOffset, - f.name.name.length, - f.fileUri); + final membersWithAnnotations = _structFieldMembers(node) + ..retainWhere((m) => (m is Field) || (m is Procedure && m.isGetter)); + for (final Member f in membersWithAnnotations) { + if (f is Field) { + if (f.initializer is! NullLiteral) { + diagnosticReporter.report( + templateFfiFieldInitializer.withArguments(f.name.name), + f.fileOffset, + f.name.name.length, + f.fileUri); + } } final nativeTypeAnnos = _getNativeTypeAnnotations(f).toList(); - if (_isPointerType(f)) { + final type = _structFieldMemberType(f); + if (_isPointerType(type)) { if (nativeTypeAnnos.length != 0) { diagnosticReporter.report( templateFfiFieldNoAnnotation.withArguments(f.name.name), @@ -192,7 +226,6 @@ class _FfiDefinitionTransformer extends FfiTransformer { f.name.name.length, f.fileUri); } else { - final DartType dartType = f.type; final DartType nativeType = InterfaceType( nativeTypesClasses[nativeTypeAnnos.first.index], Nullability.legacy); @@ -200,10 +233,10 @@ class _FfiDefinitionTransformer extends FfiTransformer { final DartType shouldBeDartType = convertNativeTypeToDartType(nativeType, /*allowStructs=*/ false); if (shouldBeDartType == null || - !env.isSubtypeOf(dartType, shouldBeDartType, + !env.isSubtypeOf(type, shouldBeDartType, SubtypeCheckMode.ignoringNullabilities)) { diagnosticReporter.report( - templateFfiTypeMismatch.withArguments(dartType, shouldBeDartType, + templateFfiTypeMismatch.withArguments(type, shouldBeDartType, nativeType, node.enclosingLibrary.isNonNullableByDefault), f.fileOffset, 1, @@ -261,19 +294,40 @@ class _FfiDefinitionTransformer extends FfiTransformer { /// /// Returns the total size of the struct (for all ABIs). Map _replaceFields(Class node, IndexedClass indexedClass) { - final fields = []; final types = []; + final fields = {}; + final getters = {}; + final setters = {}; - for (final Field f in node.fields) { - if (_isPointerType(f)) { - fields.add(f); - types.add(NativeType.kPointer); + int i = 0; + for (final Member m in _structFieldMembers(node)) { + final dartType = _structFieldMemberType(m); + + NativeType nativeType; + if (_isPointerType(dartType)) { + nativeType = NativeType.kPointer; } else { - final nativeTypeAnnos = _getNativeTypeAnnotations(f).toList(); + final nativeTypeAnnos = _getNativeTypeAnnotations(m).toList(); if (nativeTypeAnnos.length == 1) { - final NativeType t = nativeTypeAnnos.first; - fields.add(f); - types.add(t); + nativeType = nativeTypeAnnos.first; + } + } + + if ((m is Field || (m is Procedure && m.isGetter)) && + nativeType != null) { + types.add(nativeType); + if (m is Field) { + fields[i] = m; + } + if (m is Procedure) { + getters[i] = m; + } + i++; + } + if (m is Procedure && m.isSetter) { + final index = i - 1; // The corresponding getter's index. + if (getters.containsKey(index)) { + setters[i - 1] = m; } } } @@ -283,7 +337,7 @@ class _FfiDefinitionTransformer extends FfiTransformer { sizeAndOffsets[abi] = _calculateSizeAndOffsets(types, abi); } - for (int i = 0; i < fields.length; i++) { + for (final i in fields.keys) { final fieldOffsets = sizeAndOffsets .map((Abi abi, SizeAndOffsets v) => MapEntry(abi, v.offsets[i])); final methods = _generateMethodsForField( @@ -291,10 +345,35 @@ class _FfiDefinitionTransformer extends FfiTransformer { methods.forEach((p) => node.addMember(p)); } - for (final Field f in fields) { + for (final Field f in fields.values) { f.remove(); } + for (final i in getters.keys) { + final fieldOffsets = sizeAndOffsets + .map((Abi abi, SizeAndOffsets v) => MapEntry(abi, v.offsets[i])); + Procedure getter = getters[i]; + getter.function.body = _generateGetterStatement( + getter.function.returnType, + types[i], + getter.fileOffset, + fieldOffsets); + getter.isExternal = false; + } + + for (final i in setters.keys) { + final fieldOffsets = sizeAndOffsets + .map((Abi abi, SizeAndOffsets v) => MapEntry(abi, v.offsets[i])); + Procedure setter = setters[i]; + setter.function.body = _generateSetterStatement( + setter.function.positionalParameters.single.type, + types[i], + setter.fileOffset, + fieldOffsets, + setter.function.positionalParameters.single); + setter.isExternal = false; + } + return sizeAndOffsets.map((k, v) => MapEntry(k, v.size)); } @@ -315,14 +394,9 @@ class _FfiDefinitionTransformer extends FfiTransformer { listElementAt); } - List _generateMethodsForField(Field field, NativeType type, - Map offsets, IndexedClass indexedClass) { - final DartType nativeType = type == NativeType.kPointer - ? field.type - : InterfaceType(nativeTypesClasses[type.index], Nullability.legacy); - final Class nativeClass = (nativeType as InterfaceType).classNode; - final NativeType nt = getType(nativeClass); - final bool isPointer = nt == NativeType.kPointer; + Statement _generateGetterStatement(DartType dartType, NativeType type, + int fileOffset, Map offsets) { + final bool isPointer = type == NativeType.kPointer; // Sample output: // int get x => _loadInt8(pointer, offset); @@ -332,31 +406,27 @@ class _FfiDefinitionTransformer extends FfiTransformer { // _fromAddress(_loadIntPtr(pointer, offset)); final loadMethod = isPointer ? loadMethods[NativeType.kIntptr] - : optimizedTypes.contains(nt) ? loadMethods[nt] : loadStructMethod; + : optimizedTypes.contains(type) ? loadMethods[type] : loadStructMethod; Expression getterReturnValue = StaticInvocation( loadMethod, Arguments([ PropertyGet(ThisExpression(), addressOfField.name, addressOfField) - ..fileOffset = field.fileOffset, + ..fileOffset = fileOffset, _runtimeBranchOnLayout(offsets) ])) - ..fileOffset = field.fileOffset; + ..fileOffset = fileOffset; if (isPointer) { - final typeArg = (nativeType as InterfaceType).typeArguments.single; + final typeArg = (dartType as InterfaceType).typeArguments.single; getterReturnValue = StaticInvocation( fromAddressInternal, Arguments([getterReturnValue], types: [typeArg])) - ..fileOffset = field.fileOffset; + ..fileOffset = fileOffset; } - final Procedure getter = Procedure( - field.name, - ProcedureKind.Getter, - FunctionNode(ReturnStatement(getterReturnValue), - returnType: field.type), - fileUri: field.fileUri, - reference: - indexedClass?.lookupProcedureNotSetter(field.name.name)?.reference) - ..fileOffset = field.fileOffset - ..isNonNullableByDefault = field.isNonNullableByDefault; + return ReturnStatement(getterReturnValue); + } + + Statement _generateSetterStatement(DartType dartType, NativeType type, + int fileOffset, Map offsets, VariableDeclaration argument) { + final bool isPointer = type == NativeType.kPointer; // Sample output: // set x(int v) => _storeInt8(pointer, offset, v); @@ -365,35 +435,48 @@ class _FfiDefinitionTransformer extends FfiTransformer { // set x(Pointer v) => // _storeIntPtr(pointer, offset, (v as Pointer).address); final storeMethod = - isPointer ? storeMethods[NativeType.kIntptr] : storeMethods[nt]; + isPointer ? storeMethods[NativeType.kIntptr] : storeMethods[type]; + Expression argumentExpression = VariableGet(argument) + ..fileOffset = fileOffset; + if (isPointer) { + argumentExpression = DirectPropertyGet(argumentExpression, addressGetter) + ..fileOffset = fileOffset; + } + return ReturnStatement(StaticInvocation( + storeMethod, + Arguments([ + PropertyGet(ThisExpression(), addressOfField.name, addressOfField) + ..fileOffset = fileOffset, + _runtimeBranchOnLayout(offsets), + argumentExpression + ])) + ..fileOffset = fileOffset); + } + + List _generateMethodsForField(Field field, NativeType type, + Map offsets, IndexedClass indexedClass) { + final getterStatement = + _generateGetterStatement(field.type, type, field.fileOffset, offsets); + final Procedure getter = Procedure(field.name, ProcedureKind.Getter, + FunctionNode(getterStatement, returnType: field.type), + fileUri: field.fileUri, + reference: + indexedClass?.lookupProcedureNotSetter(field.name.name)?.reference) + ..fileOffset = field.fileOffset + ..isNonNullableByDefault = field.isNonNullableByDefault; + Procedure setter = null; if (!field.isFinal) { final VariableDeclaration argument = VariableDeclaration('#v', type: field.type) ..fileOffset = field.fileOffset; - Expression argumentExpression = VariableGet(argument) - ..fileOffset = field.fileOffset; - if (isPointer) { - argumentExpression = - DirectPropertyGet(argumentExpression, addressGetter) - ..fileOffset = field.fileOffset; - } + final setterStatement = _generateSetterStatement( + field.type, type, field.fileOffset, offsets, argument); setter = Procedure( field.name, ProcedureKind.Setter, - FunctionNode( - ReturnStatement(StaticInvocation( - storeMethod, - Arguments([ - PropertyGet( - ThisExpression(), addressOfField.name, addressOfField) - ..fileOffset = field.fileOffset, - _runtimeBranchOnLayout(offsets), - argumentExpression - ])) - ..fileOffset = field.fileOffset), - returnType: VoidType(), - positionalParameters: [argument]), + FunctionNode(setterStatement, + returnType: VoidType(), positionalParameters: [argument]), fileUri: field.fileUri, reference: indexedClass?.lookupProcedureSetter(field.name.name)?.reference) @@ -485,7 +568,7 @@ class _FfiDefinitionTransformer extends FfiTransformer { return fieldType; } - Iterable _getNativeTypeAnnotations(Field node) { + Iterable _getNativeTypeAnnotations(Member node) { return node.annotations .whereType() .map((expr) => expr.constant) diff --git a/tests/ffi/coordinate.dart b/tests/ffi/coordinate.dart index 92dd9d4599c..950731296fe 100644 --- a/tests/ffi/coordinate.dart +++ b/tests/ffi/coordinate.dart @@ -10,12 +10,12 @@ import "package:ffi/ffi.dart"; /// Sample struct for dart:ffi library. class Coordinate extends Struct { @Double() - double x; + external double x; @Double() - double y; + external double y; - Pointer next; + external Pointer next; factory Coordinate.allocate(double x, double y, Pointer next) { return allocate().ref diff --git a/tests/ffi/coordinate_bare.dart b/tests/ffi/coordinate_bare.dart index daaea24f84c..09509b73248 100644 --- a/tests/ffi/coordinate_bare.dart +++ b/tests/ffi/coordinate_bare.dart @@ -9,10 +9,10 @@ import 'dart:ffi'; /// Stripped down sample struct for dart:ffi library. class Coordinate extends Struct { @Double() - double x; + external double x; @Double() - double y; + external double y; - Pointer next; + external Pointer next; } diff --git a/tests/ffi/extension_methods_test.dart b/tests/ffi/extension_methods_test.dart index 19bc44b4c70..d4e8e4af35f 100644 --- a/tests/ffi/extension_methods_test.dart +++ b/tests/ffi/extension_methods_test.dart @@ -61,5 +61,5 @@ testReifiedGeneric() { class Foo extends Struct { @Int8() - int a; + external int a; } diff --git a/tests/ffi/very_large_struct.dart b/tests/ffi/very_large_struct.dart index ca29143b6c5..d827e4aeac5 100644 --- a/tests/ffi/very_large_struct.dart +++ b/tests/ffi/very_large_struct.dart @@ -7,45 +7,45 @@ import 'dart:ffi'; /// Large sample struct for dart:ffi library. class VeryLargeStruct extends Struct { @Int8() - int a; + external int a; @Int16() - int b; + external int b; @Int32() - int c; + external int c; @Int64() - int d; + external int d; @Uint8() - int e; + external int e; @Uint16() - int f; + external int f; @Uint32() - int g; + external int g; @Uint64() - int h; + external int h; @IntPtr() - int i; + external int i; @Double() - double j; + external double j; @Float() - double k; + external double k; - Pointer parent; + external Pointer parent; @IntPtr() - int numChildren; + external int numChildren; - Pointer children; + external Pointer children; @Int8() - int smallLastField; + external int smallLastField; } diff --git a/tests/ffi/vmspecific_static_checks_test.dart b/tests/ffi/vmspecific_static_checks_test.dart index a0af90f065d..f6236e8ff5f 100644 --- a/tests/ffi/vmspecific_static_checks_test.dart +++ b/tests/ffi/vmspecific_static_checks_test.dart @@ -309,9 +309,9 @@ void testNativeFunctionSignatureInvalidOptionalPositional() { // error on missing field annotation class TestStruct extends Struct { @Double() - double x; + external double x; - double y; //# 50: compile-time error + external double y; //# 50: compile-time error } // Cannot extend structs. @@ -321,25 +321,25 @@ class TestStruct3 extends TestStruct {} //# 52: compile-time error class TestStruct4 extends Struct { @Double() @Double() //# 53: compile-time error - double z; + external double z; } // error on annotation not matching up class TestStruct5 extends Struct { @Int64() //# 54: compile-time error - double z; //# 54: compile-time error + external double z; //# 54: compile-time error } // error on annotation not matching up class TestStruct6 extends Struct { @Void() //# 55: compile-time error - double z; //# 55: compile-time error + external double z; //# 55: compile-time error } // error on annotation not matching up class TestStruct7 extends Struct { @NativeType() //# 56: compile-time error - double z; //# 56: compile-time error + external double z; //# 56: compile-time error } // error on field initializer on field @@ -350,8 +350,8 @@ class TestStruct8 extends Struct { // error on field initializer in constructor class TestStruct9 extends Struct { - @Double() - double z; + @Double() //# 58: compile-time error + double z; //# 58: compile-time error TestStruct9() : z = 0.0 {} //# 58: compile-time error } @@ -364,7 +364,7 @@ class TestStruct11 extends //# 60: compile-time error // annotation). class TestStruct12 extends Struct { @Pointer //# 61: compile-time error - TestStruct9 struct; //# 61: compile-time error + external TestStruct9 struct; //# 61: compile-time error } class DummyAnnotation { @@ -375,7 +375,7 @@ class DummyAnnotation { class TestStruct13 extends Struct { @DummyAnnotation() @Double() - double z; + external double z; } // Cannot extend native types.