Re: Change in sdk[master]: [VM] Support patching of fields.

3 views
Skip to first unread message

Vyacheslav Egorov

unread,
Jan 18, 2018, 2:42:35 PM1/18/18
to change...@dart-review.googlesource.com, vm-...@dartlang.org, Siva Annamalai, commi...@chromium.org, Dart Reviews
I would talk to Peter tomorrow about this (patched_sdk is a legacy way of patching thing. we should have already switched to fasta for generating patched SDK).

It would help if you could upload some sort of reproduction patch of what you are trying to patch which causes it to break like this so that we have something as a reference. 


// Vyacheslav Egorov

On Thu, Jan 18, 2018 at 8:14 PM, Régis Crelier (Gerrit) <noreply-gerritcoderevie...@google.com> wrote:

Patch Set 3:

DBC: It might be good to test if this sort of patching already works with CFE. Because if it does not then Dart 2 builds will break.

Good call!
If I try to patch a field in the core lib, CFE complains:

FAILED: patched_sdk/lib/libraries.json
python ../../tools/patch_sdk.py --quiet vm /usr/local/google/home/regis/dart4/sdk/sdk /usr/local/google/home/regis/dart4/sdk/out/DebugX64/gen/runtime/vm/patches patched_sdk /usr/local/google/home/regis/dart4/sdk/.packages
Unhandled exception:
NoSuchMethodError: Class 'FieldDeclarationImpl' has no instance getter 'name'.
Receiver: Instance of 'FieldDeclarationImpl'
Tried calling: name
#0 Object.noSuchMethod (dart:core-patch/dart:core/object_patch.dart:46)
#1 _qualifiedName (file:///usr/local/google/home/regis/dart4/sdk/tools/patch_sdk.dart:653:32)
#2 PatchFinder.visitClassDeclaration (file:///usr/local/google/home/regis/dart4/sdk/tools/patch_sdk.dart:621:19)
#3 ClassDeclarationImpl.accept (package:analyzer/src/dart/ast/ast.dart:1906:49)
#4 NodeListImpl.accept (package:analyzer/src/dart/ast/ast.dart:8050:20)
#5 CompilationUnitImpl.visitChildren (package:analyzer/src/dart/ast/ast.dart:2502:21)
#6 GeneralizingAstVisitor.visitNode (package:analyzer/dart/ast/visitor.dart:431:10)
#7 GeneralizingAstVisitor.visitCompilationUnit (package:analyzer/dart/ast/visitor.dart:219:51)
#8 new PatchFinder.parseAndVisit (file:///usr/local/google/home/regis/dart4/sdk/tools/patch_sdk.dart:607:5)
#9 _patchLibrary (file:///usr/local/google/home/regis/dart4/sdk/tools/patch_sdk.dart:455:27)
#10 _applyPatch (file:///usr/local/google/home/regis/dart4/sdk/tools/patch_sdk.dart:412:20)
#11 _main (file:///usr/local/google/home/regis/dart4/sdk/tools/patch_sdk.dart:118:5)
<asynchronous suspension>
#12 main (file:///usr/local/google/home/regis/dart4/sdk/tools/patch_sdk.dart:62:11)
<asynchronous suspension>
#13 _startIsolate.<anonymous closure> (dart:isolate-patch/dart:isolate/isolate_patch.dart:275)
#14 _RawReceivePortImpl._handleMessage (dart:isolate-patch/dart:isolate/isolate_patch.dart:163)

What is your feeling? Is it difficult to support field patching in the CFE? If so, we should replace the BigInt fields we need to patch with getters and forget about field patching.

View Change

    To view, visit change 35384. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: sdk
    Gerrit-Branch: master
    Gerrit-MessageType: comment
    Gerrit-Change-Id: Ida19f3542b24899a306cce0c5a1ee625f578e675
    Gerrit-Change-Number: 35384
    Gerrit-PatchSet: 3
    Gerrit-Owner: Régis Crelier <re...@google.com>
    Gerrit-Reviewer: Régis Crelier <re...@google.com>
    Gerrit-Reviewer: Siva Annamalai <as...@google.com>
    Gerrit-CC: Dart Reviews <rev...@dartlang.org>
    Gerrit-CC: Vyacheslav Egorov <veg...@google.com>
    Gerrit-Comment-Date: Thu, 18 Jan 2018 19:14:52 +0000
    Gerrit-HasComments: No
    Gerrit-HasLabels: No

    Siva Annamalai

    unread,
    Jan 18, 2018, 8:58:46 PM1/18/18
    to Régis Crelier, vm-...@dartlang.org, Vyacheslav Egorov, commi...@chromium.org, Dart Reviews
    ​The current version you have (does not require field patching) seems easier as we don't have an additional requirement from the FE team (they already have a number of issues to work on)​. Why don't we go with it.

    On Thu, Jan 18, 2018 at 5:30 PM, Régis Crelier (Gerrit) <noreply-gerritcoderevie...@google.com> wrote:

    Patch Set 3:

    Slava,

    As you requested, I uploaded a cl containing both this cl and an example of field patching that fails under CFE, but works under the VM:
    https://dart-review.googlesource.com/#/c/sdk/+/35804

    The new file runtime/lib/bigint_patch.dart is simply a copy of sdk/lib/core/bigint.dart where private classes named "_BigInt..." are renamed to "_BigInteger..." and where public class BigInt is patched to refer to the renamed class _BigIntegerImpl.
    This is only an example and not to be committed.

    Thanks,
    Regis

    In case we decide not to support field patching, we can solve the issue by using getters. This cl (https://dart-review.googlesource.com/#/c/sdk/+/35521) shows how to do that and how to provide different implementations of the new BigInt class for each of the VM, dart2js, and devc.

    View Change

      To view, visit change 35384. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: sdk
      Gerrit-Branch: master
      Gerrit-MessageType: comment
      Gerrit-Change-Id: Ida19f3542b24899a306cce0c5a1ee625f578e675
      Gerrit-Change-Number: 35384
      Gerrit-PatchSet: 3
      Gerrit-Owner: Régis Crelier <re...@google.com>
      Gerrit-Reviewer: Régis Crelier <re...@google.com>
      Gerrit-Reviewer: Siva Annamalai <as...@google.com>
      Gerrit-CC: Dart Reviews <rev...@dartlang.org>
      Gerrit-CC: Vyacheslav Egorov <veg...@google.com>
      Gerrit-Comment-Date: Fri, 19 Jan 2018 01:30:20 +0000
      Gerrit-HasComments: No
      Gerrit-HasLabels: No

      Régis Crelier

      unread,
      Jan 18, 2018, 10:17:10 PM1/18/18
      to Siva Annamalai, vm-...@dartlang.org, Vyacheslav Egorov, commi...@chromium.org, Dart Reviews
      Sounds good to me.

      On Thu, Jan 18, 2018 at 5:58 PM, Siva Annamalai <as...@google.com> wrote:
      ​The current version you have (does not require field patching) seems easier as we don't have an additional requirement from the FE team (they already have a number of issues to work on)​. Why don't we go with it.
      Reply all
      Reply to author
      Forward
      0 new messages