Proposed fix to #36644

TL;DR: Unbind canonical names doesn't do what you think it does and probably shouldn't (ever) be used. This CL stops using it in a few places. Longer version: When loading a dill file it: - First loads the table of canonical names. These have no references yet. - When a canonical name is asked for its reference it creates one if it doesn't yet have one. - When loading, for instance, a library, it asks for the reference. When unbinding a canonical name: - It removes itself (the canonical name) from the reference - It removes the reference in itself - Note: Whatever has a pointer to the reference keeps it, and the reference points to whatever node it already pointed to. This also means, that if we have a dill file that's split in two and: - Load #1 - Load #2 that works fine, but if we - Load #1 - Unbind canonical names - Binds canonical names - Load #2 stuff is not bound correctly (and an error is thrown). And - the cause of this bug: - Load #1 - Load #2 everything is fine - Unbind canonical names - Binds canonical names - Load #2' stuff is not bound correctly --- references points to stuff loaded as #2, not as #2'. On top of being weird, wrong and confusing it also caused wrong things to be but into the class hierarchy which ultimatly caused the crash. This CL fixes it by not calling unbind and force loading of dill files (at specific call sites) to create new libraries (and in the process overwriting references ".node"). Revert "[dartdevc] Retry ddc incremental compile on crash" This reverts commit ecdbdf00b8. Revert "[kernel_worker] retry on failure" This reverts commit 43eebea5a3. Fixes #36644 Bug: #36644 Change-Id: Id8f548179e6a409b01f2ebfa3219f94cb64b1c05 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/100380 Reviewed-by: Kevin Millikin <kmillikin@google.com> Reviewed-by: Vijay Menon <vsm@google.com> Reviewed-by: Jake Macdonald <jakemac@google.com> Reviewed-by: Jenny Messerly <jmesserly@google.com> Commit-Queue: Jens Johansen <jensj@google.com>
2024-09-05 00:13:50 +00:00 · 2019-04-26 07:18:28 +00:00 · 2019-04-26 07:18:28 +00:00 · ed8e4255a4
parent 7adad2a245
commit ed8e4255a4
12 changed files with 283 additions and 62 deletions
--- a/pkg/dev_compiler/bin/dartdevc.dart
+++ b/pkg/dev_compiler/bin/dartdevc.dart
@ -56,18 +56,6 @@ class _CompilerWorker extends AsyncWorkerLoop {
            ZoneSpecification(print: (self, parent, zone, message) {
      output.writeln(message.toString());
    }));
-
-    if (lastResult.crashed && context != null) {
-      // TODO(vsm): See https://github.com/dart-lang/sdk/issues/36644.
-      // If the CFE is crashing with previous state, then clear compilation
-      // state and try again.
-      output.clear();
-      lastResult = await runZoned(() => compile(args, previousResult: null),
-          zoneSpecification:
-              ZoneSpecification(print: (self, parent, zone, message) {
-        output.writeln(message.toString());
-      }));
-    }
    return WorkResponse()
      ..exitCode = lastResult.success ? 0 : 1
      ..output = output.toString();
