From 189c36b82a6abc2c7f36e3198d7e5e7a4596f7bf Mon Sep 17 00:00:00 2001 From: Daco Harkes Date: Fri, 9 Apr 2021 16:45:13 +0000 Subject: [PATCH] [vm] Native effects Adds a way to express native effects in Dart expressions in kernel. This CL adds a `void _nativeEffect(Object)` to `dart:internal`. The semantics of `_nativeEffect` are to not execute its arguments and return `null`. This CL uses this `_nativeEffect` to make sure that we never execute the struct constructor invocations used to simulate the native behavior of FFI trampolines. Closes: https://github.com/dart-lang/sdk/issues/45607 TEST=tests/ffi(_2)/native_effect_test.dart Change-Id: Ie06de145e49f8b1cae9e148c2d5d97d5cd8e6878 Cq-Include-Trybots: luci.dart.try:vm-precomp-ffi-qemu-linux-release-arm-try,vm-ffi-android-debug-arm64-try,vm-kernel-precomp-asan-linux-release-x64-try,vm-kernel-precomp-dwarf-linux-product-x64-try,vm-kernel-precomp-linux-debug-x64-try,vm-kernel-precomp-obfuscate-linux-release-x64-try,analyzer-analysis-server-linux-try,analyzer-linux-release-try,dart-sdk-linux-try,vm-kernel-reload-rollback-linux-debug-x64-try,vm-kernel-reload-linux-debug-x64-try Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/194421 Reviewed-by: Alexander Markov Commit-Queue: Daco Harkes --- pkg/vm/lib/transformations/ffi.dart | 3 + pkg/vm/lib/transformations/ffi_use_sites.dart | 42 ++++++------- .../ffi_struct_constructors.dart.expect | 12 +++- runtime/lib/object.cc | 4 ++ runtime/vm/bootstrap_natives.h | 1 + .../frontend/kernel_binary_flowgraph.cc | 60 +++++++++++++------ .../frontend/kernel_binary_flowgraph.h | 7 ++- runtime/vm/compiler/recognized_methods_list.h | 1 + sdk/lib/_internal/vm/lib/internal_patch.dart | 7 ++- tests/ffi/native_effect_test.dart | 33 ++++++++++ tests/ffi_2/native_effect_test.dart | 33 ++++++++++ 11 files changed, 158 insertions(+), 45 deletions(-) create mode 100644 tests/ffi/native_effect_test.dart create mode 100644 tests/ffi_2/native_effect_test.dart diff --git a/pkg/vm/lib/transformations/ffi.dart b/pkg/vm/lib/transformations/ffi.dart index 84a6dbfc0cc..551ee3abf42 100644 --- a/pkg/vm/lib/transformations/ffi.dart +++ b/pkg/vm/lib/transformations/ffi.dart @@ -203,6 +203,7 @@ class FfiTransformer extends Transformer { final Class listClass; final Class typeClass; final Procedure unsafeCastMethod; + final Procedure nativeEffectMethod; final Class typedDataClass; final Procedure typedDataBufferGetter; final Procedure typedDataOffsetInBytesGetter; @@ -294,6 +295,8 @@ class FfiTransformer extends Transformer { typeClass = coreTypes.typeClass, unsafeCastMethod = index.getTopLevelMember('dart:_internal', 'unsafeCast'), + nativeEffectMethod = + index.getTopLevelMember('dart:_internal', '_nativeEffect'), typedDataClass = index.getClass('dart:typed_data', 'TypedData'), typedDataBufferGetter = index.getMember('dart:typed_data', 'TypedData', 'get:buffer'), diff --git a/pkg/vm/lib/transformations/ffi_use_sites.dart b/pkg/vm/lib/transformations/ffi_use_sites.dart index 1b69f262d10..20cfbe86ea3 100644 --- a/pkg/vm/lib/transformations/ffi_use_sites.dart +++ b/pkg/vm/lib/transformations/ffi_use_sites.dart @@ -387,29 +387,29 @@ class _FfiUseSiteTransformer extends FfiTransformer { } /// Prevents the struct from being tree-shaken in TFA by invoking its - /// constructor in a let expression. - /// - /// TFA does not recognize this as dead code, only the VM does. - /// TODO(http://dartbug.com/45607): Wrap with `_nativeEffect` to make the - /// intent of this code clear. + /// constructor in a `_nativeEffect` expression. Expression _invokeStructConstructor( - Expression nestedExpression, Class structClass) { - final constructor = structClass.constructors + Expression nestedExpression, Class compositeClass) { + final constructor = compositeClass.constructors .firstWhere((c) => c.name == Name("#fromTypedDataBase")); - return Let( - VariableDeclaration.forValue( - ConstructorInvocation( - constructor, - Arguments([ - StaticInvocation( - uint8ListFactory, - Arguments([ - ConstantExpression(IntConstant(1)), - ])) - ..fileOffset = nestedExpression.fileOffset, - ])) - ..fileOffset = nestedExpression.fileOffset, - type: InterfaceType(structClass, Nullability.nonNullable)), + return BlockExpression( + Block([ + ExpressionStatement(StaticInvocation( + nativeEffectMethod, + Arguments([ + ConstructorInvocation( + constructor, + Arguments([ + StaticInvocation( + uint8ListFactory, + Arguments([ + ConstantExpression(IntConstant(1)), + ])) + ..fileOffset = nestedExpression.fileOffset, + ])) + ..fileOffset = nestedExpression.fileOffset + ]))) + ]), nestedExpression) ..fileOffset = nestedExpression.fileOffset; } diff --git a/pkg/vm/testcases/transformations/type_flow/transformer/ffi_struct_constructors.dart.expect b/pkg/vm/testcases/transformations/type_flow/transformer/ffi_struct_constructors.dart.expect index 66de23b8833..34696727137 100644 --- a/pkg/vm/testcases/transformations/type_flow/transformer/ffi_struct_constructors.dart.expect +++ b/pkg/vm/testcases/transformations/type_flow/transformer/ffi_struct_constructors.dart.expect @@ -65,13 +65,17 @@ static method main() → void { } static method testLookupFunctionReturn() → void { final ffi::DynamicLibrary* dylib = [@vm.inferred-type.metadata=dart.ffi::DynamicLibrary?] ffi::DynamicLibrary::executable(); - final () →* self::Struct1* function1 = let final self::Struct1 #t1 = new self::Struct1::#fromTypedDataBase([@vm.inferred-type.metadata=dart.typed_data::_Uint8List] typ::Uint8List::•(#C18)) in ffi::_asFunctionInternal<() →* self::Struct1*, () →* self::Struct1*>([@vm.direct-call.metadata=dart.ffi::DynamicLibrary.lookup??] [@vm.inferred-type.metadata=dart.ffi::Pointer? (skip check)] dylib.{ffi::DynamicLibrary::lookup}*>("function1")); + final () →* self::Struct1* function1 = block { + _in::_nativeEffect(new self::Struct1::#fromTypedDataBase([@vm.inferred-type.metadata=dart.typed_data::_Uint8List] typ::Uint8List::•(#C18))); + } =>ffi::_asFunctionInternal<() →* self::Struct1*, () →* self::Struct1*>([@vm.direct-call.metadata=dart.ffi::DynamicLibrary.lookup??] [@vm.inferred-type.metadata=dart.ffi::Pointer? (skip check)] dylib.{ffi::DynamicLibrary::lookup}*>("function1")); final self::Struct1* struct1 = [@vm.call-site-attributes.metadata=receiverType:#lib::Struct1* Function()*] function1.call(); core::print(struct1); } static method testAsFunctionReturn() → void { final ffi::Pointer*>* pointer = [@vm.inferred-type.metadata=dart.ffi::Pointer?] ffi::Pointer::fromAddress*>(3735928559); - final () →* self::Struct2* function2 = let final self::Struct2 #t2 = new self::Struct2::#fromTypedDataBase([@vm.inferred-type.metadata=dart.typed_data::_Uint8List] typ::Uint8List::•(#C18)) in ffi::_asFunctionInternal<() →* self::Struct2*, () →* self::Struct2*>(pointer); + final () →* self::Struct2* function2 = block { + _in::_nativeEffect(new self::Struct2::#fromTypedDataBase([@vm.inferred-type.metadata=dart.typed_data::_Uint8List] typ::Uint8List::•(#C18))); + } =>ffi::_asFunctionInternal<() →* self::Struct2*, () →* self::Struct2*>(pointer); final self::Struct2* struct2 = [@vm.call-site-attributes.metadata=receiverType:#lib::Struct2* Function()*] function2.call(); core::print(struct2); } @@ -79,7 +83,9 @@ static method testAsFunctionReturn() → void { return 42; } static method testFromFunctionArgument() → void { - final ffi::Pointer*>* pointer = let final self::Struct3 #t3 = new self::Struct3::#fromTypedDataBase([@vm.inferred-type.metadata=dart.typed_data::_Uint8List] typ::Uint8List::•(#C18)) in [@vm.inferred-type.metadata=dart.ffi::Pointer?] self::_#ffiCallback0; + final ffi::Pointer*>* pointer = block { + _in::_nativeEffect(new self::Struct3::#fromTypedDataBase([@vm.inferred-type.metadata=dart.typed_data::_Uint8List] typ::Uint8List::•(#C18))); + } =>[@vm.inferred-type.metadata=dart.ffi::Pointer?] self::_#ffiCallback0; core::print(pointer); } static method testLookupFunctionArgument() → void { diff --git a/runtime/lib/object.cc b/runtime/lib/object.cc index 11b7f6021df..507ce9d11b9 100644 --- a/runtime/lib/object.cc +++ b/runtime/lib/object.cc @@ -279,6 +279,10 @@ DEFINE_NATIVE_ENTRY(Internal_unsafeCast, 0, 1) { return arguments->NativeArgAt(0); } +DEFINE_NATIVE_ENTRY(Internal_nativeEffect, 0, 1) { + UNREACHABLE(); +} + DEFINE_NATIVE_ENTRY(Internal_reachabilityFence, 0, 1) { UNREACHABLE(); } diff --git a/runtime/vm/bootstrap_natives.h b/runtime/vm/bootstrap_natives.h index 31908830e03..fe0f1921dae 100644 --- a/runtime/vm/bootstrap_natives.h +++ b/runtime/vm/bootstrap_natives.h @@ -333,6 +333,7 @@ namespace dart { V(GrowableList_setLength, 2) \ V(GrowableList_setData, 2) \ V(Internal_unsafeCast, 1) \ + V(Internal_nativeEffect, 1) \ V(Internal_reachabilityFence, 1) \ V(Internal_collectAllGarbage, 0) \ V(Internal_makeListFixedLength, 1) \ diff --git a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc index d90c132dc89..9dfcc4e016f 100644 --- a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc +++ b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc @@ -2643,7 +2643,7 @@ Fragment StreamingFlowGraphBuilder::BuildStaticSet(TokenPosition* p) { } Fragment StreamingFlowGraphBuilder::BuildMethodInvocation(TokenPosition* p) { - const intptr_t offset = ReaderOffset() - 1; // Include the tag. + const intptr_t offset = ReaderOffset() - 1; // Include the tag. const uint8_t flags = ReadFlags(); // read flags. const bool is_invariant = (flags & kMethodInvocationFlagInvariant) != 0; @@ -3007,7 +3007,9 @@ Fragment StreamingFlowGraphBuilder::BuildStaticInvocation(TokenPosition* p) { } const auto recognized_kind = target.recognized_kind(); - if (recognized_kind == MethodRecognizer::kFfiAsFunctionInternal) { + if (recognized_kind == MethodRecognizer::kNativeEffect) { + return BuildNativeEffect(); + } else if (recognized_kind == MethodRecognizer::kFfiAsFunctionInternal) { return BuildFfiAsFunctionInternal(); } else if (CompilerState::Current().is_aot() && recognized_kind == MethodRecognizer::kFfiNativeCallbackFunction) { @@ -5018,20 +5020,42 @@ Fragment StreamingFlowGraphBuilder::BuildFunctionNode( return instructions; } +Fragment StreamingFlowGraphBuilder::BuildNativeEffect() { + const intptr_t argc = ReadUInt(); // Read argument count. + ASSERT(argc == 1); // Native side effect to ignore. + const intptr_t list_length = ReadListLength(); // Read types list length. + ASSERT(list_length == 0); + + const intptr_t positional_count = + ReadListLength(); // Read positional argument count. + ASSERT(positional_count == 1); + + BuildExpression(); // Consume expression but don't save the fragment. + Pop(); // Restore the stack. + + const intptr_t named_args_len = + ReadListLength(); // Skip empty named arguments. + ASSERT(named_args_len == 0); + + Fragment code; + code += NullConstant(); // Return type is void. + return code; +} + Fragment StreamingFlowGraphBuilder::BuildFfiAsFunctionInternal() { - const intptr_t argc = ReadUInt(); // read argument count. - ASSERT(argc == 1); // pointer - const intptr_t list_length = ReadListLength(); // read types list length. - ASSERT(list_length == 2); // dart signature, then native signature + const intptr_t argc = ReadUInt(); // Read argument count. + ASSERT(argc == 1); // Pointer. + const intptr_t list_length = ReadListLength(); // Read types list length. + ASSERT(list_length == 2); // Dart signature, then native signature. const TypeArguments& type_arguments = - T.BuildTypeArguments(list_length); // read types. + T.BuildTypeArguments(list_length); // Read types. Fragment code; const intptr_t positional_count = - ReadListLength(); // read positional argument count + ReadListLength(); // Read positional argument count. ASSERT(positional_count == 1); - code += BuildExpression(); // build first positional argument (pointer) + code += BuildExpression(); // Build first positional argument (pointer). const intptr_t named_args_len = - ReadListLength(); // skip (empty) named arguments list + ReadListLength(); // Skip empty named arguments list. ASSERT(named_args_len == 0); code += B->BuildFfiAsFunctionInternalCall(type_arguments); return code; @@ -5044,24 +5068,24 @@ Fragment StreamingFlowGraphBuilder::BuildFfiNativeCallbackFunction() { // // The FE also guarantees that all three arguments are constants. - const intptr_t argc = ReadUInt(); // read argument count - ASSERT(argc == 2); // target, exceptionalReturn + const intptr_t argc = ReadUInt(); // Read argument count. + ASSERT(argc == 2); // Target, exceptionalReturn. - const intptr_t list_length = ReadListLength(); // read types list length - ASSERT(list_length == 1); // native signature + const intptr_t list_length = ReadListLength(); // Read types list length. + ASSERT(list_length == 1); // The native signature. const TypeArguments& type_arguments = - T.BuildTypeArguments(list_length); // read types. + T.BuildTypeArguments(list_length); // Read types. ASSERT(type_arguments.Length() == 1 && type_arguments.IsInstantiated()); const FunctionType& native_sig = FunctionType::CheckedHandle(Z, type_arguments.TypeAt(0)); Fragment code; const intptr_t positional_count = - ReadListLength(); // read positional argument count + ReadListLength(); // Read positional argument count. ASSERT(positional_count == 2); // Read target expression and extract the target function. - code += BuildExpression(); // build first positional argument (target) + code += BuildExpression(); // Build first positional argument (target). Definition* target_def = B->Peek(); ASSERT(target_def->IsConstant()); const Closure& target_closure = @@ -5081,7 +5105,7 @@ Fragment StreamingFlowGraphBuilder::BuildFfiNativeCallbackFunction() { code += Drop(); const intptr_t named_args_len = - ReadListLength(); // skip (empty) named arguments list + ReadListLength(); // Skip (empty) named arguments list. ASSERT(named_args_len == 0); const Function& result = diff --git a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.h b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.h index 7f133f7ab9e..47f5bca615c 100644 --- a/runtime/vm/compiler/frontend/kernel_binary_flowgraph.h +++ b/runtime/vm/compiler/frontend/kernel_binary_flowgraph.h @@ -344,11 +344,14 @@ class StreamingFlowGraphBuilder : public KernelReaderHelper { Fragment BuildFunctionNode(TokenPosition parent_position, StringIndex name_index); - // Build build FG for '_asFunctionInternal'. Reads an Arguments from the + // Build flow graph for '_nativeEffect'. + Fragment BuildNativeEffect(); + + // Build FG for '_asFunctionInternal'. Reads an Arguments from the // Kernel buffer and pushes the resulting closure. Fragment BuildFfiAsFunctionInternal(); - // Build build FG for '_nativeCallbackFunction'. Reads an Arguments from the + // Build FG for '_nativeCallbackFunction'. Reads an Arguments from the // Kernel buffer and pushes the resulting Function object. Fragment BuildFfiNativeCallbackFunction(); diff --git a/runtime/vm/compiler/recognized_methods_list.h b/runtime/vm/compiler/recognized_methods_list.h index 8a2fbbc5533..02acfb71778 100644 --- a/runtime/vm/compiler/recognized_methods_list.h +++ b/runtime/vm/compiler/recognized_methods_list.h @@ -166,6 +166,7 @@ namespace dart { V(::, _abi, FfiAbi, 0x7c4ab775) \ V(::, _asFunctionInternal, FfiAsFunctionInternal, 0xbbcb235a) \ V(::, _nativeCallbackFunction, FfiNativeCallbackFunction, 0x3ff5ae9c) \ + V(::, _nativeEffect, NativeEffect, 0x61e00b59) \ V(::, _loadInt8, FfiLoadInt8, 0x0f04dfd6) \ V(::, _loadInt16, FfiLoadInt16, 0xec44312d) \ V(::, _loadInt32, FfiLoadInt32, 0xee223fc3) \ diff --git a/sdk/lib/_internal/vm/lib/internal_patch.dart b/sdk/lib/_internal/vm/lib/internal_patch.dart index c434ec95e59..7ee1e6b0226 100644 --- a/sdk/lib/_internal/vm/lib/internal_patch.dart +++ b/sdk/lib/_internal/vm/lib/internal_patch.dart @@ -163,11 +163,16 @@ Int32List _growRegExpStack(Int32List stack) { T unsafeCast(Object? v) native "Internal_unsafeCast"; // This function can be used to keep an object alive til that point. -// @pragma("vm:recognized", "other") @pragma('vm:prefer-inline') void reachabilityFence(Object object) native "Internal_reachabilityFence"; +// This function can be used to encode native side effects. +// +// The function call and it's argument are removed in flow graph construction. +@pragma("vm:recognized", "other") +void _nativeEffect(Object object) native "Internal_nativeEffect"; + void sendAndExit(SendPort sendPort, var message) native "SendPortImpl_sendAndExitInternal_"; diff --git a/tests/ffi/native_effect_test.dart b/tests/ffi/native_effect_test.dart new file mode 100644 index 00000000000..41d4aea831d --- /dev/null +++ b/tests/ffi/native_effect_test.dart @@ -0,0 +1,33 @@ +// Copyright (c) 2021, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. +// +// SharedObjects=ffi_test_functions + +// Tests that the dart:internal _nativeEffect flow graph builder works. + +import 'dart:ffi'; + +import "package:expect/expect.dart"; + +import 'dylib_utils.dart'; + +void main() { + testReturnStruct1ByteInt(); +} + +final ffiTestFunctions = dlopenPlatformSpecific("ffi_test_functions"); + +final returnStruct1ByteInt = ffiTestFunctions.lookupFunction< + Struct1ByteInt Function(Int8), + Struct1ByteInt Function(int)>("ReturnStruct1ByteInt"); + +void testReturnStruct1ByteInt() { + final result = returnStruct1ByteInt(1); + Expect.equals(1, result.a0); +} + +class Struct1ByteInt extends Struct { + @Int8() + external int a0; +} diff --git a/tests/ffi_2/native_effect_test.dart b/tests/ffi_2/native_effect_test.dart new file mode 100644 index 00000000000..1b88d14a577 --- /dev/null +++ b/tests/ffi_2/native_effect_test.dart @@ -0,0 +1,33 @@ +// Copyright (c) 2021, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. +// +// SharedObjects=ffi_test_functions + +// Tests that the dart:internal _nativeEffect flow graph builder works. + +import 'dart:ffi'; + +import "package:expect/expect.dart"; + +import 'dylib_utils.dart'; + +void main() { + testReturnStruct1ByteInt(); +} + +final ffiTestFunctions = dlopenPlatformSpecific("ffi_test_functions"); + +final returnStruct1ByteInt = ffiTestFunctions.lookupFunction< + Struct1ByteInt Function(Int8), + Struct1ByteInt Function(int)>("ReturnStruct1ByteInt"); + +void testReturnStruct1ByteInt() { + final result = returnStruct1ByteInt(1); + Expect.equals(1, result.a0); +} + +class Struct1ByteInt extends Struct { + @Int8() + int a0; +}