From 63ee73a2ed2e0938c77f3c04c2d7d834830bbd26 Mon Sep 17 00:00:00 2001 From: Nate Biggs Date: Thu, 22 May 2025 16:15:58 -0700 Subject: [PATCH] [dart2wasm] Fix dynamic modules bugs. - For direct calls in selector branch, ensure 'if' branches have correct inputs. - For overrideable selectors, the receiver type cannot be known in the main module so use 'top' type. Technically we could do better if selectors tracked interfaces they targeted. Then we could take the LUB of all those classes. But this would be a significant refactor for a small benefit. - Type checks on classes defined in the main module should use the class ID ranges from the main module rather than those from the dynamic submodule. This only applies to non-dynamic module extendable types (otherwise we'd use the RTT checks). So we know the class can only exist in one of the range sets, not both. I discovered (1) from running Flutter which led me to create this test which uncovered (2) and (3). Change-Id: I80f39835f66aa7cf0cff527341e3f4a948a7a0cb Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/430360 Reviewed-by: Martin Kustermann Commit-Queue: Nate Biggs --- pkg/dart2wasm/lib/class_info.dart | 16 ++++++-- pkg/dart2wasm/lib/dispatch_table.dart | 4 +- pkg/dart2wasm/lib/dynamic_modules.dart | 2 +- pkg/dart2wasm/lib/types.dart | 37 +++++++++++++++++-- .../invoke_checked/dynamic_interface.yaml | 17 +++++++++ .../test/data/invoke_checked/main.dart | 23 ++++++++++++ .../data/invoke_checked/modules/entry1.dart | 15 ++++++++ .../data/invoke_checked/shared/shared.dart | 12 ++++++ 8 files changed, 116 insertions(+), 10 deletions(-) create mode 100644 pkg/dynamic_modules/test/data/invoke_checked/dynamic_interface.yaml create mode 100644 pkg/dynamic_modules/test/data/invoke_checked/main.dart create mode 100644 pkg/dynamic_modules/test/data/invoke_checked/modules/entry1.dart create mode 100644 pkg/dynamic_modules/test/data/invoke_checked/shared/shared.dart diff --git a/pkg/dart2wasm/lib/class_info.dart b/pkg/dart2wasm/lib/class_info.dart index fa5c731ff03..31a20d6474f 100644 --- a/pkg/dart2wasm/lib/class_info.dart +++ b/pkg/dart2wasm/lib/class_info.dart @@ -678,10 +678,18 @@ class ClassIdNumbering { _concreteSubclassIdRangeForDynamicSubmodule); } - List getConcreteClassIdRangeForCurrentModule(Class klass) { - return translator.isDynamicSubmodule - ? getConcreteClassIdRangeForDynamicSubmodule(klass) - : getConcreteClassIdRangeForMainModule(klass); + /// In case the [klass] is from a dynamic module the returned class id + /// ranges may be relative. The caller has to ensure to use them + /// appropriately. + List getConcreteClassIdRangeForClass(Class klass) { + // We cannot return class id ranges for [klass] if there can be more + // classes in future dynamic module compilations. + assert(!klass.isDynamicSubmoduleExtendable(translator.coreTypes)); + + return !translator.isDynamicSubmodule || + klass.enclosingLibrary.isFromMainModule(translator.coreTypes) + ? getConcreteClassIdRangeForMainModule(klass) + : getConcreteClassIdRangeForDynamicSubmodule(klass); } List _getConcreteClassIdRange(Class klass, diff --git a/pkg/dart2wasm/lib/dispatch_table.dart b/pkg/dart2wasm/lib/dispatch_table.dart index cf74f0eff6a..483bc848391 100644 --- a/pkg/dart2wasm/lib/dispatch_table.dart +++ b/pkg/dart2wasm/lib/dispatch_table.dart @@ -202,7 +202,9 @@ class SelectorInfo { } } assert(returns.length <= outputSets.length); - inputSets[0].add(translator.translateType(receiver)); + inputSets[0].add(isDynamicSubmoduleOverridable + ? translator.topInfo.nonNullableType + : translator.translateType(receiver)); for (int i = 0; i < positional.length; i++) { DartType type = positional[i]; inputSets[1 + i].add(translator.translateType(type)); diff --git a/pkg/dart2wasm/lib/dynamic_modules.dart b/pkg/dart2wasm/lib/dynamic_modules.dart index 59baf8f8eff..b7001aab186 100644 --- a/pkg/dart2wasm/lib/dynamic_modules.dart +++ b/pkg/dart2wasm/lib/dynamic_modules.dart @@ -835,7 +835,7 @@ class DynamicModuleInfo { // Check if the invocation is checked or unchecked and use the // appropriate offset. ib.local_get(ib.locals[function.type.inputs.length - 1]); - ib.if_(const [], localSignature.outputs); + ib.if_(localSignature.inputs, localSignature.outputs); ib.invoke(translator.directCallTarget(uncheckedTarget)); ib.else_(); ib.invoke(translator.directCallTarget(checkedTarget)); diff --git a/pkg/dart2wasm/lib/types.dart b/pkg/dart2wasm/lib/types.dart index 936a8636b85..45f6e17ebda 100644 --- a/pkg/dart2wasm/lib/types.dart +++ b/pkg/dart2wasm/lib/types.dart @@ -767,7 +767,7 @@ class IsCheckerCallTarget extends CallTarget { // Always inline single class-id range checks (no branching, simply loads, // arithmetic and unsigned compare). final ranges = translator.classIdNumbering - .getConcreteClassIdRangeForCurrentModule(interfaceClass); + .getConcreteClassIdRangeForClass(interfaceClass); return ranges.length <= 1; } @@ -883,10 +883,37 @@ class IsCheckerCodeGenerator implements CodeGenerator { b.ref_test(translator.closureInfo.nonNullableType); } else { final ranges = translator.classIdNumbering - .getConcreteClassIdRangeForCurrentModule(interfaceClass); + .getConcreteClassIdRangeForClass(interfaceClass); b.local_get(operand); b.struct_get(translator.topInfo.struct, FieldIndex.classId); - b.emitClassIdRangeCheck(ranges); + if (translator.isDynamicSubmodule) { + // Only types that are not dynamic module extendable can get here. + final classIdLocal = b.addLocal(w.NumType.i32); + b.local_tee(classIdLocal); + translator.callReference(translator.classIdToModuleId.reference, b); + b.i32_wrap_i64(); + // Check if the class ID belongs to this module or the main module. + b.global_get(translator.dynamicModuleInfo!.moduleIdGlobal); + b.i32_wrap_i64(); + b.i32_eq(); + b.local_get(classIdLocal); + b.i32_const(translator.classIdNumbering.firstDynamicSubmoduleClassId); + b.i32_lt_u(); + b.i32_or(); + b.if_(const [], const [w.NumType.i32]); + // If it is in a known range, then localize the class ID to this + // module. If the class is from the main module this will do nothing. + b.local_get(classIdLocal); + translator.callReference(translator.localizeClassId.reference, b); + b.emitClassIdRangeCheck(ranges); + b.else_(); + // If it's not in the main module or this submodule then the type is + // unknown to this module so the test fails. + b.i32_const(0); + b.end(); + } else { + b.emitClassIdRangeCheck(ranges); + } } b.br(resultLabel); } @@ -922,7 +949,9 @@ class AsCheckerCallTarget extends CallTarget { this.testedAgainstType, this.operandIsNullable, this.checkArguments, - this.argumentCount); + this.argumentCount) + : assert(!testedAgainstType.classNode + .isDynamicSubmoduleExtendable(translator.coreTypes)); @override String get name { diff --git a/pkg/dynamic_modules/test/data/invoke_checked/dynamic_interface.yaml b/pkg/dynamic_modules/test/data/invoke_checked/dynamic_interface.yaml new file mode 100644 index 00000000000..b5842a08bf1 --- /dev/null +++ b/pkg/dynamic_modules/test/data/invoke_checked/dynamic_interface.yaml @@ -0,0 +1,17 @@ +# Copyright (c) 2025, 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. +callable: + - library: 'shared/shared.dart' + class: 'Base' + member: '' + - library: 'shared/shared.dart' + class: 'Tester' +extendable: + - library: 'shared/shared.dart' + class: 'Base' + +can-be-overridden: + - library: 'shared/shared.dart' + class: 'Base' + member: 'foo' diff --git a/pkg/dynamic_modules/test/data/invoke_checked/main.dart b/pkg/dynamic_modules/test/data/invoke_checked/main.dart new file mode 100644 index 00000000000..29e42c9713d --- /dev/null +++ b/pkg/dynamic_modules/test/data/invoke_checked/main.dart @@ -0,0 +1,23 @@ +// Copyright (c) 2025, 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. + +import '../../common/testing.dart' as helper; +import 'package:expect/expect.dart'; + +import 'shared/shared.dart'; + +class MyChild extends Base { + @override + Tester foo(covariant Tester x) { + return Tester(x.val + 1); + } +} + +/// A dynamic module is allowed to extend a class in the dynamic interface and +/// override its members. +void main() async { + final c = (await helper.load('entry1.dart')) as Base; + Expect.equals(3, c.foo(Tester(3)).val); + helper.done(); +} diff --git a/pkg/dynamic_modules/test/data/invoke_checked/modules/entry1.dart b/pkg/dynamic_modules/test/data/invoke_checked/modules/entry1.dart new file mode 100644 index 00000000000..08c16bf08a0 --- /dev/null +++ b/pkg/dynamic_modules/test/data/invoke_checked/modules/entry1.dart @@ -0,0 +1,15 @@ +// Copyright (c) 2025, 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. + +import '../shared/shared.dart'; + +class Child extends Base { + @override + Tester foo(covariant Tester x) { + return x; + } +} + +@pragma('dyn-module:entry-point') +Object? dynamicModuleEntrypoint() => Child(); diff --git a/pkg/dynamic_modules/test/data/invoke_checked/shared/shared.dart b/pkg/dynamic_modules/test/data/invoke_checked/shared/shared.dart new file mode 100644 index 00000000000..d65cc024aeb --- /dev/null +++ b/pkg/dynamic_modules/test/data/invoke_checked/shared/shared.dart @@ -0,0 +1,12 @@ +// Copyright (c) 2025, 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. + +abstract class Base { + Tester foo(Tester t); +} + +class Tester { + final int val; + Tester(this.val); +}