--- a/pkg/dev_compiler/lib/src/flutter/track_widget_constructor_locations.dart
+++ b/pkg/dev_compiler/lib/src/flutter/track_widget_constructor_locations.dart
@ -364,13 +364,15 @@ class WidgetCreatorTracker {
    // We intentionally use the library context of the _HasCreationLocation
    // class for the private field even if [clazz] is in a different library
    // so that all classes implementing Widget behave consistently.
-    final Field locationField = new Field(
-      new Name(
-        _locationFieldName,
-        _hasCreationLocationClass.enclosingLibrary,
-      ),
-      isFinal: true,
+    final Name fieldName = new Name(
+      _locationFieldName,
+      _hasCreationLocationClass.enclosingLibrary,
    );
+    final Field locationField = new Field(fieldName,
+        isFinal: true,
+        reference: clazz.reference.canonicalName
+            ?.getChildFromFieldWithName(fieldName)
+            ?.reference);
    clazz.addMember(locationField);

    final Set<Constructor> _handledConstructors =
--- a/pkg/dev_compiler/lib/src/kernel/command.dart
+++ b/pkg/dev_compiler/lib/src/kernel/command.dart
@ -323,6 +323,7 @@ Future<CompilerResult> _compile(List<String> args,
  // TODO(jmesserly): remove this hack once Flutter SDK has a `dartdevc` with
  // support for the widget inspector.
  if (argResults['track-widget-creation'] as bool) {
+    component.computeCanonicalNames();
    WidgetCreatorTracker(hierarchy).transform(component);
  }

--- a/pkg/front_end/lib/src/api_unstable/bazel_worker.dart
+++ b/pkg/front_end/lib/src/api_unstable/bazel_worker.dart
@ -114,8 +114,6 @@ Future<InitializedCompilerState> initializeIncrementalCompiler(
    for (WorkerInputComponent cachedInput in workerInputCache.values) {
      cachedInput.component.adoptChildren();
    }
-    sdkComponent.unbindCanonicalNames();
-    sdkComponent.computeCanonicalNames();

    // Reuse the incremental compiler, but reset as needed.
    incrementalCompiler = oldState.incrementalCompiler;
@ -125,9 +123,6 @@ Future<InitializedCompilerState> initializeIncrementalCompiler(
  }

  // Then read all the input summary components.
-  // The nameRoot from the sdk was either just created or just unbound.
-  // If just unbound, only the sdk stuff is bound. Either way, don't clear it
-  // again and bind as much as possible before loading new stuff!
  CanonicalName nameRoot = cachedSdkInput.component.root;
  final inputSummaries = <Component>[];
  List<Uri> loadFromDill = new List<Uri>();
@ -144,8 +139,6 @@ Future<InitializedCompilerState> initializeIncrementalCompiler(
      for (var lib in component.libraries) {
        lib.isExternal = cachedInput.externalLibs.contains(lib.importUri);
      }
-      // We don't unbind as the root was unbound already. We do have to compute
-      // the canonical names though, to rebind everything in the component.
      component.adoptChildren();
      component.computeCanonicalNames();
      inputSummaries.add(component);
@ -158,7 +151,8 @@ Future<InitializedCompilerState> initializeIncrementalCompiler(
    WorkerInputComponent cachedInput = WorkerInputComponent(
        summaryDigest,
        await processedOpts.loadComponent(
-            await fileSystem.entityForUri(summary).readAsBytes(), nameRoot));
+            await fileSystem.entityForUri(summary).readAsBytes(), nameRoot,
+            alwaysCreateNewNamedNodes: true));
    workerInputCache[summary] = cachedInput;
    inputSummaries.add(cachedInput.component);
  }
--- a/pkg/front_end/lib/src/api_unstable/ddc.dart
+++ b/pkg/front_end/lib/src/api_unstable/ddc.dart
@ -180,11 +180,10 @@ Future<InitializedCompilerState> initializeIncrementalCompiler(
    for (var lib in cachedSdkInput.component.libraries) {
      lib.isExternal = false;
    }
+    cachedSdkInput.component.adoptChildren();
    for (WorkerInputComponent cachedInput in workerInputCache.values) {
      cachedInput.component.adoptChildren();
    }
-    cachedSdkInput.component.unbindCanonicalNames();
-    cachedSdkInput.component.computeCanonicalNames();

    // Reuse the incremental compiler, but reset as needed.
    incrementalCompiler = oldState.incrementalCompiler;
@ -219,9 +218,6 @@ Future<InitializedCompilerState> initializeIncrementalCompiler(
      for (var lib in component.libraries) {
        lib.isExternal = cachedInput.externalLibs.contains(lib.importUri);
      }
-      // We don't unbind as the root was unbound already. We do have to
-      // compute the canonical names though, to rebind everything in the
-      // component.
      component.computeCanonicalNames();
      doneInputSummaries[i] = component;
    }
@ -232,7 +228,9 @@ Future<InitializedCompilerState> initializeIncrementalCompiler(
    Uri summary = inputSummaries[index];
    List<int> data = await fileSystem.entityForUri(summary).readAsBytes();
    WorkerInputComponent cachedInput = WorkerInputComponent(
-        data, await compilerState.processedOpts.loadComponent(data, nameRoot));
+        data,
+        await compilerState.processedOpts
+            .loadComponent(data, nameRoot, alwaysCreateNewNamedNodes: true));
    workerInputCache[summary] = cachedInput;
    doneInputSummaries[index] = cachedInput.component;
  }
--- a/pkg/front_end/lib/src/base/processed_options.dart
+++ b/pkg/front_end/lib/src/base/processed_options.dart
@ -379,12 +379,16 @@ class ProcessedOptions {
  }

  /// Helper to load a .dill file from [uri] using the existing [nameRoot].
-  Component loadComponent(List<int> bytes, CanonicalName nameRoot) {
+  Component loadComponent(List<int> bytes, CanonicalName nameRoot,
+      {bool alwaysCreateNewNamedNodes}) {
    Component component =
        target.configureComponent(new Component(nameRoot: nameRoot));
    // TODO(ahe): Pass file name to BinaryBuilder.
    // TODO(ahe): Control lazy loading via an option.
-    new BinaryBuilder(bytes, filename: null, disableLazyReading: false)
+    new BinaryBuilder(bytes,
+            filename: null,
+            disableLazyReading: false,
+            alwaysCreateNewNamedNodes: alwaysCreateNewNamedNodes)
        .readComponent(component);
    return component;
  }
--- a/pkg/front_end/test/incremental_load_from_dill_test.dart
+++ b/pkg/front_end/test/incremental_load_from_dill_test.dart
@ -33,7 +33,15 @@ import 'package:front_end/src/fasta/severity.dart' show Severity;
 import 'package:kernel/binary/ast_from_binary.dart' show BinaryBuilder;

 import 'package:kernel/kernel.dart'
-    show Class, Component, EmptyStatement, Field, Library, Procedure;
+    show
+        Class,
+        Component,
+        EmptyStatement,
+        Field,
+        Library,
+        LibraryDependency,
+        Name,
+        Procedure;

 import 'package:kernel/target/targets.dart' show TargetFlags;

@ -294,8 +302,6 @@ Future<Null> newWorldTest(
      for (Component c in moduleComponents.values) {
        c.adoptChildren();
      }
-      sdk.unbindCanonicalNames();
-      sdk.computeCanonicalNames();

      modulesToUse = new List<Component>();
      for (String moduleName in world["modules"]) {
@ -310,7 +316,9 @@ Future<Null> newWorldTest(
        if (moduleComponent == null) {
          moduleComponent = new Component(nameRoot: sdk.root);
          new BinaryBuilder(moduleData[moduleName],
-                  filename: null, disableLazyReading: false)
+                  filename: null,
+                  disableLazyReading: false,
+                  alwaysCreateNewNamedNodes: true)
              .readComponent(moduleComponent);
          moduleComponents[moduleName] = moduleComponent;
          modulesToUse.add(moduleComponent);
@ -433,8 +441,8 @@ Future<Null> newWorldTest(
    Stopwatch stopwatch = new Stopwatch()..start();
    Component component = await compiler.computeDelta(
        entryPoints: entries,
-        fullComponent:
-            brandNewWorld ? false : (noFullComponent ? false : true));
+        fullComponent: brandNewWorld ? false : (noFullComponent ? false : true),
+        simulateTransformer: world["simulateTransformer"]);
    if (outlineOnly) {
      for (Library lib in component.libraries) {
        for (Class c in lib.classes) {
@ -459,6 +467,24 @@ Future<Null> newWorldTest(

    checkExpectedContent(world, component);

+    if (!noFullComponent) {
+      Set<Library> allLibraries = new Set<Library>();
+      for (Library lib in component.libraries) {
+        computeAllReachableLibrariesFor(lib, allLibraries);
+      }
+      if (allLibraries.length != component.libraries.length) {
+        Expect.fail("Expected for the reachable stuff to be equal to "
+            "${component.libraries} but it was $allLibraries");
+      }
+      Set<Library> tooMany = allLibraries.toSet()
+        ..removeAll(component.libraries);
+      if (tooMany.isNotEmpty) {
+        Expect.fail("Expected for the reachable stuff to be equal to "
+            "${component.libraries} but these were there too: $tooMany "
+            "(and others were missing)");
+      }
+    }
+
    newestWholeComponentData = util.postProcess(component);
    newestWholeComponent = component;
    print("*****\n\ncomponent:\n"
@ -542,7 +568,9 @@ Future<Null> newWorldTest(
      gotWarning = false;
      formattedWarnings.clear();
      Component component2 = await compiler.computeDelta(
-          entryPoints: entries, fullComponent: true);
+          entryPoints: entries,
+          fullComponent: true,
+          simulateTransformer: world["simulateTransformer"]);
      performErrorAndWarningCheck(
          world, gotError, formattedErrors, gotWarning, formattedWarnings);
      List<int> thisWholeComponent = util.postProcess(component2);
@ -576,6 +604,23 @@ Future<Null> newWorldTest(
  }
 }

+void computeAllReachableLibrariesFor(Library lib, Set<Library> allLibraries) {
+  Set<Library> libraries = new Set<Library>();
+  List<Library> workList = <Library>[];
+  allLibraries.add(lib);
+  libraries.add(lib);
+  workList.add(lib);
+  while (workList.isNotEmpty) {
+    Library library = workList.removeLast();
+    for (LibraryDependency dependency in library.dependencies) {
+      if (libraries.add(dependency.targetLibrary)) {
+        workList.add(dependency.targetLibrary);
+        allLibraries.add(dependency.targetLibrary);
+      }
+    }
+  }
+}
+
 void checkExpectedContent(YamlMap world, Component component) {
  if (world["expectedContent"] != null) {
    Map<String, Set<String>> actualContent = new Map<String, Set<String>>();
@ -858,7 +903,9 @@ class TestIncrementalCompiler extends IncrementalCompiler {

  @override
  Future<Component> computeDelta(
-      {List<Uri> entryPoints, bool fullComponent = false}) async {
+      {List<Uri> entryPoints,
+      bool fullComponent = false,
+      bool simulateTransformer}) async {
    Component result = await super
        .computeDelta(entryPoints: entryPoints, fullComponent: fullComponent);

@ -869,6 +916,39 @@ class TestIncrementalCompiler extends IncrementalCompiler {
      throw "Loaders builder should contain the sdk, "
          "but didn't even contain dart:core.";
    }
+
+    if (simulateTransformer == true) {
+      doSimulateTransformer(result);
+    }
    return result;
  }
 }
+
+void doSimulateTransformer(Component c) {
+  for (Library lib in c.libraries) {
+    if (lib.fields
+        .where((f) => f.name.name == "lalala_SimulateTransformer")
+        .toList()
+        .isNotEmpty) continue;
+    Name fieldName = new Name("lalala_SimulateTransformer");
+    Field field = new Field(fieldName,
+        isFinal: true,
+        reference: lib.reference.canonicalName
+            ?.getChildFromFieldWithName(fieldName)
+            ?.reference);
+    lib.addMember(field);
+    for (Class c in lib.classes) {
+      if (c.fields
+          .where((f) => f.name.name == "lalala_SimulateTransformer")
+          .toList()
+          .isNotEmpty) continue;
+      fieldName = new Name("lalala_SimulateTransformer");
+      field = new Field(fieldName,
+          isFinal: true,
+          reference: c.reference.canonicalName
+              ?.getChildFromFieldWithName(fieldName)
+              ?.reference);
+      c.addMember(field);
+    }
+  }
+}
--- a/pkg/front_end/testcases/incremental_initialize_from_dill/changing_modules_2.yaml
+++ b/pkg/front_end/testcases/incremental_initialize_from_dill/changing_modules_2.yaml
@ -0,0 +1,73 @@
+# Copyright (c) 2019, 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.md file.
+
+# Compile an application with a number of modules.
+# Compile again with changing modules.
+
+type: newworld
+strong: true
+modules:
+  example_1:
+    example_1/a.dart: |
+      a() {}
+      var example = true;
+    example_1/.packages: |
+      example:.
+  example_2:
+    example_2/a.dart: |
+      a() {}
+      var example = true;
+    example_2/.packages: |
+      example:.
+  foo:
+    foo/foo.dart: |
+      export "package:example/a.dart";
+      var foo = true;
+    foo/.packages: |
+      foo:.
+      example:../example_1
+worlds:
+  - entry: main.dart
+    fromComponent: true
+    sources:
+      main.dart: |
+        import "package:foo/foo.dart";
+        main() {
+          print("hello");
+          a();
+        }
+      .packages: |
+        foo:foo
+        example:example_1
+    modules:
+      - foo
+      - example_1
+    expectedLibraryCount: 3
+    expectedContent:
+      org-dartlang-test:///main.dart:
+        - Procedure main
+      package:foo/foo.dart:
+        - Field foo
+      package:example/a.dart:
+        - Procedure a
+        - Field example
+  - entry: main.dart
+    worldType: updated
+    expectInitializeFromDill: false
+    sources:
+      .packages: |
+        foo:foo
+        example:example_2
+    modules:
+      - foo
+      - example_2
+    expectedLibraryCount: 3
+    expectedContent:
+      org-dartlang-test:///main.dart:
+        - Procedure main
+      package:foo/foo.dart:
+        - Field foo
+      package:example/a.dart:
+        - Procedure a
+        - Field example
--- a/pkg/front_end/testcases/incremental_initialize_from_dill/changing_modules_3.yaml
+++ b/pkg/front_end/testcases/incremental_initialize_from_dill/changing_modules_3.yaml
@ -0,0 +1,64 @@
+# Copyright (c) 2019, 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.md file.
+
+# Compile an application with a number of modules.
+# Compile again with changing modules.
+
+type: newworld
+strong: true
+modules:
+  foo:
+    foo/foo.dart: |
+      var foo = true;
+    foo/.packages: |
+      foo:.
+  foo2:
+    foo2/foo.dart: |
+      var foo2 = true;
+    foo2/.packages: |
+      foo:.
+worlds:
+  - entry: main.dart
+    fromComponent: true
+    simulateTransformer: true
+    sources:
+      main.dart: |
+        import "package:foo/foo.dart";
+        main() {
+          print(foo);
+        }
+      .packages: |
+        foo:foo
+    modules:
+      - foo
+    expectedLibraryCount: 2
+    expectedContent:
+      org-dartlang-test:///main.dart:
+        - Procedure main
+        - Field lalala_SimulateTransformer
+      package:foo/foo.dart:
+        - Field foo
+        - Field lalala_SimulateTransformer
+  - entry: main.dart
+    worldType: updated
+    expectInitializeFromDill: false
+    simulateTransformer: true
+    sources:
+      main.dart: |
+        import "package:foo/foo.dart";
+        main() {
+          print(foo2);
+        }
+      .packages: |
+        foo:foo2
+    modules:
+      - foo2
+    expectedLibraryCount: 2
+    expectedContent:
+      org-dartlang-test:///main.dart:
+        - Procedure main
+        - Field lalala_SimulateTransformer
+      package:foo/foo.dart:
+        - Field foo2
+        - Field lalala_SimulateTransformer
--- a/pkg/kernel/lib/binary/ast_from_binary.dart
+++ b/pkg/kernel/lib/binary/ast_from_binary.dart
@ -76,14 +76,19 @@ class BinaryBuilder {
  List<String> debugPath = <String>[];

  bool _isReadingLibraryImplementation = false;
+  final bool alwaysCreateNewNamedNodes;

  /// If binary contains metadata section with payloads referencing other nodes
  /// such Kernel binary can't be read lazily because metadata cross references
  /// will not be resolved correctly.
  bool _disableLazyReading = false;

-  BinaryBuilder(this._bytes, {this.filename, disableLazyReading = false})
-      : _disableLazyReading = disableLazyReading;
+  BinaryBuilder(this._bytes,
+      {this.filename,
+      bool disableLazyReading = false,
+      bool alwaysCreateNewNamedNodes})
+      : _disableLazyReading = disableLazyReading,
+        this.alwaysCreateNewNamedNodes = alwaysCreateNewNamedNodes ?? false;

  fail(String message) {
    throw ParseError(message,
@ -803,6 +808,9 @@ class BinaryBuilder {
    var canonicalName = readCanonicalNameReference();
    Reference reference = canonicalName.getReference();
    Library library = reference.node;
+    if (alwaysCreateNewNamedNodes) {
+      library = null;
+    }
    bool shouldWriteData = library == null || _isReadingLibraryImplementation;
    if (library == null) {
      library =
@ -923,6 +931,9 @@ class BinaryBuilder {
    var canonicalName = readCanonicalNameReference();
    var reference = canonicalName.getReference();
    Typedef node = reference.node;
+    if (alwaysCreateNewNamedNodes) {
+      node = null;
+    }
    bool shouldWriteData = node == null || _isReadingLibraryImplementation;
    if (node == null) {
      node = new Typedef(null, null, reference: reference);
@ -968,6 +979,9 @@ class BinaryBuilder {
    var canonicalName = readCanonicalNameReference();
    var reference = canonicalName.getReference();
    Class node = reference.node;
+    if (alwaysCreateNewNamedNodes) {
+      node = null;
+    }
    bool shouldWriteData = node == null || _isReadingLibraryImplementation;
    if (node == null) {
      node = new Class(reference: reference)..level = ClassLevel.Temporary;
@ -1040,6 +1054,9 @@ class BinaryBuilder {
    var canonicalName = readCanonicalNameReference();
    var reference = canonicalName.getReference();
    Field node = reference.node;
+    if (alwaysCreateNewNamedNodes) {
+      node = null;
+    }
    bool shouldWriteData = node == null || _isReadingLibraryImplementation;
    if (node == null) {
      node = new Field(null, reference: reference);
@ -1079,6 +1096,9 @@ class BinaryBuilder {
    var canonicalName = readCanonicalNameReference();
    var reference = canonicalName.getReference();
    Constructor node = reference.node;
+    if (alwaysCreateNewNamedNodes) {
+      node = null;
+    }
    bool shouldWriteData = node == null || _isReadingLibraryImplementation;
    if (node == null) {
      node = new Constructor(null, reference: reference);
@ -1125,6 +1145,9 @@ class BinaryBuilder {
    var canonicalName = readCanonicalNameReference();
    var reference = canonicalName.getReference();
    Procedure node = reference.node;
+    if (alwaysCreateNewNamedNodes) {
+      node = null;
+    }
    bool shouldWriteData = node == null || _isReadingLibraryImplementation;
    if (node == null) {
      node = new Procedure(null, null, null, reference: reference);
@ -1184,6 +1207,9 @@ class BinaryBuilder {
    var canonicalName = readCanonicalNameReference();
    var reference = canonicalName.getReference();
    RedirectingFactoryConstructor node = reference.node;
+    if (alwaysCreateNewNamedNodes) {
+      node = null;
+    }
    bool shouldWriteData = node == null || _isReadingLibraryImplementation;
    if (node == null) {
      node = new RedirectingFactoryConstructor(null, reference: reference);
--- a/pkg/kernel/lib/canonical_name.dart
+++ b/pkg/kernel/lib/canonical_name.dart
@ -123,6 +123,10 @@ class CanonicalName {
        .getChildFromQualifiedName(member.name);
  }

+  CanonicalName getChildFromFieldWithName(Name name) {
+    return getChild('@fields').getChildFromQualifiedName(name);
+  }
+
  CanonicalName getChildFromTypedef(Typedef typedef_) {
    return getChild('@typedefs').getChild(typedef_.name);
  }
--- a/utils/bazel/kernel_worker.dart
+++ b/utils/bazel/kernel_worker.dart
@ -55,24 +55,11 @@ class KernelWorker extends AsyncWorkerLoop {
      } else {
        previousState = null;
      }
-      ComputeKernelResult result;
-      // TODO(vsm): See https://github.com/dart-lang/sdk/issues/36644.
-      // If the CFE is crashing with previous state, then clear compilation
-      // state and try again.
-      try {
-        result = await computeKernel(request.arguments,
-            isWorker: true,
-            outputBuffer: outputBuffer,
-            inputs: request.inputs,
-            previousState: previousStateToPass);
-      } catch (_) {
-        outputBuffer.clear();
-        result = await computeKernel(request.arguments,
-            isWorker: true,
-            outputBuffer: outputBuffer,
-            inputs: request.inputs,
-            previousState: null);
-      }
+      var result = await computeKernel(request.arguments,
+          isWorker: true,
+          outputBuffer: outputBuffer,
+          inputs: request.inputs,
+          previousState: previousStateToPass);
      previousState = result.previousState;
      if (!result.succeeded) {
        response.exitCode = 15;