search

dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.25k stars 1.58k forks source link

The switch from Bytecode to AST has revealed differences in behavior which were approved #38782

Open mkustermann opened 5 years ago

mkustermann commented 5 years ago

It seems at least the following failures were approved after the change from Bytecode -> AST has landed in 8bf424ea89b956ada5f969db2880355285a311b0 / cl/120720.

language_2/regress_29025_test: Crash
co19_2/LanguageFeatures/Instantiate-to-bound/class/static/class_typedef_l1_t04/none: DartkCrash
co19_2/LanguageFeatures/Instantiate-to-bound/class/static/class_typedef_l2_t08/none: DartkCrash
language_2/covariant_subtyping_with_mixin_test: RuntimeError
language_2/type_alias_equality_test/03: RuntimeError
language_2/vm/bool_check_stack_traces_test/02: RuntimeError
service/evaluate_activation_test/instance: RuntimeError
vm/dart/il_round_trip_deserialization_test/5: Crash
vm/dart/il_round_trip_deserialization_test/7: Crash

These failures should be investigated and triaged.

(The failures have been approved - but I haven't found any github issue for it!)

/cc @mraleph @a-siva @alexmarkov

alexmarkov commented 5 years ago

These failures are among hundreds of other approved test failures (see test results dashboard for the full list), and they were failing before bytecode was enabled. Some of them are even mentioned in status files and originate before status-file free workflow. They just happened to be fixed by new bytecode pipeline due to various reasons (for example, bypassing certain bugs in AST serialization or flow graph construction). Is there any particular reason to look at them and not other failures, especially given that they will be fixed by switching to the new bytecode pipeline?

mkustermann commented 5 years ago

Just because they are seemingly passing on bytecode pipeline doesn't mean that there isn't a bug.

For example the vm/dart/il_round_trip* tests: They test functionality for serializing IR graphs out and reading them back in. They should not crash. The failures should be analyzed and triaged and not get ignored.

alexmarkov commented 5 years ago

Here is my understanding why some of these tests fail with AST but pass with bytecode:

language_2/regress_29025_test - most likely related to finalization of recursive types. In bytecode mode most of the type finalization machinery is skipped as types are finalized during bytecode generation.

co19_2/LanguageFeatures/Instantiate-to-bound/class/static/class_typedef_l1_t04/none, co19_2/LanguageFeatures/Instantiate-to-bound/class/static/class_typedef_l2_t04/none, co19_2/LanguageFeatures/Instantiate-to-bound/class/static/class_typedef_l2_t08/none - crash during kernel AST serialization. Crashed code is not used when bytecode is serialized.

language_2/covariant_subtyping_with_mixin_test - problem with how AST-based flow graph builder generates parameter type checks in forwarding stubs, tracked in https://github.com/dart-lang/sdk/issues/34321. Bytecode generator correctly handles this case.

language_2/type_alias_equality_test/03 - this test is for equality of function types. The underlying problem is tracked in https://github.com/dart-lang/sdk/issues/32783. It passes in bytecode as bytecode pipeline is more likely to canonicalize types.

language_2/vm/bool_check_stack_traces_test/02 - AST-based flow graph builder does not correctly assign token position in AssertBoolean check in the code like if (null). Bytecode generator is able to handle this case.

service/evaluate_activation_test/instance - this test is related to expression evaluation inside closures and tracked in https://github.com/dart-lang/sdk/issues/20047. This test passes in bytecode mode because this is captured in bytecode mode and it's available for expression evaluation.

vm/dart/il_round_trip_deserialization_test/5, vm/dart/il_round_trip_deserialization_test/7 - tracked in https://github.com/dart-lang/sdk/issues/36882. There are no comments saying that underlying problem is fixed after the comment https://github.com/dart-lang/sdk/issues/36882#issuecomment-530878161 about these failures. My guess is that flow graph constructed from bytecode is sufficiently different from the one constructed from AST and it doesn't trigger a bug in serialization.

mkustermann commented 5 years ago

Thank you very much for the analysis, @alexmarkov !

co19_2/LanguageFeatures/Instantiate-to-bound/class/static/class_typedef_l1_t04/none, crash during kernel AST serialization. Crashed code is not used when bytecode is serialized.

We should investigate this issue. I've filed https://github.com/dart-lang/sdk/issues/38812 for the CFE team. @johnniwinther already fixed the bug. Do we know why the bytecode compiler didn't barf when seeing a UnknownType?

vm/dart/il_round_trip_deserialization_test/7 tracked in #36882 There are no comments saying that underlying problem is fixed

Yes, but if the problem doesn't reproduce anymore with bytecode on-by-default it's hard to know the bug is still there. Attempting to reproduce it might fail and the issue might get closed.

At least we have to ping the github issue to say that turning bytecode on will make the issue disappear but the bug is probably still there.

alexmarkov commented 5 years ago

Do we know why the bytecode compiler didn't barf when seeing a UnknownType?

From the call stack of the crash it looks like UnknownType happened in the typedef declaration. typedef declarations are not serialized in bytecode - only the underlying function types.