Revert "Wrap the user entrypoint function in a zone with native exception callback. (#7512)" #7522

mraleph · 2019-01-17T10:31:20Z

This reverts commit 25559ed.

Reason for revert: broken in AOT mode.

@pragma('vm:entry-point') placed on a function only instructs
the compiler to retain the function itself, but does not tell
compiler to generate and retain tear-off for this function.

In this PR _runMainZoned was marked as an entry-point but C++
code was trying to tear it off and use a closure, instead of
invoking it directly, which is not supported.

@pragma

…tion callback. (flutter#7512)" This reverts commit 25559ed. Reason for revert: broken in AOT mode. @pragma('vm:entry-point') placed on a function only instructs the compiler to retain the function itself, but does not tell compiler to generate and retain tear-off for this function. In this PR _runMainZoned was marked as an entry-point but C++ code was trying to tear it off and use a closure, instead of invoking it directly, which is not supported.

mkustermann

If there is no current way of specifying that a global function is used as a tear-off from embedder, we should probably file an issue and address this?

mraleph · 2019-01-17T10:49:03Z

@mkustermann I thought about that but I am not sure if we actually want that. If you need to tear-off a function you can always write code that does that in pure Dart.

…ve exception callback. (flutter#7512)" (flutter/engine#7522)

d470fc6 Roll src/third_party/skia 31972f889641..57263c2e0ccd (3 commits) (flutter/engine#7529) 4acfced Ensure the ResourceContext is not ripped out from under dart (flutter/engine#7528) 270e9a7 Roll src/third_party/skia a4e46804946c..31972f889641 (8 commits) (flutter/engine#7526) e984372 Roll src/third_party/skia 33b4b4908b7a..a4e46804946c (1 commits) (flutter/engine#7524) 4c135c2 Revert "Wrap the user entrypoint function in a zone with native exception callback. (flutter/engine#7512)" (flutter/engine#7522) 0a080a1 Roll src/third_party/dart 700254996f..da09945643 (14 commits) da09945643 Update dartfix pubspec before publishing b81c1b2095 Parse spread collections in map literals 1b0d93366d Add list literal spread collection fasta test cases 6c2ea4936a Clean up several deprecation hints 07f95e7761 Revert "[vm] Enable timeline on Fuchsia even in product mode." 9cdce03e16 [dart2js] Improve null receiver guard removal near JS code c3599a9d8c Revert "[vm, isolate] Fix length truncation in message snapshots." 95e10c336f Revert "[vm] Assert callback state for all Dart_Set*ReturnValue." 3f7b371f2c [vm] Enable timeline on Fuchsia even in product mode. 4b22195ea1 [vm, isolate] Fix length truncation in message snapshots. a353b1172a [vm, compiler] Remove TAG_IC_DATA, which has since been subsumed by RebindRule. ca12afec50 [vm] Assert callback state for all Dart_Set*ReturnValue. 2028006a25 [Observatory] Updated Dart icon to new colour scheme 84273b9f36 Improvements for flow analysis. fff5377 Roll src/third_party/skia 25b9f192ed8c..33b4b4908b7a (1 commits) (flutter/engine#7520) a58cc39 Roll src/third_party/skia 1ce80fb351a2..25b9f192ed8c (5 commits) (flutter/engine#7517) 52e0e9d Roll src/third_party/skia 081e6f375497..1ce80fb351a2 (12 commits) (flutter/engine#7514) 25559ed Wrap the user entrypoint function in a zone with native exception callback. (flutter/engine#7512) 1b0d09b Roll src/third_party/dart f701e11756..700254996f (5 commits) 700254996f [ Observatory / Dartium ] Updated observatory documentation and tests to remove references to Dartium. 78abb98ee7 [vm/bytecode] Fix AST removal for package-split kernel files with bytecode 0075b58bb8 CHANGELOG entry for DEPRECATED_MEMBER_USE split a5f102a7d1 Analyzer: first pass at reporting unchecked nullable value usage. a10ddca1b1 [ VM / Service ] Allow for `profile_period` flag to be set via the service protocol e7ade51 Remove unused headers (flutter/engine#7511) 369b4db Roll src/third_party/skia 1374c85fbf53..081e6f375497 (6 commits) (flutter/engine#7510) 366d44e Roll src/third_party/dart 9b5eabdaca..f701e11756 (10 commits) f701e11756 [ VM / Debugger ] Fix issue where a 'Step' command issued when there's no stack caused a crash. c5bfccc6fb Make downloading the LSP spec a flag and commit the version parsed locally dbeec3bbf3 Fix formatting in generated LSP file header bfe15d87d8 Fix LSP exceptions serializing ResponseErrors with Uris 7984dc4fcc Prepare to publish analyzer version 0.34.2. 4b1b2f9176 Switch LSP formatter to not fetch resolved ASTs that aren't used 5ce5d697da Implement LSP code folding b47524d5b0 Ensure all unhandled exceptions are recorded on the server 8ba2de2344 Sort context for conflicting inherited members 215f6620e7 [Kernel] Signal errors on static fields in constant contexts

* Roll engine to d470fc6 d470fc6 Roll src/third_party/skia 31972f889641..57263c2e0ccd (3 commits) (flutter/engine#7529) 4acfced Ensure the ResourceContext is not ripped out from under dart (flutter/engine#7528) 270e9a7 Roll src/third_party/skia a4e46804946c..31972f889641 (8 commits) (flutter/engine#7526) e984372 Roll src/third_party/skia 33b4b4908b7a..a4e46804946c (1 commits) (flutter/engine#7524) 4c135c2 Revert "Wrap the user entrypoint function in a zone with native exception callback. (flutter/engine#7512)" (flutter/engine#7522) 0a080a1 Roll src/third_party/dart 700254996f..da09945643 (14 commits) da09945643 Update dartfix pubspec before publishing b81c1b2095 Parse spread collections in map literals 1b0d93366d Add list literal spread collection fasta test cases 6c2ea4936a Clean up several deprecation hints 07f95e7761 Revert "[vm] Enable timeline on Fuchsia even in product mode." 9cdce03e16 [dart2js] Improve null receiver guard removal near JS code c3599a9d8c Revert "[vm, isolate] Fix length truncation in message snapshots." 95e10c336f Revert "[vm] Assert callback state for all Dart_Set*ReturnValue." 3f7b371f2c [vm] Enable timeline on Fuchsia even in product mode. 4b22195ea1 [vm, isolate] Fix length truncation in message snapshots. a353b1172a [vm, compiler] Remove TAG_IC_DATA, which has since been subsumed by RebindRule. ca12afec50 [vm] Assert callback state for all Dart_Set*ReturnValue. 2028006a25 [Observatory] Updated Dart icon to new colour scheme 84273b9f36 Improvements for flow analysis. fff5377 Roll src/third_party/skia 25b9f192ed8c..33b4b4908b7a (1 commits) (flutter/engine#7520) a58cc39 Roll src/third_party/skia 1ce80fb351a2..25b9f192ed8c (5 commits) (flutter/engine#7517) 52e0e9d Roll src/third_party/skia 081e6f375497..1ce80fb351a2 (12 commits) (flutter/engine#7514) 25559ed Wrap the user entrypoint function in a zone with native exception callback. (flutter/engine#7512) 1b0d09b Roll src/third_party/dart f701e11756..700254996f (5 commits) 700254996f [ Observatory / Dartium ] Updated observatory documentation and tests to remove references to Dartium. 78abb98ee7 [vm/bytecode] Fix AST removal for package-split kernel files with bytecode 0075b58bb8 CHANGELOG entry for DEPRECATED_MEMBER_USE split a5f102a7d1 Analyzer: first pass at reporting unchecked nullable value usage. a10ddca1b1 [ VM / Service ] Allow for `profile_period` flag to be set via the service protocol e7ade51 Remove unused headers (flutter/engine#7511) 369b4db Roll src/third_party/skia 1374c85fbf53..081e6f375497 (6 commits) (flutter/engine#7510) 366d44e Roll src/third_party/dart 9b5eabdaca..f701e11756 (10 commits) f701e11756 [ VM / Debugger ] Fix issue where a 'Step' command issued when there's no stack caused a crash. c5bfccc6fb Make downloading the LSP spec a flag and commit the version parsed locally dbeec3bbf3 Fix formatting in generated LSP file header bfe15d87d8 Fix LSP exceptions serializing ResponseErrors with Uris 7984dc4fcc Prepare to publish analyzer version 0.34.2. 4b1b2f9176 Switch LSP formatter to not fetch resolved ASTs that aren't used 5ce5d697da Implement LSP code folding b47524d5b0 Ensure all unhandled exceptions are recorded on the server 8ba2de2344 Sort context for conflicting inherited members 215f6620e7 [Kernel] Signal errors on static fields in constant contexts

…ve exception callback. (flutter#7512)" (flutter/engine#7522)

chinmaygarde · 2019-01-19T03:04:18Z

I am not sure how I can achieve the effect of this patch without being able to closurize at least one method marked with vm:entry-point. If I don't get to closurize _runMainZoned and invoke it directly (certainly possible), I'll still need to closurize _startMainIsolate from dart:isolate. I am using _runMainZoned as a trampoline for the main function invocation. In Dart, this function cannot access _startMainIsolate because that is package private.

I was able to work around this by adding a method (also marked vm:entry-point) to isolate_patch.dart that closurizes the _startMainFunction call and returns the same to C++. C++ can then invoke it to get the handle that is passed off to the trampoline. This workaround seems hacky however.

Something else I don't understand about this limitation is how the closurization of the user entrypoint function currently works. That is not even marked with vm:entry-point. Why don't I run into the "unreachable code" assertion there as well. We also allow users to run arbitrary main entrypoints from arbitrary libraries. Are these broken in AOT too?

@a-siva is considering committing the hack I described to dart-lang/sdk. Other alternatives could also be to add another pragma that allows native code to clourize entrypoints. Or, just generate the code for all entrypoints. After all, they are probably not that many in there and this behavior is different from JIT mode operation and hard to catch at compile time.

Also CC @jason-simmons who was debugging this with us.

a-siva · 2019-01-19T03:11:23Z

I agree with @mkustermann's comment above about having a mechanism for doing a tear-off of a global function from the embedder.
For now I will add a method in dart:isolate_patch to tear off _startMainFunction so that this PR can land.

mraleph · 2019-01-21T12:30:35Z

@chinmaygarde main is treated specially.

For convenience we could provide @pragma('vm:entry-point', 'get') as a signal to generate tear-off. Filed an issue for this: dart-lang/sdk#35720

Or, just generate the code for all entrypoints. After all, they are probably not that many in there and this behavior is different from JIT mode operation and hard to catch at compile time.

We could check the impact of this solution.

/cc @sjindel-google

chinmaygarde · 2019-01-21T20:05:33Z

main is treated specially.

Thanks for clearing that up. Does this also mean that the support for running custom entrypoints in arbitrary libraries also currently broken for AOT? If so, I'll file a bug for that. I believe that code path is only used for AOT and some targets in Fuchsia. There are all presumably in the JIT mode.

mraleph · 2019-01-23T21:59:53Z

Does this also mean that the support for running custom entrypoints in arbitrary libraries also currently broken for AOT?

Does this support tear-off the main method or does it use Dart_Invoke? If it uses tear-off then it is broken and that means we should go ahead and implement dart-lang/sdk#35720.

dballesteros7 · 2019-02-06T13:11:08Z

At least with the Flutter Embedder in Android, we just hit the issue that @chinmaygarde mentioned in the comment above.

Adding a new entry point to a Flutter app as:

@pragma('vm:entry-point')
void otherEntryPoint() {
...
}

Resulted in a crash with /DartVM: ../../third_party/dart/runtime/vm/object.cc: 7146: error: unreachable code

@mkustermann helped me track down the point where the embedder tries to tear-off this function which fails in AoT mode because there's no implicit closure function created by the compiler. flutter/engine/dart_isolate.cc

I'd be great to get dart-lang/sdk#35720 so Flutter apps can define entry points without hitting this cryptic error.

sjindel-google · 2019-02-06T13:20:29Z

@dballesteros7: How did you discover the "@pragma("vm:entry-point")" annotation? Did you read the documentation in runtime/docs/compiler/aot/entry_point_pragma.md?

dballesteros7 · 2019-02-06T13:22:32Z

We asked internally on how to prevent a function from being tree-shaken by AOT and were pointed to that annotation.

I haven't read the documentation.

* Roll engine to d470fc6 d470fc6 Roll src/third_party/skia 31972f889641..57263c2e0ccd (3 commits) (flutter/engine#7529) 4acfced Ensure the ResourceContext is not ripped out from under dart (flutter/engine#7528) 270e9a7 Roll src/third_party/skia a4e46804946c..31972f889641 (8 commits) (flutter/engine#7526) e984372 Roll src/third_party/skia 33b4b4908b7a..a4e46804946c (1 commits) (flutter/engine#7524) 4c135c2 Revert "Wrap the user entrypoint function in a zone with native exception callback. (flutter/engine#7512)" (flutter/engine#7522) 0a080a1 Roll src/third_party/dart 700254996f..da09945643 (14 commits) da09945643 Update dartfix pubspec before publishing b81c1b2095 Parse spread collections in map literals 1b0d93366d Add list literal spread collection fasta test cases 6c2ea4936a Clean up several deprecation hints 07f95e7761 Revert "[vm] Enable timeline on Fuchsia even in product mode." 9cdce03e16 [dart2js] Improve null receiver guard removal near JS code c3599a9d8c Revert "[vm, isolate] Fix length truncation in message snapshots." 95e10c336f Revert "[vm] Assert callback state for all Dart_Set*ReturnValue." 3f7b371f2c [vm] Enable timeline on Fuchsia even in product mode. 4b22195ea1 [vm, isolate] Fix length truncation in message snapshots. a353b1172a [vm, compiler] Remove TAG_IC_DATA, which has since been subsumed by RebindRule. ca12afec50 [vm] Assert callback state for all Dart_Set*ReturnValue. 2028006a25 [Observatory] Updated Dart icon to new colour scheme 84273b9f36 Improvements for flow analysis. fff5377 Roll src/third_party/skia 25b9f192ed8c..33b4b4908b7a (1 commits) (flutter/engine#7520) a58cc39 Roll src/third_party/skia 1ce80fb351a2..25b9f192ed8c (5 commits) (flutter/engine#7517) 52e0e9d Roll src/third_party/skia 081e6f375497..1ce80fb351a2 (12 commits) (flutter/engine#7514) 25559ed Wrap the user entrypoint function in a zone with native exception callback. (flutter/engine#7512) 1b0d09b Roll src/third_party/dart f701e11756..700254996f (5 commits) 700254996f [ Observatory / Dartium ] Updated observatory documentation and tests to remove references to Dartium. 78abb98ee7 [vm/bytecode] Fix AST removal for package-split kernel files with bytecode 0075b58bb8 CHANGELOG entry for DEPRECATED_MEMBER_USE split a5f102a7d1 Analyzer: first pass at reporting unchecked nullable value usage. a10ddca1b1 [ VM / Service ] Allow for `profile_period` flag to be set via the service protocol e7ade51 Remove unused headers (flutter/engine#7511) 369b4db Roll src/third_party/skia 1374c85fbf53..081e6f375497 (6 commits) (flutter/engine#7510) 366d44e Roll src/third_party/dart 9b5eabdaca..f701e11756 (10 commits) f701e11756 [ VM / Debugger ] Fix issue where a 'Step' command issued when there's no stack caused a crash. c5bfccc6fb Make downloading the LSP spec a flag and commit the version parsed locally dbeec3bbf3 Fix formatting in generated LSP file header bfe15d87d8 Fix LSP exceptions serializing ResponseErrors with Uris 7984dc4fcc Prepare to publish analyzer version 0.34.2. 4b1b2f9176 Switch LSP formatter to not fetch resolved ASTs that aren't used 5ce5d697da Implement LSP code folding b47524d5b0 Ensure all unhandled exceptions are recorded on the server 8ba2de2344 Sort context for conflicting inherited members 215f6620e7 [Kernel] Signal errors on static fields in constant contexts

mkustermann · 2019-02-18T08:48:09Z

I agree with @mkustermann's comment above about having a mechanism for doing a tear-off of a global function from the embedder.

@chinmaygarde This has been implemented now in dart-lang/sdk@ae7bf9e. The default is to allow closurization of the procedure.
-> This increased the code size of flutter apps slightly. For those entry points where you don't need support for closurization you can use the more fine-grained annotation, please see entry_point_pragma.md.

sjindel-google · 2019-02-18T12:31:34Z

In addition, I'm working on using the more fine-grained annotation for existing entry-points in Flutter and Dart SDK to reduce the regression.

googlebot added cla: yes labels Jan 17, 2019

mraleph requested review from chinmaygarde and mkustermann January 17, 2019 10:31

mkustermann approved these changes Jan 17, 2019

View reviewed changes

mraleph merged commit 4c135c2 into flutter:master Jan 17, 2019

mraleph deleted the revert-crash branch January 17, 2019 10:49

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 17, 2019

4c135c2 Revert "Wrap the user entrypoint function in a zone with nati…

a6745b5

…ve exception callback. (flutter#7512)" (flutter/engine#7522)

engine-flutter-autoroll mentioned this pull request Jan 17, 2019

Roll engine 1e93a8eb39d7..4c135c298a81 (10 commits) flutter/flutter#26707

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 17, 2019

4c135c2 Revert "Wrap the user entrypoint function in a zone with nati…

cf5e42c

…ve exception callback. (flutter#7512)" (flutter/engine#7522)

engine-flutter-autoroll mentioned this pull request Jan 17, 2019

Roll engine 1e93a8eb39d7..e98437248f02 (11 commits) flutter/flutter#26710

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 17, 2019

4c135c2 Revert "Wrap the user entrypoint function in a zone with nati…

a359629

…ve exception callback. (flutter#7512)" (flutter/engine#7522)

engine-flutter-autoroll mentioned this pull request Jan 17, 2019

Roll engine 1e93a8eb39d7..270e9a78a685 (12 commits) flutter/flutter#26719

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 17, 2019

4c135c2 Revert "Wrap the user entrypoint function in a zone with nati…

35c294d

…ve exception callback. (flutter#7512)" (flutter/engine#7522)

engine-flutter-autoroll mentioned this pull request Jan 17, 2019

Roll engine 1e93a8eb39d7..4acfced383b6 (13 commits) flutter/flutter#26726

Closed

dnfield mentioned this pull request Jan 17, 2019

Roll engine to d470fc65ea1cb91ae66706b320d82c4536a4da8b flutter/flutter#26727

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 18, 2019

4c135c2 Revert "Wrap the user entrypoint function in a zone with nati…

8672deb

…ve exception callback. (flutter#7512)" (flutter/engine#7522)

engine-flutter-autoroll mentioned this pull request Jan 18, 2019

Roll engine 1e93a8eb39d7..d470fc65ea1c (14 commits) flutter/flutter#26731

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 18, 2019

4c135c2 Revert "Wrap the user entrypoint function in a zone with nati…

4412a5a

…ve exception callback. (flutter#7512)" (flutter/engine#7522)

engine-flutter-autoroll mentioned this pull request Jan 18, 2019

Roll engine 1e93a8eb39d7..d470fc65ea1c (14 commits) flutter/flutter#26732

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 18, 2019

4c135c2 Revert "Wrap the user entrypoint function in a zone with nati…

8e92fc8

…ve exception callback. (flutter#7512)" (flutter/engine#7522)

engine-flutter-autoroll mentioned this pull request Jan 18, 2019

Roll engine 1e93a8eb39d7..d470fc65ea1c (14 commits) flutter/flutter#26733

Closed

Revert "Wrap the user entrypoint function in a zone with native exception callback. (#7512)" #7522

Revert "Wrap the user entrypoint function in a zone with native exception callback. (#7512)" #7522

Uh oh!

Conversation

mraleph commented Jan 17, 2019

Uh oh!

mkustermann left a comment

Choose a reason for hiding this comment

Uh oh!

mraleph commented Jan 17, 2019

Uh oh!

chinmaygarde commented Jan 19, 2019

Uh oh!

a-siva commented Jan 19, 2019

Uh oh!

mraleph commented Jan 21, 2019

Uh oh!

chinmaygarde commented Jan 21, 2019

Uh oh!

mraleph commented Jan 23, 2019

Uh oh!

dballesteros7 commented Feb 6, 2019

Uh oh!

sjindel-google commented Feb 6, 2019

Uh oh!

dballesteros7 commented Feb 6, 2019

Uh oh!

mkustermann commented Feb 18, 2019

Uh oh!

sjindel-google commented Feb 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants