Summary:
1. Previously, in 'BuildGraphOfConvertedClosureFunction', the VM was unable to
correctly forward parameters to converted closure functions when
they were captured in the converted function's body. This could happen when, for
example, a closure was introduced into it by async conversion.
Now, this is fixed by an approach that mirrors the technique in
'BuildGraphOfFunction'.
2. Previously, local variables declared inside loop bodies were being saved in
the loop's enclosing context, so closures within the loop would see new
values initialized to the variable in subsequent iterations.
Now, this is fixed by creating nested contexts for all loops, regardless of
whether the loop variables are captured.
3. Previously, arity checks were not being performed on converted closures, so
they could be called with too few or too many arguments. In the former case, the
missing arguments would be filled in with garbage on the stack.
Now, the assembly generation in 'CompileGraph' inserts argument count checks
for converted closures as well as regular closures.
Test Plan:
Introduced new tests in the closure conversion suite to test each bug:
1. syncstart.dart
2. loop2.dart, blocks.dart, updated for_in_closure.dart
3. arity.dart
With these changes, closure conversion passes all co19 tests in non-checked mode, except those that are not passed without it:
python tools/test.py -m release -c dartk --vm-options "--reify --reify_generic_functions" co19
BUG=
R=dmitryas@google.com
Review-Url: https://codereview.chromium.org/3000333002 .
- Evaluation of PropertyGet expression by accessing a field,
execution of a getter or creating a method tearoff.
- Evaluation of PropertySet expression by setting a field or
execution of a setter.
BUG=
R=dmitryas@google.com
Review-Url: https://codereview.chromium.org/2999673002 .
- Evaluation of StaticPropertyGet expression by reading a value stored in
the main environment, execution of static getter or creating a method tear off.
- Evaluation of StaticPropertySet expression by modifying the value
stored in main environment or execution of static setter.
BUG=
R=dmitryas@google.com, kmillikin@google.com
Review-Url: https://codereview.chromium.org/2997563002 .
Previously, the captured variable analysis did not provide sufficient
information for the conversion phase regarding variable uses in initalizers: in
particular, it did not differentiate the case when a variable is used in an
initializer and captured in the body vs. being captured in the body and not used
in an initializer. In addition, there were a few bugs stemming from the use
of lazy iterables and OR conjunctives with effectful operations.
Now, we separate the information about which variables are captured from flags
indicating whether variables are used in initializers. The other bugs are fixed
in obvious ways.
Finally, we reintroduce some code that ensures that redirecting factory
constructors listed in "_redirecting#" field (a hack used when writing DILL
files) remain with one-expression bodies after closure conversion.
Test Plan:
Added a test case for the initializers bug, ensured that the patched SDK builds
with closure conversion always-on.
Reviewers: dmitryas@google.com
BUG=
R=dmitryas@google.com
Review-Url: https://codereview.chromium.org/2995083002 .
Summary:
Previously, there was no support for generic methods in kernel. This prevented
us from being able to pass captured type arguments to the target top-level
function in converted closures, so these type arguments were always instantiated
to 'dynamic'.
Now, we save the type arguments to the closure creation operation in the
context, and read them out and forward them appropriately in closure wrapper
function. Since fasta doesn't currently support generic methods (their type
parameters are replaced by 'dynamic'), only top-level generic functions can
surface in kernel, as they are generated by closure conversion of closures that
capture type parameters of a class.
My focus here is enabling closure conversion to work in only these cases, and as
such, the code has some temporary "hacks" in the VM that may not work for
generic member functions or generic closures when they are enabled in fasta.
Test Plan:
I ran all the tests in closures/, and those which were previously expected to
crash due to missing VM support now pass and produce correct results.
Further testing is paused until we understand why the recent commit "[kernel]
Insert kernel bodies into VM heap" has broken all these tests.
Reviewers: regis@google.com, jensj@google.com, dmitryas@google.com
BUG=
R=dmitryas@google.com, jensj@google.com
Review-Url: https://codereview.chromium.org/2998803002 .
This CL copies the kernel bodies for all functions and
fields into the VM heap. The function bodies in the VM
heap are then used when compiling the flowgraphs.
This theoretically means that the malloc'd data can be
freed and that snapshotting from kernel could possibly
work, though it hasn't been tested.
R=kmillikin@google.com
Review-Url: https://codereview.chromium.org/2972343002 .
Prior to this CL we carried around information about the containing class
and member, both of which was fetched by reading out-of-line in the binary
(i.e. while reading the current member, start reading something from the
parent member etc).
It had also required the introduction of extra fields in the kernel
binary file (dill file).
This CL cleans that up, by
a) Setting type parameters on functions as needed (in kernel_reader.cc)
b) Using the VM Class and VM Function to get the required information
(with a above the information is all available).
(in kernel_binary_flowgraph.cc.) This means that
c) We don't have to read the binary out-of-line (for TypeParameterType
to work at least), and that
d) We can remove the previously introduced extra fields from the
kernel binary file (dill file).
R=dmitryas@google.com, kmillikin@google.com
Review-Url: https://codereview.chromium.org/2973633002 .
Summary:
Previously, the VM would crash when it encountered a ClosureCreation node
because it was not aware of the new type arguments field.
Now, it skips the type arguemnts field, which allows many tests to pass again,
even though it doesn't correct forward the type arguments at runtime.
Test Plan:
Removed expected failure lines for all the tests added in my prior
CL (introducing the new field).
BUG=
R=dmitryas@google.com, jensj@google.com
Reviewers: dmitryas@google.com
Review-Url: https://codereview.chromium.org/2987143002 .
Summary:
Previously, when a function parameter was captured both in an initializer and in
the body of a constructor, we would create two contexts: one created in a local
initializer and used by other initializers, and another in the body of the
function. This is incorrect, as it means that changes to the parameter in the
initializer's closure won't be visible in the body.
Now, to work around this problem we re-use the context created for the
initializers in the body of the constructor by moving the body into a new
constructor, and redirecting the original constructor to that one, passing the
context as an additional argument. This dance is necessary because local
initializers aren't visible in the body of a constructor.
Test Plan:
A few of the existing closure conversion tests were changed or fixed by this
revision. We also modify the 'closure_in_initializer.dart' test to hit this case
directly.
R=dmitryas@google.com
Reviewers: dmitryas@google.com
Review-Url: https://codereview.chromium.org/2991853002 .
Summary:
Previously, we filled in all occurrences of captured type variables with either
"dynamic" or their bound, if they had one.
Now, we add extra type parameters to the top-level function corresponding to the
closure, and pass in the corresponding arguments as type arguments to the
"MakeClosure" operation.
Test Plan:
Updated [type_variables.dart] and added a new test case to it.
R=dmitryas@google.com
Review-Url: https://codereview.chromium.org/2989563002 .
The closure-conversion transformation is not enabled yet. This commit
only adds the support for it to FlowGraphBuilder and
StreamingFlowGraphBuilder. More work should be done before enabling the
transformation; most mportantly, the 'platform.dill' file that is used
in the Kernel isolate and is loaded by VM for linking with executed
programs should be separated. The former should receive a file not
touched by the transformation, and the latter should receive a
transformed one.
BUG=
R=jensj@google.com, karlklose@google.com, kustermann@google.com
Review-Url: https://codereview.chromium.org/2891053003 .
It is informative in the same way as documentation.
Yesterday I tried to make Analyzer and Analysis Server stop using
FieldFormalParameterElement(s) and found one important use case requested
by the Flutter team. We need to show documentation of a field when
user requests documentation on the corresponding field formal named
parameter. This happens outside of the file that declares the parameter
and the field, so we don't have AST available. Of course we could
resolve the unit, but it would cost us something. And if we decide
that it is OK, then maybe we don't need to have documenation comments
in elements at all.
This is of course more convenience, and we could store documentation
and parameter to field mappings outside, like we store index. Just a
compromise - convenience vs. purity.
R=ahe@google.com, kmillikin@google.com, paulberry@google.com, sigmund@google.com
BUG=
Review-Url: https://codereview.chromium.org/2983173002 .
This review scraps the (currently disabled) code for converting tearoffs in the
closure conversion pass.
The closure conversion pass can only ever do a partial job with tearoffs, due to
the possibility of an unconverted library tearing off a method from any object
it likes. Partially converting [PropertyGet]s makes the closure conversion pass
slower and introduces a new method for any field or method anywhere with a name
used in any [PropertyGet], inflating code size and potentially regressing
performance. As it provides no concrete value in return we've decided to scrap
this aspect of the transformation. Anyway, creating closures for tearoffs is
much easier for a backend than converting anonymous or nested functions, since
there is only one object ("this") captured. Thus ignoring tear-offs does not
undermine the value of the transformation.
BUG=
R=dmitryas@google.com
Review-Url: https://codereview.chromium.org/2986553002 .
Jens Johansen