From f21b7cafbcc6fe8ce29bb1ebbbc33b175d9cc1f4 Mon Sep 17 00:00:00 2001 From: Daco Harkes Date: Sat, 4 Sep 2021 07:22:03 +0000 Subject: [PATCH] [vm/ffi] Adds param number in trampoline null error Before: `NoSuchMethodError: The method 'FfiTrampoline' was called on null.` After: `Invalid argument(s): argument value for ':ffi_param2' is null`. Makes the ArgumentNullError RTE lookup the name of the argument in the code source map when reporting a null argument. Makes the FFI call arguments and FFI callbacks use kArgumentError instead of the default kNoSuchMethod so that we target this RTE instead. This changes the Error type from `NoSuchMethodError` to `ArgumentError`. Because `Error`s should not be caught [1], this is fine. Since FFI trampolines are created from type arguments, the arguments do not have names. The arguments are assigned names programmatically. See the related bug. Also, this CL cleans up the SourcePosition of the `CheckNullOptimized`, it was never passed. [1] https://dart.dev/guides/language/effective-dart/usage#dont-explicitly-catch-error-or-types-that-implement-it TEST=tests/ffi/function_test.dart Closes: https://github.com/dart-lang/sdk/issues/47094 Bug: https://github.com/dart-lang/sdk/issues/36780 Change-Id: I15e7de4d026e034bde0eda3ba7fe3785f0da5057 Cq-Include-Trybots: luci.dart.try:vm-precomp-ffi-qemu-linux-release-arm-try,vm-ffi-android-debug-arm-try,vm-kernel-precomp-dwarf-linux-product-x64-try,vm-kernel-precomp-linux-debug-x64-try,app-kernel-linux-debug-x64-try,vm-kernel-reload-rollback-linux-debug-x64-try,vm-kernel-reload-linux-debug-x64-try,vm-ffi-android-debug-arm64-try,vm-kernel-nnbd-mac-debug-x64-try,vm-kernel-precomp-nnbd-linux-debug-x64-try Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/212462 Commit-Queue: Daco Harkes Reviewed-by: Clement Skau Reviewed-by: Tess Strickland --- runtime/vm/compiler/backend/il.h | 6 +-- .../frontend/base_flow_graph_builder.cc | 11 +++-- .../frontend/base_flow_graph_builder.h | 15 ++++-- runtime/vm/compiler/frontend/kernel_to_il.cc | 46 ++++++++----------- runtime/vm/runtime_entry.cc | 28 ++++++++--- tests/ffi/function_test.dart | 14 ++++++ tests/ffi_2/function_test.dart | 14 ++++++ 7 files changed, 86 insertions(+), 48 deletions(-) diff --git a/runtime/vm/compiler/backend/il.h b/runtime/vm/compiler/backend/il.h index b4b8664d78c..f28d7419997 100644 --- a/runtime/vm/compiler/backend/il.h +++ b/runtime/vm/compiler/backend/il.h @@ -778,8 +778,7 @@ class Instruction : public ZoneAllocated { // the inlining ID to that; instead, treat it as unset. explicit Instruction(const InstructionSource& source, intptr_t deopt_id = DeoptId::kNone) - : deopt_id_(deopt_id), - inlining_id_(source.inlining_id) {} + : deopt_id_(deopt_id), inlining_id_(source.inlining_id) {} explicit Instruction(intptr_t deopt_id = DeoptId::kNone) : Instruction(InstructionSource(), deopt_id) {} @@ -1508,7 +1507,6 @@ class BlockEntryInstr : public Instruction { DEFINE_INSTRUCTION_TYPE_CHECK(BlockEntry) - protected: BlockEntryInstr(intptr_t block_id, intptr_t try_index, @@ -5301,7 +5299,6 @@ class DebugStepCheckInstr : public TemplateInstruction<0, NoThrow> { virtual bool HasUnknownSideEffects() const { return true; } virtual Instruction* Canonicalize(FlowGraph* flow_graph); - private: const TokenPosition token_pos_; const UntaggedPcDescriptors::Kind stub_kind_; @@ -8803,7 +8800,6 @@ class CheckNullInstr : public TemplateDefinition<1, Throws, Pure> { virtual Value* RedefinedValue() const; - PRINT_OPERANDS_TO_SUPPORT private: diff --git a/runtime/vm/compiler/frontend/base_flow_graph_builder.cc b/runtime/vm/compiler/frontend/base_flow_graph_builder.cc index e4666e27595..a34ae49794e 100644 --- a/runtime/vm/compiler/frontend/base_flow_graph_builder.cc +++ b/runtime/vm/compiler/frontend/base_flow_graph_builder.cc @@ -1054,11 +1054,14 @@ Fragment BaseFlowGraphBuilder::CheckNull(TokenPosition position, return instructions; } -Fragment BaseFlowGraphBuilder::CheckNullOptimized(TokenPosition position, - const String& function_name) { +Fragment BaseFlowGraphBuilder::CheckNullOptimized( + const String& function_name, + CheckNullInstr::ExceptionType exception_type, + TokenPosition position) { Value* value = Pop(); - CheckNullInstr* check_null = new (Z) CheckNullInstr( - value, function_name, GetNextDeoptId(), InstructionSource(position)); + CheckNullInstr* check_null = + new (Z) CheckNullInstr(value, function_name, GetNextDeoptId(), + InstructionSource(position), exception_type); Push(check_null); // Use the redefinition. return Fragment(check_null); } diff --git a/runtime/vm/compiler/frontend/base_flow_graph_builder.h b/runtime/vm/compiler/frontend/base_flow_graph_builder.h index 6a8b1b3cee1..93875e09200 100644 --- a/runtime/vm/compiler/frontend/base_flow_graph_builder.h +++ b/runtime/vm/compiler/frontend/base_flow_graph_builder.h @@ -384,13 +384,20 @@ class BaseFlowGraphBuilder { // Pops the top of the stack, checks it for null, and pushes the result on // the stack to create a data dependency. - // 'function_name' is a selector which is being called (reported in - // NoSuchMethod message). + // // Note that the result can currently only be used in optimized code, because // optimized code uses FlowGraph::RemoveRedefinitions to remove the // redefinitions, while unoptimized code does not. - Fragment CheckNullOptimized(TokenPosition position, - const String& function_name); + Fragment CheckNullOptimized( + const String& name, + CheckNullInstr::ExceptionType exception_type, + TokenPosition position = TokenPosition::kNoSource); + Fragment CheckNullOptimized( + const String& function_name, + TokenPosition position = TokenPosition::kNoSource) { + return CheckNullOptimized(function_name, CheckNullInstr::kNoSuchMethod, + position); + } // Records extra unchecked entry point 'unchecked_entry' in 'graph_entry'. void RecordUncheckedEntryPoint(GraphEntryInstr* graph_entry, diff --git a/runtime/vm/compiler/frontend/kernel_to_il.cc b/runtime/vm/compiler/frontend/kernel_to_il.cc index 115caf4e1cc..ad2f1038316 100644 --- a/runtime/vm/compiler/frontend/kernel_to_il.cc +++ b/runtime/vm/compiler/frontend/kernel_to_il.cc @@ -1280,12 +1280,10 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfRecognizedMethod( body += LoadLocal(parsed_function_->RawParameterVariable(0)); // decoder body += LoadLocal(parsed_function_->RawParameterVariable(1)); // bytes body += LoadLocal(parsed_function_->RawParameterVariable(2)); // start - body += CheckNullOptimized(TokenPosition::kNoSource, - String::ZoneHandle(Z, function.name())); + body += CheckNullOptimized(String::ZoneHandle(Z, function.name())); body += UnboxTruncate(kUnboxedIntPtr); body += LoadLocal(parsed_function_->RawParameterVariable(3)); // end - body += CheckNullOptimized(TokenPosition::kNoSource, - String::ZoneHandle(Z, function.name())); + body += CheckNullOptimized(String::ZoneHandle(Z, function.name())); body += UnboxTruncate(kUnboxedIntPtr); body += LoadLocal(parsed_function_->RawParameterVariable(4)); // table body += Utf8Scan(); @@ -1327,13 +1325,11 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfRecognizedMethod( LocalVariable* arg_offset = parsed_function_->RawParameterVariable(1); body += LoadLocal(arg_offset); - body += CheckNullOptimized(TokenPosition::kNoSource, - String::ZoneHandle(Z, function.name())); + body += CheckNullOptimized(String::ZoneHandle(Z, function.name())); LocalVariable* arg_offset_not_null = MakeTemporary(); body += LoadLocal(arg_pointer); - body += CheckNullOptimized(TokenPosition::kNoSource, - String::ZoneHandle(Z, function.name())); + body += CheckNullOptimized(String::ZoneHandle(Z, function.name())); // No GC from here til LoadIndexed. body += LoadUntagged(compiler::target::PointerBase::data_field_offset()); body += LoadLocal(arg_offset_not_null); @@ -1430,8 +1426,7 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfRecognizedMethod( // Pointer> as argument, and (2) the bound on the pointer // type parameter guarantees X is an interface type. body += LoadLocal(arg_pointer); - body += CheckNullOptimized(TokenPosition::kNoSource, - String::ZoneHandle(Z, function.name())); + body += CheckNullOptimized(String::ZoneHandle(Z, function.name())); body += LoadNativeField( Slot::GetTypeArgumentsSlotFor(thread_, pointer_class)); body += NullConstant(); // function_type_args. @@ -1441,17 +1436,14 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfRecognizedMethod( ASSERT_EQUAL(function.NumParameters(), 3); body += LoadLocal(arg_offset); - body += CheckNullOptimized(TokenPosition::kNoSource, - String::ZoneHandle(Z, function.name())); + body += CheckNullOptimized(String::ZoneHandle(Z, function.name())); LocalVariable* arg_offset_not_null = MakeTemporary(); body += LoadLocal(arg_value); - body += CheckNullOptimized(TokenPosition::kNoSource, - String::ZoneHandle(Z, function.name())); + body += CheckNullOptimized(String::ZoneHandle(Z, function.name())); LocalVariable* arg_value_not_null = MakeTemporary(); body += LoadLocal(arg_pointer); // Pointer. - body += CheckNullOptimized(TokenPosition::kNoSource, - String::ZoneHandle(Z, function.name())); + body += CheckNullOptimized(String::ZoneHandle(Z, function.name())); // No GC from here til StoreIndexed. body += LoadUntagged(compiler::target::PointerBase::data_field_offset()); body += LoadLocal(arg_offset_not_null); @@ -1489,16 +1481,14 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfRecognizedMethod( body += AllocateObject(TokenPosition::kNoSource, pointer_class, 1); body += LoadLocal(MakeTemporary()); // Duplicate Pointer. body += LoadLocal(parsed_function_->RawParameterVariable(0)); // Address. - body += CheckNullOptimized(TokenPosition::kNoSource, - String::ZoneHandle(Z, function.name())); + body += CheckNullOptimized(String::ZoneHandle(Z, function.name())); body += UnboxTruncate(kUnboxedFfiIntPtr); body += StoreNativeField(Slot::Pointer_data_field()); } break; case MethodRecognizer::kFfiGetAddress: { ASSERT_EQUAL(function.NumParameters(), 1); body += LoadLocal(parsed_function_->RawParameterVariable(0)); // Pointer. - body += CheckNullOptimized(TokenPosition::kNoSource, - String::ZoneHandle(Z, function.name())); + body += CheckNullOptimized(String::ZoneHandle(Z, function.name())); // This can only be Pointer, so it is always safe to LoadUntagged. body += LoadUntagged(compiler::target::Pointer::data_field_offset()); body += ConvertUntaggedToUnboxed(kUnboxedFfiIntPtr); @@ -1517,9 +1507,9 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfRecognizedMethod( // Load TypedDataArray from Instance Handle implementing // NativeFieldWrapper. body += LoadLocal(parsed_function_->RawParameterVariable(0)); // Object. - body += CheckNullOptimized(TokenPosition::kNoSource, name); + body += CheckNullOptimized(name); body += LoadNativeField(Slot::Instance_native_fields_array()); // Fields. - body += CheckNullOptimized(TokenPosition::kNoSource, name); + body += CheckNullOptimized(name); // Load the native field at index. body += IntConstant(0); // Index. body += LoadIndexed(kIntPtrCid); @@ -4362,10 +4352,10 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfFfiNative(const Function& function) { function_body += LoadLocal( parsed_function_->ParameterVariable(kFirstArgumentParameterOffset + i)); // Check for 'null'. - // TODO(36780): Mention the param name instead of function reciever. - function_body += - CheckNullOptimized(TokenPosition::kNoSource, - String::ZoneHandle(Z, marshaller.function_name())); + function_body += CheckNullOptimized( + String::ZoneHandle( + Z, function.ParameterNameAt(kFirstArgumentParameterOffset + i)), + CheckNullInstr::kArgumentError); function_body += StoreLocal( TokenPosition::kNoSource, parsed_function_->ParameterVariable(kFirstArgumentParameterOffset + i)); @@ -4544,8 +4534,8 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfFfiCallback(const Function& function) { body += IntConstant(0); } else if (!marshaller.IsHandle(compiler::ffi::kResultIndex)) { body += - CheckNullOptimized(TokenPosition::kNoSource, - String::ZoneHandle(Z, marshaller.function_name())); + CheckNullOptimized(String::ZoneHandle(Z, String::New("return_value")), + CheckNullInstr::kArgumentError); } if (marshaller.IsCompound(compiler::ffi::kResultIndex)) { diff --git a/runtime/vm/runtime_entry.cc b/runtime/vm/runtime_entry.cc index fb90a6e5b6c..5ef43b9c446 100644 --- a/runtime/vm/runtime_entry.cc +++ b/runtime/vm/runtime_entry.cc @@ -144,7 +144,19 @@ DEFINE_RUNTIME_ENTRY(RangeError, 2) { Exceptions::ThrowByType(Exceptions::kRange, args); } -static void NullErrorHelper(Zone* zone, const String& selector) { +static void NullErrorHelper(Zone* zone, + const String& selector, + bool is_param_name = false) { + if (is_param_name) { + const String& error = String::Handle( + selector.IsNull() + ? String::New("argument value is null") + : String::NewFormatted("argument value for '%s' is null", + selector.ToCString())); + Exceptions::ThrowArgumentError(error); + return; + } + // If the selector is null, this must be a null check that wasn't due to a // method invocation, so was due to the null check operator. if (selector.IsNull()) { @@ -178,7 +190,10 @@ static void NullErrorHelper(Zone* zone, const String& selector) { Exceptions::ThrowByType(Exceptions::kNoSuchMethod, args); } -static void DoThrowNullError(Isolate* isolate, Thread* thread, Zone* zone) { +static void DoThrowNullError(Isolate* isolate, + Thread* thread, + Zone* zone, + bool is_param) { DartFrameIterator iterator(thread, StackFrameIterator::kNoCrossThreadIteration); const StackFrame* caller_frame = iterator.NextFrame(); @@ -205,11 +220,11 @@ static void DoThrowNullError(Isolate* isolate, Thread* thread, Zone* zone) { member_name = Symbols::OptimizedOut().ptr(); } - NullErrorHelper(zone, member_name); + NullErrorHelper(zone, member_name, is_param); } DEFINE_RUNTIME_ENTRY(NullError, 0) { - DoThrowNullError(isolate, thread, zone); + DoThrowNullError(isolate, thread, zone, /*is_param=*/false); } // Collects information about pointers within the top |kMaxSlotsCollected| @@ -258,7 +273,7 @@ DEFINE_RUNTIME_ENTRY(DispatchTableNullError, 1) { RELEASE_ASSERT(caller_frame->IsDartFrame()); ReportImpossibleNullError(cid.Value(), caller_frame, thread); } - DoThrowNullError(isolate, thread, zone); + DoThrowNullError(isolate, thread, zone, /*is_param=*/false); } DEFINE_RUNTIME_ENTRY(NullErrorWithSelector, 1) { @@ -271,8 +286,7 @@ DEFINE_RUNTIME_ENTRY(NullCastError, 0) { } DEFINE_RUNTIME_ENTRY(ArgumentNullError, 0) { - const String& error = String::Handle(String::New("argument value is null")); - Exceptions::ThrowArgumentError(error); + DoThrowNullError(isolate, thread, zone, /*is_param=*/true); } DEFINE_RUNTIME_ENTRY(ArgumentError, 1) { diff --git a/tests/ffi/function_test.dart b/tests/ffi/function_test.dart index d5fd3c0f1c5..6d4c77e9c71 100644 --- a/tests/ffi/function_test.dart +++ b/tests/ffi/function_test.dart @@ -39,6 +39,7 @@ void main() { testFloatRounding(); testVoidReturn(); testNoArgs(); + testNativeFunctionNullableInt(); } } @@ -457,3 +458,16 @@ void testNoArgs() { double result = inventFloatValue(); Expect.approxEquals(1337.0, result); } + +void testNativeFunctionNullableInt() { + final sumPlus42 = ffiTestFunctions.lookupFunction< + Int32 Function(Int32, Int32), int Function(int, int?)>("SumPlus42"); + + try { + sumPlus42(3, null); + } catch (e) { + // TODO(http://dartbug.com/47098): Save param names to dwarf. + Expect.isTrue(e.toString().contains('ffi_param2') || + e.toString().contains('')); + } +} diff --git a/tests/ffi_2/function_test.dart b/tests/ffi_2/function_test.dart index f955bbdfa0e..c0f9657159c 100644 --- a/tests/ffi_2/function_test.dart +++ b/tests/ffi_2/function_test.dart @@ -41,6 +41,7 @@ void main() { testFloatRounding(); testVoidReturn(); testNoArgs(); + testNativeFunctionNullableInt(); } } @@ -459,3 +460,16 @@ void testNoArgs() { double result = inventFloatValue(); Expect.approxEquals(1337.0, result); } + +void testNativeFunctionNullableInt() { + final sumPlus42 = ffiTestFunctions.lookupFunction< + Int32 Function(Int32, Int32), int Function(int, int)>("SumPlus42"); + + try { + sumPlus42(3, null); + } catch (e) { + // TODO(http://dartbug.com/47098): Save param names to dwarf. + Expect.isTrue(e.toString().contains('ffi_param2') || + e.toString().contains('')); + } +}