dart-lang/sdk
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

[vm/ffi] Adds param number in trampoline null error

Browse Source 
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 <dacoharkes@google.com>
Reviewed-by: Clement Skau <cskau@google.com>
Reviewed-by: Tess Strickland <sstrickl@google.com>
This commit is contained in:
Daco Harkes
2021-09-04 07:22:03 +00:00
committed by commit-bot@chromium.org
parent 01c4999b31
commit f21b7cafbc
7 changed files with 86 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files
runtime/vm/compiler/backend/il.h
+1 -5
View File
@@ -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:
runtime/vm/compiler/frontend/base_flow_graph_builder.cc
+7 -4
View File
@@ -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);
}
runtime/vm/compiler/frontend/base_flow_graph_builder.h
+11 -4
View File
@@ -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,
runtime/vm/compiler/frontend/kernel_to_il.cc
+18 -28
View File
@@ -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<Pointer<X>> 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)) {
runtime/vm/runtime_entry.cc
+21 -7
View File
@@ -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) {
tests/ffi/function_test.dart
+14
View File
@@ -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('<optimized out>'));
}
}
tests/ffi_2/function_test.dart
+14
View File
@@ -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('<optimized out>'));
}
}