From 6e53ac6e4c7b3d94de5ee0370a538b03dafded1c Mon Sep 17 00:00:00 2001 From: Alexander Markov Date: Thu, 2 Apr 2020 01:39:10 +0000 Subject: [PATCH] [vm/nnbd] Fix supertype check in GenerateSubtype1TestCacheLookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before calling Subtype1TestCache stub, there was a quick check if supertype's class id matches tested class id. Null extends Object, so supertype of Null matches Object. However, in NNBD strong mode Null is not assignable to Object. Fixes https://github.com/dart-lang/sdk/issues/41272 Change-Id: Icc3efaf90992b5083cbe086af81db7504724b855 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/142085 Reviewed-by: Régis Crelier Commit-Queue: Alexander Markov --- .../backend/flow_graph_compiler_arm.cc | 18 ++++++++------ .../backend/flow_graph_compiler_arm64.cc | 14 +++++++---- .../backend/flow_graph_compiler_ia32.cc | 14 +++++++---- .../backend/flow_graph_compiler_x64.cc | 15 ++++++++---- runtime/vm/object.cc | 3 ++- .../nnbd/subtyping/regress41272_test.dart | 24 +++++++++++++++++++ 6 files changed, 65 insertions(+), 23 deletions(-) create mode 100644 tests/language/nnbd/subtyping/regress41272_test.dart diff --git a/runtime/vm/compiler/backend/flow_graph_compiler_arm.cc b/runtime/vm/compiler/backend/flow_graph_compiler_arm.cc index c32f5d1926a..d1d05c63774 100644 --- a/runtime/vm/compiler/backend/flow_graph_compiler_arm.cc +++ b/runtime/vm/compiler/backend/flow_graph_compiler_arm.cc @@ -428,13 +428,17 @@ RawSubtypeTestCache* FlowGraphCompiler::GenerateSubtype1TestCacheLookup( __ LoadClassId(R2, TypeTestABI::kInstanceReg); __ LoadClassById(R1, R2); // R1: instance class. - // Check immediate superclass equality. - __ ldr(R2, compiler::FieldAddress( - R1, compiler::target::Class::super_type_offset())); - __ ldr(R2, compiler::FieldAddress( - R2, compiler::target::Type::type_class_id_offset())); - __ CompareImmediate(R2, Smi::RawValue(type_class.id())); - __ b(is_instance_lbl, EQ); + // Check immediate superclass equality. If type_class is Object, then testing + // supertype may yield a wrong result for Null in NNBD strong mode (because + // Null also extends Object). + if (!type_class.IsObjectClass() || !Isolate::Current()->null_safety()) { + __ ldr(R2, compiler::FieldAddress( + R1, compiler::target::Class::super_type_offset())); + __ ldr(R2, compiler::FieldAddress( + R2, compiler::target::Type::type_class_id_offset())); + __ CompareImmediate(R2, Smi::RawValue(type_class.id())); + __ b(is_instance_lbl, EQ); + } const Register kInstantiatorTypeArgumentsReg = kNoRegister; const Register kFunctionTypeArgumentsReg = kNoRegister; diff --git a/runtime/vm/compiler/backend/flow_graph_compiler_arm64.cc b/runtime/vm/compiler/backend/flow_graph_compiler_arm64.cc index 20cc37dc3a1..fc9b93cc3e4 100644 --- a/runtime/vm/compiler/backend/flow_graph_compiler_arm64.cc +++ b/runtime/vm/compiler/backend/flow_graph_compiler_arm64.cc @@ -414,11 +414,15 @@ RawSubtypeTestCache* FlowGraphCompiler::GenerateSubtype1TestCacheLookup( __ LoadClassId(TMP, TypeTestABI::kInstanceReg); __ LoadClassById(R1, TMP); // R1: instance class. - // Check immediate superclass equality. - __ LoadFieldFromOffset(R2, R1, Class::super_type_offset()); - __ LoadFieldFromOffset(R2, R2, Type::type_class_id_offset()); - __ CompareImmediate(R2, Smi::RawValue(type_class.id())); - __ b(is_instance_lbl, EQ); + // Check immediate superclass equality. If type_class is Object, then testing + // supertype may yield a wrong result for Null in NNBD strong mode (because + // Null also extends Object). + if (!type_class.IsObjectClass() || !Isolate::Current()->null_safety()) { + __ LoadFieldFromOffset(R2, R1, Class::super_type_offset()); + __ LoadFieldFromOffset(R2, R2, Type::type_class_id_offset()); + __ CompareImmediate(R2, Smi::RawValue(type_class.id())); + __ b(is_instance_lbl, EQ); + } const Register kInstantiatorTypeArgumentsReg = kNoRegister; const Register kFunctionTypeArgumentsReg = kNoRegister; diff --git a/runtime/vm/compiler/backend/flow_graph_compiler_ia32.cc b/runtime/vm/compiler/backend/flow_graph_compiler_ia32.cc index d362feb3733..579430cdbbf 100644 --- a/runtime/vm/compiler/backend/flow_graph_compiler_ia32.cc +++ b/runtime/vm/compiler/backend/flow_graph_compiler_ia32.cc @@ -401,11 +401,15 @@ RawSubtypeTestCache* FlowGraphCompiler::GenerateSubtype1TestCacheLookup( __ LoadClassId(EDI, TypeTestABI::kInstanceReg); __ LoadClassById(ECX, EDI); // ECX: instance class. - // Check immediate superclass equality. - __ movl(EDI, compiler::FieldAddress(ECX, Class::super_type_offset())); - __ movl(EDI, compiler::FieldAddress(EDI, Type::type_class_id_offset())); - __ cmpl(EDI, compiler::Immediate(Smi::RawValue(type_class.id()))); - __ j(EQUAL, is_instance_lbl); + // Check immediate superclass equality. If type_class is Object, then testing + // supertype may yield a wrong result for Null in NNBD strong mode (because + // Null also extends Object). + if (!type_class.IsObjectClass() || !Isolate::Current()->null_safety()) { + __ movl(EDI, compiler::FieldAddress(ECX, Class::super_type_offset())); + __ movl(EDI, compiler::FieldAddress(EDI, Type::type_class_id_offset())); + __ cmpl(EDI, compiler::Immediate(Smi::RawValue(type_class.id()))); + __ j(EQUAL, is_instance_lbl); + } const Register kInstantiatorTypeArgumentsReg = kNoRegister; const Register kFunctionTypeArgumentsReg = kNoRegister; diff --git a/runtime/vm/compiler/backend/flow_graph_compiler_x64.cc b/runtime/vm/compiler/backend/flow_graph_compiler_x64.cc index f1f0c6b8f64..151047fae93 100644 --- a/runtime/vm/compiler/backend/flow_graph_compiler_x64.cc +++ b/runtime/vm/compiler/backend/flow_graph_compiler_x64.cc @@ -433,11 +433,16 @@ RawSubtypeTestCache* FlowGraphCompiler::GenerateSubtype1TestCacheLookup( __ LoadClassId(TMP, TypeTestABI::kInstanceReg); __ LoadClassById(R10, TMP); // R10: instance class. - // Check immediate superclass equality. - __ movq(R13, compiler::FieldAddress(R10, Class::super_type_offset())); - __ movq(R13, compiler::FieldAddress(R13, Type::type_class_id_offset())); - __ CompareImmediate(R13, compiler::Immediate(Smi::RawValue(type_class.id()))); - __ j(EQUAL, is_instance_lbl); + // Check immediate superclass equality. If type_class is Object, then testing + // supertype may yield a wrong result for Null in NNBD strong mode (because + // Null also extends Object). + if (!type_class.IsObjectClass() || !Isolate::Current()->null_safety()) { + __ movq(R13, compiler::FieldAddress(R10, Class::super_type_offset())); + __ movq(R13, compiler::FieldAddress(R13, Type::type_class_id_offset())); + __ CompareImmediate(R13, + compiler::Immediate(Smi::RawValue(type_class.id()))); + __ j(EQUAL, is_instance_lbl); + } const Register kInstantiatorTypeArgumentsReg = kNoRegister; const Register kFunctionTypeArgumentsReg = kNoRegister; diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc index 2afe44c5558..90d12303b05 100644 --- a/runtime/vm/object.cc +++ b/runtime/vm/object.cc @@ -17827,7 +17827,8 @@ bool Instance::RuntimeTypeIsSubtypeOf( } const Class& cls = Class::Handle(zone, clazz()); if (cls.IsClosureClass()) { - if (other.IsDartFunctionType() || other.IsDartClosureType()) { + if (other.IsDartFunctionType() || other.IsDartClosureType() || + other.IsObjectType()) { return true; } AbstractType& instantiated_other = AbstractType::Handle(zone, other.raw()); diff --git a/tests/language/nnbd/subtyping/regress41272_test.dart b/tests/language/nnbd/subtyping/regress41272_test.dart new file mode 100644 index 00000000000..2519404ec1c --- /dev/null +++ b/tests/language/nnbd/subtyping/regress41272_test.dart @@ -0,0 +1,24 @@ +// Copyright (c) 2020, 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. + +// VMOptions=--optimization_counter_threshold=10 --deterministic +// Requirements=nnbd-strong + +// Verifies that null cannot be casted to Object. +// Regression test for https://github.com/dart-lang/sdk/issues/41272. + +import 'package:expect/expect.dart'; + +doTest() { + dynamic x; + Expect.throwsTypeError(() { + x as Object; + }); +} + +main() { + for (int i = 0; i < 20; ++i) { + doTest(); + } +}