From 3593de9179d1faf8150fc16fb68031f98011cb2b Mon Sep 17 00:00:00 2001 From: Daco Harkes Date: Wed, 17 Feb 2021 11:39:42 +0000 Subject: [PATCH] [vm/ffi] Change `Pointer.elementAt` and `sizeOf` to use static type This CL changes the semantics of `Pointer.elementAt` and `sizeOf` to use the compile-time `T` rather than the runtime `T`. Issue: https://github.com/dart-lang/sdk/issues/38721 TEST=tests/ffi/data_test.dart TEST=tests/ffi/sizeof_test.dart TEST=tests/ffi/structs_test.dart TEST=tests/ffi/vmspecific_static_checks_test.dart Change-Id: Ifb25a4bd66d50a385d3db6dec9213b96dff21722 Cq-Include-Trybots: luci.dart.try:vm-precomp-ffi-qemu-linux-release-arm-try,analyzer-analysis-server-linux-try,analyzer-linux-release-try,analyzer-nnbd-linux-release-try,front-end-linux-release-x64-try,front-end-nnbd-linux-release-x64-try,benchmark-linux-try,dart-sdk-linux-try,pkg-linux-release-try,vm-ffi-android-release-arm-try,vm-ffi-android-release-arm64-try Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/178200 Reviewed-by: Aske Simon Christensen --- pkg/analyzer/lib/error/error.dart | 1 - pkg/analyzer/lib/src/dart/error/ffi_code.dart | 14 --------- .../lib/src/generated/ffi_verifier.dart | 6 ++-- pkg/front_end/lib/src/api_unstable/vm.dart | 1 - .../src/fasta/fasta_codes_cfe_generated.dart | 29 ------------------- pkg/front_end/messages.yaml | 7 ----- pkg/vm/lib/transformations/ffi_use_sites.dart | 27 ++--------------- .../vm/lib/ffi_allocation_patch.dart | 3 +- sdk/lib/_internal/vm/lib/ffi_patch.dart | 23 +++++---------- sdk/lib/ffi/ffi.dart | 11 ++++--- tests/ffi/data_test.dart | 2 +- tests/ffi/vmspecific_static_checks_test.dart | 8 ++--- tests/ffi_2/data_test.dart | 2 +- .../ffi_2/vmspecific_static_checks_test.dart | 8 ++--- 14 files changed, 29 insertions(+), 113 deletions(-) diff --git a/pkg/analyzer/lib/error/error.dart b/pkg/analyzer/lib/error/error.dart index 54cd975df1c..95ce3fd3aa2 100644 --- a/pkg/analyzer/lib/error/error.dart +++ b/pkg/analyzer/lib/error/error.dart @@ -474,7 +474,6 @@ const List errorCodeValues = [ FfiCode.MUST_BE_A_NATIVE_FUNCTION_TYPE, FfiCode.MUST_BE_A_SUBTYPE, FfiCode.NON_CONSTANT_TYPE_ARGUMENT, - FfiCode.NON_CONSTANT_TYPE_ARGUMENT_WARNING, FfiCode.NON_NATIVE_FUNCTION_TYPE_ARGUMENT_TO_POINTER, FfiCode.NON_SIZED_TYPE_ARGUMENT, FfiCode.SUBTYPE_OF_FFI_CLASS_IN_EXTENDS, diff --git a/pkg/analyzer/lib/src/dart/error/ffi_code.dart b/pkg/analyzer/lib/src/dart/error/ffi_code.dart index 1e71193f4eb..bfbb9bda97c 100644 --- a/pkg/analyzer/lib/src/dart/error/ffi_code.dart +++ b/pkg/analyzer/lib/src/dart/error/ffi_code.dart @@ -183,20 +183,6 @@ class FfiCode extends AnalyzerErrorCode { "parameters are not constants.", correction: "Try changing the type argument to be a constant type."); - /** - * Parameters: - * 0: the name of the function, method, or constructor having type arguments - */ - static const FfiCode NON_CONSTANT_TYPE_ARGUMENT_WARNING = FfiCode( - name: 'NON_CONSTANT_TYPE_ARGUMENT_WARNING', - message: - "Support for using non-constant type arguments '{0}' in this FFI API" - " is deprecated and will be removed in the next stable version of " - "Dart. Rewrite the code to ensure that type arguments are compile " - "time constants referring to a valid native type.", - correction: "Try changing the type argument to be a constant type.", - type: ErrorType.HINT); - /** * Parameters: * 0: the type that should be a valid dart:ffi native type. diff --git a/pkg/analyzer/lib/src/generated/ffi_verifier.dart b/pkg/analyzer/lib/src/generated/ffi_verifier.dart index bf59990e764..5f6241e5ece 100644 --- a/pkg/analyzer/lib/src/generated/ffi_verifier.dart +++ b/pkg/analyzer/lib/src/generated/ffi_verifier.dart @@ -525,9 +525,7 @@ class FfiVerifier extends RecursiveAstVisitor { if (!_isValidFfiNativeType(T, allowVoid: true, allowEmptyStruct: true)) { final AstNode errorNode = node; _errorReporter.reportErrorForNode( - FfiCode.NON_CONSTANT_TYPE_ARGUMENT_WARNING, - errorNode, - ['elementAt']); + FfiCode.NON_CONSTANT_TYPE_ARGUMENT, errorNode, ['elementAt']); } } } @@ -703,7 +701,7 @@ class FfiVerifier extends RecursiveAstVisitor { if (!_isValidFfiNativeType(T, allowVoid: true, allowEmptyStruct: true)) { final AstNode errorNode = node; _errorReporter.reportErrorForNode( - FfiCode.NON_CONSTANT_TYPE_ARGUMENT_WARNING, errorNode, ['sizeOf']); + FfiCode.NON_CONSTANT_TYPE_ARGUMENT, errorNode, ['sizeOf']); } } diff --git a/pkg/front_end/lib/src/api_unstable/vm.dart b/pkg/front_end/lib/src/api_unstable/vm.dart index 78acbcfa682..33b9faa8581 100644 --- a/pkg/front_end/lib/src/api_unstable/vm.dart +++ b/pkg/front_end/lib/src/api_unstable/vm.dart @@ -62,7 +62,6 @@ export '../fasta/fasta_codes.dart' templateFfiFieldCyclic, templateFfiFieldInitializer, templateFfiFieldNoAnnotation, - templateFfiNonConstantTypeArgumentWarning, templateFfiNotStatic, templateFfiStructGeneric, templateFfiTypeInvalid, diff --git a/pkg/front_end/lib/src/fasta/fasta_codes_cfe_generated.dart b/pkg/front_end/lib/src/fasta/fasta_codes_cfe_generated.dart index 0d94cfb011b..a145ee79b43 100644 --- a/pkg/front_end/lib/src/fasta/fasta_codes_cfe_generated.dart +++ b/pkg/front_end/lib/src/fasta/fasta_codes_cfe_generated.dart @@ -915,35 +915,6 @@ Message _withArgumentsFfiExpectedNoExceptionalReturn( arguments: {'type': _type}); } -// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE. -const Template - templateFfiNonConstantTypeArgumentWarning = const Template< - Message Function(DartType _type, bool isNonNullableByDefault)>( - messageTemplate: - r"""Support for using non-constant type arguments '#type' in this FFI API is deprecated and will be removed in the next stable version of Dart. Rewrite the code to ensure that type arguments are compile time constants referring to a valid native type.""", - withArguments: _withArgumentsFfiNonConstantTypeArgumentWarning); - -// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE. -const Code - codeFfiNonConstantTypeArgumentWarning = - const Code( - "FfiNonConstantTypeArgumentWarning", - analyzerCodes: ["NON_CONSTANT_TYPE_ARGUMENT_WARNING"], - severity: Severity.info); - -// DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE. -Message _withArgumentsFfiNonConstantTypeArgumentWarning( - DartType _type, bool isNonNullableByDefault) { - TypeLabeler labeler = new TypeLabeler(isNonNullableByDefault); - List typeParts = labeler.labelType(_type); - String type = typeParts.join(); - return new Message(codeFfiNonConstantTypeArgumentWarning, - message: - """Support for using non-constant type arguments '${type}' in this FFI API is deprecated and will be removed in the next stable version of Dart. Rewrite the code to ensure that type arguments are compile time constants referring to a valid native type.""" + - labeler.originMessages, - arguments: {'type': _type}); -} - // DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE. const Template< Message Function( diff --git a/pkg/front_end/messages.yaml b/pkg/front_end/messages.yaml index 7b6082d13a8..5a98676e2e4 100644 --- a/pkg/front_end/messages.yaml +++ b/pkg/front_end/messages.yaml @@ -4242,13 +4242,6 @@ FfiFieldCyclic: #names external: test/ffi_test.dart -FfiNonConstantTypeArgumentWarning: - # Used by dart:ffi - template: "Support for using non-constant type arguments '#type' in this FFI API is deprecated and will be removed in the next stable version of Dart. Rewrite the code to ensure that type arguments are compile time constants referring to a valid native type." - analyzerCode: NON_CONSTANT_TYPE_ARGUMENT_WARNING - severity: INFO - external: test/ffi_test.dart - FfiNotStatic: # Used by dart:ffi template: "#name expects a static function as parameter. dart:ffi only supports calling static Dart functions from native code." diff --git a/pkg/vm/lib/transformations/ffi_use_sites.dart b/pkg/vm/lib/transformations/ffi_use_sites.dart index c03f4b41d2a..154a575e40d 100644 --- a/pkg/vm/lib/transformations/ffi_use_sites.dart +++ b/pkg/vm/lib/transformations/ffi_use_sites.dart @@ -13,7 +13,6 @@ import 'package:front_end/src/api_unstable/vm.dart' templateFfiExpectedExceptionalReturn, templateFfiExpectedNoExceptionalReturn, templateFfiExtendsOrImplementsSealedClass, - templateFfiNonConstantTypeArgumentWarning, templateFfiNotStatic, templateFfiTypeInvalid, templateFfiTypeMismatch; @@ -189,7 +188,7 @@ class _FfiUseSiteTransformer extends FfiTransformer { // TODO(http://dartbug.com/38721): Change this to an error after // package:ffi is no longer using sizeOf generically. if (!isFfiLibrary) { - _warningNativeTypeValid(nativeType, node); + _ensureNativeTypeValid(nativeType, node); } if (nativeType is InterfaceType) { @@ -467,13 +466,7 @@ class _FfiUseSiteTransformer extends FfiTransformer { node.receiver.getStaticType(_staticTypeContext); final DartType nativeType = _pointerTypeGetTypeArg(pointerType); - _warningNativeTypeValid(nativeType, node); - - // TODO(http://dartbug.com/38721): Change this to an error. - if (nativeType is TypeParameterType) { - // Do not rewire generic invocations. - return node; - } + _ensureNativeTypeValid(nativeType, node); Expression inlineSizeOf = _inlineSizeOf(nativeType); if (inlineSizeOf != null) { @@ -541,22 +534,6 @@ class _FfiUseSiteTransformer extends FfiTransformer { } } - void _warningNativeTypeValid(DartType nativeType, Expression node, - {bool allowHandle: false, bool allowStructItself = true}) { - if (!_nativeTypeValid(nativeType, - allowStructs: true, - allowStructItself: allowStructItself, - allowHandle: allowHandle)) { - diagnosticReporter.report( - templateFfiNonConstantTypeArgumentWarning.withArguments( - nativeType, currentLibrary.isNonNullableByDefault), - node.fileOffset, - 1, - node.location.file); - throw _FfiStaticTypeError(); - } - } - void _ensureNoEmptyStructs(DartType nativeType, Expression node) { // Error on structs with no fields. if (nativeType is InterfaceType) { diff --git a/sdk/lib/_internal/vm/lib/ffi_allocation_patch.dart b/sdk/lib/_internal/vm/lib/ffi_allocation_patch.dart index daa428e7532..0edd45ce603 100644 --- a/sdk/lib/_internal/vm/lib/ffi_allocation_patch.dart +++ b/sdk/lib/_internal/vm/lib/ffi_allocation_patch.dart @@ -12,6 +12,7 @@ extension AllocatorAlloc on Allocator { // TODO(http://dartbug.com/39964): Add `alignmentOf()` call. @patch Pointer call([int count = 1]) { - return this.allocate(sizeOf() * count); + // This case should have been rewritten in pre-processing. + throw UnimplementedError("Pointer<$T>"); } } diff --git a/sdk/lib/_internal/vm/lib/ffi_patch.dart b/sdk/lib/_internal/vm/lib/ffi_patch.dart index 4c69e41b072..7b55e4a6b84 100644 --- a/sdk/lib/_internal/vm/lib/ffi_patch.dart +++ b/sdk/lib/_internal/vm/lib/ffi_patch.dart @@ -26,19 +26,10 @@ int get _intPtrSize => (const [8, 4, 4])[_abi()]; @patch int sizeOf() { - // This is not super fast, but it is faster than a runtime entry. - // Hot loops with elementAt().load() do not use this sizeOf, elementAt is - // optimized per NativeType statically to prevent use of sizeOf at runtime. - final int? knownSize = _knownSizes[T]; - if (knownSize != null) return knownSize; - if (T == IntPtr) return _intPtrSize; - if (T == Pointer) return _intPtrSize; - // For structs we fall back to a runtime entry. - return _sizeOf(); + // This case should have been rewritten in pre-processing. + throw UnimplementedError("$T"); } -int _sizeOf() native "Ffi_sizeOf"; - @pragma("vm:recognized", "other") Pointer _fromAddress(int ptr) native "Ffi_fromAddress"; @@ -98,11 +89,13 @@ class Pointer { @pragma("vm:recognized", "other") int get address native "Ffi_address"; - // For statically known types, this is rewired. - // (Method sizeOf is slow, see notes above.) + // For statically known types, this is rewritten. @patch - Pointer elementAt(int index) => - Pointer.fromAddress(address + sizeOf() * index); + Pointer elementAt(int index) { + // This case should have been rewritten in pre-processing. + // Only dynamic invocations are not rewritten in pre-processing. + throw UnsupportedError("Pointer.elementAt cannot be called dynamically."); + } @patch Pointer _offsetBy(int offsetInBytes) => diff --git a/sdk/lib/ffi/ffi.dart b/sdk/lib/ffi/ffi.dart index aef06c1906e..6beed019ff8 100644 --- a/sdk/lib/ffi/ffi.dart +++ b/sdk/lib/ffi/ffi.dart @@ -24,9 +24,7 @@ part "struct.dart"; /// /// Includes padding and alignment of structs. /// -/// Support for invoking this function with non-constant [T] will be removed in -/// the next stable version of Dart and it will become mandatory to invoke it -/// with a compile-time constant [T]. +/// This function must be invoked with a compile-time constant [T]. external int sizeOf(); /// Represents a pointer into the native C memory corresponding to "NULL", e.g. @@ -65,9 +63,10 @@ class Pointer extends NativeType { /// Pointer arithmetic (takes element size into account). /// - /// Support for invoking this method with non-constant [T] will be removed in - /// the next stable version of Dart and it will become mandatory to invoke it - /// with a compile-time constant [T]. + /// This method must be invoked with a compile-time constant [T]. + /// + /// Does not accept dynamic invocations -- where the type of the receiver is + /// [dynamic]. external Pointer elementAt(int index); /// Cast Pointer to a Pointer. diff --git a/tests/ffi/data_test.dart b/tests/ffi/data_test.dart index 1a101249746..4b619dd681c 100644 --- a/tests/ffi/data_test.dart +++ b/tests/ffi/data_test.dart @@ -449,7 +449,7 @@ void testDynamicInvocation() { final int i = p.value; }); Expect.throws(() => p.value = 1); - p.elementAt(5); // Works, but is slow. + Expect.throws(() => p.elementAt(5)); final int addr = p.address; final Pointer p2 = p.cast(); calloc.free(p); diff --git a/tests/ffi/vmspecific_static_checks_test.dart b/tests/ffi/vmspecific_static_checks_test.dart index 832d1ad6807..b88ef03afaa 100644 --- a/tests/ffi/vmspecific_static_checks_test.dart +++ b/tests/ffi/vmspecific_static_checks_test.dart @@ -625,7 +625,7 @@ T genericRef3(Pointer p) => //# 1202: compile-time error void testSizeOfGeneric() { int generic() { int size = sizeOf(); - size = sizeOf(); //# 1300: ok + size = sizeOf(); //# 1300: compile-time error return size; } @@ -634,7 +634,7 @@ void testSizeOfGeneric() { void testSizeOfNativeType() { try { - sizeOf(); //# 1301: ok + sizeOf(); //# 1301: compile-time error } catch (e) { print(e); } @@ -643,7 +643,7 @@ void testSizeOfNativeType() { void testElementAtGeneric() { Pointer generic(Pointer pointer) { Pointer returnValue = pointer; - returnValue = returnValue.elementAt(1); //# 1310: ok + returnValue = returnValue.elementAt(1); //# 1310: compile-time error return returnValue; } @@ -657,6 +657,6 @@ void testElementAtNativeType() { Pointer p = calloc(); p.elementAt(1); Pointer p2 = p; - p2.elementAt(1); //# 1311: ok + p2.elementAt(1); //# 1311: compile-time error calloc.free(p); } diff --git a/tests/ffi_2/data_test.dart b/tests/ffi_2/data_test.dart index f8ac487e36b..448b465a7b5 100644 --- a/tests/ffi_2/data_test.dart +++ b/tests/ffi_2/data_test.dart @@ -449,7 +449,7 @@ void testDynamicInvocation() { final int i = p.value; }); Expect.throws(() => p.value = 1); - p.elementAt(5); // Works, but is slow. + Expect.throws(() => p.elementAt(5)); final int addr = p.address; final Pointer p2 = p.cast(); calloc.free(p); diff --git a/tests/ffi_2/vmspecific_static_checks_test.dart b/tests/ffi_2/vmspecific_static_checks_test.dart index b6a0451686f..079da6ff33f 100644 --- a/tests/ffi_2/vmspecific_static_checks_test.dart +++ b/tests/ffi_2/vmspecific_static_checks_test.dart @@ -623,7 +623,7 @@ T genericRef3(Pointer p) => //# 1202: compile-time error void testSizeOfGeneric() { int generic() { int size = sizeOf(); - size = sizeOf(); //# 1300: ok + size = sizeOf(); //# 1300: compile-time error return size; } @@ -632,7 +632,7 @@ void testSizeOfGeneric() { void testSizeOfNativeType() { try { - sizeOf(); //# 1301: ok + sizeOf(); //# 1301: compile-time error } catch (e) { print(e); } @@ -641,7 +641,7 @@ void testSizeOfNativeType() { void testElementAtGeneric() { Pointer generic(Pointer pointer) { Pointer returnValue = pointer; - returnValue = returnValue.elementAt(1); //# 1310: ok + returnValue = returnValue.elementAt(1); //# 1310: compile-time error return returnValue; } @@ -655,6 +655,6 @@ void testElementAtNativeType() { Pointer p = calloc(); p.elementAt(1); Pointer p2 = p; - p2.elementAt(1); //# 1311: ok + p2.elementAt(1); //# 1311: compile-time error calloc.free(p); }