VM: Crash while handling reference from an instance constant to a class which is not loaded yet #37533
Comments
alexmarkov
added
area-vm
|
Thanks Alex. Related to #36220 |
…oaded yet Issue: #37533 Issue: #36391 Change-Id: Ib1addecabd60c7de4387afe5f6b8d7cf856a548b Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/109103 Auto-Submit: Alexander Markov <[email protected]> Reviewed-by: Aart Bik <[email protected]> Commit-Queue: Alexander Markov <[email protected]>
|
Rolled to "6eb357c7fea8b49603941f2ea4e541405e3253bb" (Alex' informative message) and was able to reproduce. Thanks Alex! |
|
I have a very simple fix for this which we can apply right away (independent of the discussion whether moving the evaluation points will be beneficial for other reasons). Since the kernel_loader when evaluating a constant for the kernel loader, we can simply stop at instance constants for which the class declaration can never be external or pragma. This incidentally also allows me to remove the somewhat ad-hoc cyclic evaluation detection. |
|
Drive-by question: are we sure concatenated dills produced by build runner are valid with respect to (reverse) topological order that we are expecting from them? I would expected that dependencies are loaded before the other things that depend on them? |
Rationale: Only look for "external" and "pragma" to avoid cyclic or unloaded class evaluation #37533 #36220 Change-Id: I78ef5dcd2c7e7dee0d196394320e994f8aa8a695 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/109300 Commit-Queue: Aart Bik <[email protected]> Reviewed-by: Alexander Markov <[email protected]>
|
My question from #37533 (comment) still stands - in general the expectation is that we should not have concatenated dill files that are out of reverse topological order with respect to the dependencies. This really simplifies things. @grouma Gary do you happen to know whom we should talk to about Another question I have is why build_runner is concatenating dill files instead of using dill lists which is what is used in G3. I think @jensjoha mentioned to me that he sees gigabytes of space wasted on this when he runs Flutter tests. |
|
Concatenated dill files is not the only case where this issue arises, we have to also account for it in the Fuchsia '.dilp' situation where individual dil files are created for packages and loaded. |
|
@jakemac53 will be your best point of contact for I thought we included the topological logic externally but I couldn't find such logic in The concatenation logic is in the |
|
Actually it looks like we have a bug here where we add the root kernel file first https://github.com/dart-lang/build/blob/master/build_vm_compilers/lib/src/vm_entrypoint_builder.dart#L65. The rest of the transitive deps are in reverse dependency order though. I can do a quick fix and we can see if it resolves things? We also want to change to use the kernel manifest files soon instead of fully concatenated dills. |
|
@a-siva Ok so I tried changing the order but it turns out we did it in that order intentionally. The reason is that the vm will run the first kernel file that contains a Any ideas how we should work around this? |
|
@jakemac53 I think I am getting confused by terminology again (and the fact that we flipped the order in the implementation). The ordering which would make VM work is the following: such that if The main would be taken from the first |
|
OK, just to clarify, if you have 3 modules [ |
|
@jakemac53 correct. I think this is the order that we are getting internally. The loading code would then load them in reverse order. (so that |
|
@mraleph presumably this only applies to the manifest files though? We aren't using those yet. For a regular concatenated file it seems like we should be doing the reverse ordering ourselves? The issue with multiple |
|
@jakemac53 the ordering is the same for manifest files and concatenated files AFAIK. (manifest files do not really exist for internal loading purposes - outer layers simply perform concatenation in memory and give the concatenated dill file to the kernel loader).
|
|
OK - I can try changing the ordering internally and externally and see how it goes. I am surprised though that we haven't had more problems related to this before now...? |
…l instead of reverse topological, see dart-lang/sdk#37533
…l instead of reverse topological, see dart-lang/sdk#37533 (#2384)
|
I have updated the external build_vm_compilers to do the requested ordering, and published as version 1.0.2. It turns out that internal already had the correct ordering. |
This appears on the Flutter tests which use pub build_runner.
Repro:
flutter/tools/gn --full-dart-sdkninja -j 80 -C out/host_debugcd FLUTTER/packages/flutter_toolsrm -rf ../../bin/cache/dart-sdkln -s FLUTTER_ENGINE/src/out/host_debug/dart-sdk ../../bin/cache/dart-sdk../../bin/cache/dart-sdk/bin/pub run build_runner test -- -rcompact -j1 --no-colorThis crashes with
Stack trace:
Pub build_runner uses concatenated dill files, so multiple kernel components are loaded one by one. The problem is that when reading instance constant, class
Library:'package:meta/meta.dart' Class: _Experimental@49056369is not loaded yet (it is probably coming from a kernel component which is loaded after the current component). An attempt to finalize such class triggers its lazy loading. Normally, lazy loading of classes could happen only if class is loaded from bytecode, so bytecode loading is triggered fromsdk/runtime/vm/object.cc
Lines 3551 to 3564 in d0cb753
Bytecode is not actually used here. As flutter engine builds Dart VM without asserts they are not triggered until bytecode reading crashes.
With https://dart-review.googlesource.com/c/sdk/+/109103 applied a more informative error message is printed:
/cc @a-siva @aartbik
The text was updated successfully, but these errors were encountered: