[vm/aot] Avoid unbinding canonical names in AOT transformations

AOT transformations (mixin deduplication and TFA tree shaker) were
using CanonicalName.unbind() in order to make sure that kernel
writer will not be able to write dangling references to the deleted
classes or members (to detect such dangling references earlier).

This conflicts with lazy reading from a kernel file if --from-dill
option is used: lazy reader may read a reference to such class after
unbind() and it creates a new Reference object for such canonical name.
This causes crash later when the fresh Reference is used as it doesn't
have Class node filled in.

The solution is to avoid calling CanonicalName.unbind().
Instead, it is enough to nullify node.reference.canonicalName, which
won't break CanonicalName.reference used by lazy reader.

This crash can be reproduced on tests/language_2/vm/regress_flutter_55345_test.dart
if it is compiled in AOT mode using 2-step kernel generation.

Bug: b/158853113
Change-Id: Ib2004dbbbda85d9f56adc56b48882f4ef08869a7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/151120
Commit-Queue: Alexander Markov <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>

Author: alexmarkov

pkg/vm/lib/transformations/mixin_deduplication.dart

@@ -113,7 +113,9 @@ class DeduplicateMixinsTransformer extends Transformer {
     assert(canonical != null);
 
     if (canonical != c) {
-      c.canonicalName?.unbind();
+      // Ensure that kernel file writer will not be able to
+      // write a dangling reference to the deleted class.
+      c.reference.canonicalName = null;
       _duplicatedMixins[c] = canonical;
       return null; // Remove class.
     }

pkg/vm/lib/transformations/type_flow/transformer.dart

@@ -1464,7 +1464,9 @@ class _TreeShakerPass2 extends Transformer {
   Class visitClass(Class node) {
     if (!shaker.isClassUsed(node)) {
       debugPrint('Dropped class ${node.name}');
-      node.canonicalName?.unbind();
+      // Ensure that kernel file writer will not be able to
+      // write a dangling reference to the deleted class.
+      node.reference.canonicalName = null;
       Statistics.classesDropped++;
       return null; // Remove the class.
     }
@@ -1532,7 +1534,9 @@ class _TreeShakerPass2 extends Transformer {
   @override
   Member defaultMember(Member node) {
     if (!shaker.isMemberUsed(node) && !_preserveSpecialMember(node)) {
-      node.canonicalName?.unbind();
+      // Ensure that kernel file writer will not be able to
+      // write a dangling reference to the deleted member.
+      node.reference.canonicalName = null;
       Statistics.membersDropped++;
       return null;
     }

runtime/tests/vm/dart/split_aot_kernel_generation2_test.dart

@@ -0,0 +1,18 @@
+// 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.
+
+// OtherResources=../../../../tests/language/vm/regress_flutter_55345_test.dart
+
+// Runs regress_flutter_55345_test.dart using AOT kernel generation split into
+// 2 steps using '--from-dill' option.
+
+import 'dart:io' show Platform;
+
+import 'split_aot_kernel_generation_test.dart'
+    show runSplitAOTKernelGenerationTest;
+
+main() async {
+  await runSplitAOTKernelGenerationTest(Platform.script.resolve(
+      '../../../../tests/language/vm/regress_flutter_55345_test.dart'));
+}

runtime/tests/vm/dart/split_aot_compilation_test.dart renamed to runtime/tests/vm/dart/split_aot_kernel_generation_test.dart

@@ -4,16 +4,14 @@
 
 // OtherResources=splay_test.dart
 
-// Tests AOT compilation split into 2 steps using '--from-dill' option.
+// Tests AOT kernel generation split into 2 steps using '--from-dill' option.
 
 import 'dart:io' show Platform;
 
 import 'package:path/path.dart' as path;
 import 'snapshot_test_helper.dart';
 
-main() async {
-  final testScriptUri = Platform.script.resolve('splay_test.dart');
-
+Future<void> runSplitAOTKernelGenerationTest(Uri testScriptUri) async {
   await withTempDir((String temp) async {
     final intermediateDillPath = path.join(temp, 'intermediate.dill');
     final outputDillPath = path.join(temp, 'output.dill');
@@ -52,3 +50,8 @@ main() async {
     await runBinary('RUN SNAPSHOT', dartPrecompiledRuntime, [snapshotPath]);
   });
 }
+
+main() async {
+  await runSplitAOTKernelGenerationTest(
+      Platform.script.resolve('splay_test.dart'));
+}

runtime/tests/vm/dart_2/split_aot_kernel_generation2_test.dart

@@ -0,0 +1,18 @@
+// 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.
+
+// OtherResources=../../../../tests/language_2/vm/regress_flutter_55345_test.dart
+
+// Runs regress_flutter_55345_test.dart using AOT kernel generation split into
+// 2 steps using '--from-dill' option.
+
+import 'dart:io' show Platform;
+
+import 'split_aot_kernel_generation_test.dart'
+    show runSplitAOTKernelGenerationTest;
+
+main() async {
+  await runSplitAOTKernelGenerationTest(Platform.script.resolve(
+      '../../../../tests/language_2/vm/regress_flutter_55345_test.dart'));
+}

runtime/tests/vm/dart_2/split_aot_compilation_test.dart renamed to runtime/tests/vm/dart_2/split_aot_kernel_generation_test.dart

@@ -4,16 +4,14 @@
 
 // OtherResources=splay_test.dart
 
-// Tests AOT compilation split into 2 steps using '--from-dill' option.
+// Tests AOT kernel generation split into 2 steps using '--from-dill' option.
 
 import 'dart:io' show Platform;
 
 import 'package:path/path.dart' as path;
 import 'snapshot_test_helper.dart';
 
-main() async {
-  final testScriptUri = Platform.script.resolve('splay_test.dart');
-
+Future<void> runSplitAOTKernelGenerationTest(Uri testScriptUri) async {
   await withTempDir((String temp) async {
     final intermediateDillPath = path.join(temp, 'intermediate.dill');
     final outputDillPath = path.join(temp, 'output.dill');
@@ -52,3 +50,8 @@ main() async {
     await runBinary('RUN SNAPSHOT', dartPrecompiledRuntime, [snapshotPath]);
   });
 }
+
+main() async {
+  await runSplitAOTKernelGenerationTest(
+      Platform.script.resolve('splay_test.dart'));
+}

runtime/tests/vm/vm.status

@@ -88,10 +88,12 @@ dart_2/transferable_throws_oom_test: SkipByDesign # This test tries to allocate
 [ $builder_tag == crossword || $builder_tag == crossword_ast ]
 dart/emit_aot_size_info_flag_test: SkipByDesign # The test itself cannot determine the location of gen_snapshot (only tools/test.py knows where it is).
 dart/bytecode_with_ast_in_aot_test: SkipByDesign # The test doesn't know location of cross-platform gen_snapshot
-dart/split_aot_compilation_test: SkipByDesign # The test doesn't know location of cross-platform gen_snapshot
+dart/split_aot_kernel_generation_test: SkipByDesign # The test doesn't know location of cross-platform gen_snapshot
+dart/split_aot_kernel_generation2_test: SkipByDesign # The test doesn't know location of cross-platform gen_snapshot
 dart_2/emit_aot_size_info_flag_test: SkipByDesign # The test itself cannot determine the location of gen_snapshot (only tools/test.py knows where it is).
 dart_2/bytecode_with_ast_in_aot_test: SkipByDesign # The test doesn't know location of cross-platform gen_snapshot
-dart_2/split_aot_compilation_test: SkipByDesign # The test doesn't know location of cross-platform gen_snapshot
+dart_2/split_aot_kernel_generation_test: SkipByDesign # The test doesn't know location of cross-platform gen_snapshot
+dart_2/split_aot_kernel_generation2_test: SkipByDesign # The test doesn't know location of cross-platform gen_snapshot
 
 [ $builder_tag == obfuscated ]
 dart/causal_stacks/async_throws_stack_lazy_test: SkipByDesign # Asserts exact stacktrace output.
@@ -410,11 +412,13 @@ dart_2/redirection_type_shuffling_test/none: RuntimeError
 dart/emit_aot_size_info_flag_test: SkipByDesign # This test is for VM AOT only and is quite slow (so we don't run it in debug mode).
 dart/use_bare_instructions_flag_test: SkipByDesign # This test is for VM AOT only and is quite slow (so we don't run it in debug mode).
 dart/bytecode_with_ast_in_aot_test: SkipByDesign # This test is for VM AOT only and is quite slow (so we don't run it in debug mode).
-dart/split_aot_compilation_test: SkipByDesign # This test is for VM AOT only and is quite slow (so we don't run it in debug mode).
+dart/split_aot_kernel_generation_test: SkipByDesign # This test is for VM AOT only and is quite slow (so we don't run it in debug mode).
+dart/split_aot_kernel_generation2_test: SkipByDesign # This test is for VM AOT only and is quite slow (so we don't run it in debug mode).
 dart_2/emit_aot_size_info_flag_test: SkipByDesign # This test is for VM AOT only and is quite slow (so we don't run it in debug mode).
 dart_2/use_bare_instructions_flag_test: SkipByDesign # This test is for VM AOT only and is quite slow (so we don't run it in debug mode).
 dart_2/bytecode_with_ast_in_aot_test: SkipByDesign # This test is for VM AOT only and is quite slow (so we don't run it in debug mode).
-dart_2/split_aot_compilation_test: SkipByDesign # This test is for VM AOT only and is quite slow (so we don't run it in debug mode).
+dart_2/split_aot_kernel_generation_test: SkipByDesign # This test is for VM AOT only and is quite slow (so we don't run it in debug mode).
+dart_2/split_aot_kernel_generation2_test: SkipByDesign # This test is for VM AOT only and is quite slow (so we don't run it in debug mode).
 
 # It makes no sense to run any test that uses spawnURI under the simulator
 # as that would involve running CFE (the front end) in simulator mode

tests/language/vm/regress_flutter_55345_test.dart

@@ -0,0 +1,41 @@
+// 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.
+
+// Verifies that references to deduplicated mixins are properly updated.
+// Regression test for https://github.com/flutter/flutter/issues/55345.
+
+class Diagnosticable {}
+
+class SomeClass with Diagnosticable {}
+
+class State<T> with Diagnosticable {}
+
+class StateA extends State {}
+
+class StateB extends State<int> {}
+
+StateA? a = StateA();
+StateB? b = StateB();
+
+List<T> foo<T>(T x) {
+  print(T);
+  return <T>[x];
+}
+
+T Function<S extends T>(T) bar<T>(T x) {
+  print(T);
+
+  return <S extends T>(T y) {
+    print(S);
+    print(y);
+    return y;
+  };
+}
+
+main() {
+  var x2 = a ?? b;
+  var x3 = foo(x2);
+  var x4 = bar(x3);
+  x4(x3);
+}

tools/bots/test_matrix.json

@@ -45,6 +45,7 @@
       "tests/corelib_2/",
       "tests/dart2js_2/",
       "tests/kernel/",
+      "tests/language/",
       "tests/language_2/",
       "tests/lib_2/",
       "tests/light_unittest.dart",
@@ -85,6 +86,7 @@
       "tests/dart2js/",
       "tests/kernel/",
       "tests/language/",
+      "tests/language_2/",
       "tests/lib/",
       "tests/light_unittest.dart",
       "tests/search/",
@@ -130,6 +132,7 @@
       "tests/corelib_2/",
       "tests/dart2js_2/",
       "tests/kernel/",
+      "tests/language/",
       "tests/language_2/",
       "tests/lib_2/",
       "tests/light_unittest.dart",
@@ -177,6 +180,7 @@
       "tests/corelib_2/",
       "tests/dart2js_2/",
       "tests/kernel/",
+      "tests/language/",
       "tests/language_2/",
       "tests/lib_2/",
       "tests/light_unittest.dart",
@@ -218,6 +222,7 @@
       "tests/dart2js/",
       "tests/kernel/",
       "tests/language/",
+      "tests/language_2/",
       "tests/lib/",
       "tests/light_unittest.dart",
       "tests/search/",