Skip to content

Commit 63ee73a

Browse files
natebiggsCommit Queue
authored and
Commit Queue
committed
[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 <[email protected]> Commit-Queue: Nate Biggs <[email protected]>
1 parent 09cdebd commit 63ee73a

File tree

8 files changed

+116
-10
lines changed

8 files changed

+116
-10
lines changed

pkg/dart2wasm/lib/class_info.dart

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -678,10 +678,18 @@ class ClassIdNumbering {
678678
_concreteSubclassIdRangeForDynamicSubmodule);
679679
}
680680

681-
List<Range> getConcreteClassIdRangeForCurrentModule(Class klass) {
682-
return translator.isDynamicSubmodule
683-
? getConcreteClassIdRangeForDynamicSubmodule(klass)
684-
: getConcreteClassIdRangeForMainModule(klass);
681+
/// In case the [klass] is from a dynamic module the returned class id
682+
/// ranges may be relative. The caller has to ensure to use them
683+
/// appropriately.
684+
List<Range> getConcreteClassIdRangeForClass(Class klass) {
685+
// We cannot return class id ranges for [klass] if there can be more
686+
// classes in future dynamic module compilations.
687+
assert(!klass.isDynamicSubmoduleExtendable(translator.coreTypes));
688+
689+
return !translator.isDynamicSubmodule ||
690+
klass.enclosingLibrary.isFromMainModule(translator.coreTypes)
691+
? getConcreteClassIdRangeForMainModule(klass)
692+
: getConcreteClassIdRangeForDynamicSubmodule(klass);
685693
}
686694

687695
List<Range> _getConcreteClassIdRange(Class klass,

pkg/dart2wasm/lib/dispatch_table.dart

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,9 @@ class SelectorInfo {
202202
}
203203
}
204204
assert(returns.length <= outputSets.length);
205-
inputSets[0].add(translator.translateType(receiver));
205+
inputSets[0].add(isDynamicSubmoduleOverridable
206+
? translator.topInfo.nonNullableType
207+
: translator.translateType(receiver));
206208
for (int i = 0; i < positional.length; i++) {
207209
DartType type = positional[i];
208210
inputSets[1 + i].add(translator.translateType(type));

pkg/dart2wasm/lib/dynamic_modules.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -835,7 +835,7 @@ class DynamicModuleInfo {
835835
// Check if the invocation is checked or unchecked and use the
836836
// appropriate offset.
837837
ib.local_get(ib.locals[function.type.inputs.length - 1]);
838-
ib.if_(const [], localSignature.outputs);
838+
ib.if_(localSignature.inputs, localSignature.outputs);
839839
ib.invoke(translator.directCallTarget(uncheckedTarget));
840840
ib.else_();
841841
ib.invoke(translator.directCallTarget(checkedTarget));

pkg/dart2wasm/lib/types.dart

Lines changed: 33 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -767,7 +767,7 @@ class IsCheckerCallTarget extends CallTarget {
767767
// Always inline single class-id range checks (no branching, simply loads,
768768
// arithmetic and unsigned compare).
769769
final ranges = translator.classIdNumbering
770-
.getConcreteClassIdRangeForCurrentModule(interfaceClass);
770+
.getConcreteClassIdRangeForClass(interfaceClass);
771771
return ranges.length <= 1;
772772
}
773773

@@ -883,10 +883,37 @@ class IsCheckerCodeGenerator implements CodeGenerator {
883883
b.ref_test(translator.closureInfo.nonNullableType);
884884
} else {
885885
final ranges = translator.classIdNumbering
886-
.getConcreteClassIdRangeForCurrentModule(interfaceClass);
886+
.getConcreteClassIdRangeForClass(interfaceClass);
887887
b.local_get(operand);
888888
b.struct_get(translator.topInfo.struct, FieldIndex.classId);
889-
b.emitClassIdRangeCheck(ranges);
889+
if (translator.isDynamicSubmodule) {
890+
// Only types that are not dynamic module extendable can get here.
891+
final classIdLocal = b.addLocal(w.NumType.i32);
892+
b.local_tee(classIdLocal);
893+
translator.callReference(translator.classIdToModuleId.reference, b);
894+
b.i32_wrap_i64();
895+
// Check if the class ID belongs to this module or the main module.
896+
b.global_get(translator.dynamicModuleInfo!.moduleIdGlobal);
897+
b.i32_wrap_i64();
898+
b.i32_eq();
899+
b.local_get(classIdLocal);
900+
b.i32_const(translator.classIdNumbering.firstDynamicSubmoduleClassId);
901+
b.i32_lt_u();
902+
b.i32_or();
903+
b.if_(const [], const [w.NumType.i32]);
904+
// If it is in a known range, then localize the class ID to this
905+
// module. If the class is from the main module this will do nothing.
906+
b.local_get(classIdLocal);
907+
translator.callReference(translator.localizeClassId.reference, b);
908+
b.emitClassIdRangeCheck(ranges);
909+
b.else_();
910+
// If it's not in the main module or this submodule then the type is
911+
// unknown to this module so the test fails.
912+
b.i32_const(0);
913+
b.end();
914+
} else {
915+
b.emitClassIdRangeCheck(ranges);
916+
}
890917
}
891918
b.br(resultLabel);
892919
}
@@ -922,7 +949,9 @@ class AsCheckerCallTarget extends CallTarget {
922949
this.testedAgainstType,
923950
this.operandIsNullable,
924951
this.checkArguments,
925-
this.argumentCount);
952+
this.argumentCount)
953+
: assert(!testedAgainstType.classNode
954+
.isDynamicSubmoduleExtendable(translator.coreTypes));
926955

927956
@override
928957
String get name {
Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
# Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
2+
# for details. All rights reserved. Use of this source code is governed by a
3+
# BSD-style license that can be found in the LICENSE file.
4+
callable:
5+
- library: 'shared/shared.dart'
6+
class: 'Base'
7+
member: ''
8+
- library: 'shared/shared.dart'
9+
class: 'Tester'
10+
extendable:
11+
- library: 'shared/shared.dart'
12+
class: 'Base'
13+
14+
can-be-overridden:
15+
- library: 'shared/shared.dart'
16+
class: 'Base'
17+
member: 'foo'
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
import '../../common/testing.dart' as helper;
6+
import 'package:expect/expect.dart';
7+
8+
import 'shared/shared.dart';
9+
10+
class MyChild extends Base {
11+
@override
12+
Tester foo(covariant Tester x) {
13+
return Tester(x.val + 1);
14+
}
15+
}
16+
17+
/// A dynamic module is allowed to extend a class in the dynamic interface and
18+
/// override its members.
19+
void main() async {
20+
final c = (await helper.load('entry1.dart')) as Base;
21+
Expect.equals(3, c.foo(Tester(3)).val);
22+
helper.done();
23+
}
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
import '../shared/shared.dart';
6+
7+
class Child extends Base {
8+
@override
9+
Tester foo(covariant Tester x) {
10+
return x;
11+
}
12+
}
13+
14+
@pragma('dyn-module:entry-point')
15+
Object? dynamicModuleEntrypoint() => Child();
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
abstract class Base {
6+
Tester foo(Tester t);
7+
}
8+
9+
class Tester {
10+
final int val;
11+
Tester(this.val);
12+
}

0 commit comments

Comments
 (0)