[vm, aot] Ensure fixups applied after the world has been compiled are applied to closures in finall…

[vm, aot] Ensure fixups applied after the world has been compiled are applied to closures in finally clauses.

Finally clauses are inlined into try and catch blocks. This causes the parser to allocate at least two functions for any closure in a finally cause, only one of which will be registered by the flow graph builder. The unregistered function gets compiled, but the program visitor is unable to iterate it. To fix this, we use both the program visitor and the tree-shaker's work queue to apply fixups.

Fixes #30522

R=asiva@google.com, cbernaschina@google.com

Committed: https://github.com/dart-lang/sdk/commit/20002d14b1edeadb2c1da26eedf8c32acfab675c

[rmacnak, 2017-08-23 19:29:45]

rmacnak@google.com changed reviewers:
+ asiva@google.com, cbernaschina@google.com

[cbernaschina, 2017-08-23 19:37:40]

On 2017/08/23 19:29:46, rmacnak wrote:

lgtm

[siva, 2017-08-23 20:11:38]

lgtm

https://codereview.chromium.org/3003253002/diff/1/runtime/vm/precompiler.cc
File runtime/vm/precompiler.cc (right):

https://codereview.chromium.org/3003253002/diff/1/runtime/vm/precompiler.cc#n...
runtime/vm/precompiler.cc:2198: // This visit misses closures from duplicated
finally clauses.
Maybe the comment should be more clear , stating that two iterations are needed
to ensure we visit all functions as the ProgramVisitor's VisitFunctions does not
not visit closures from duplicated finally clauses and an iterations over the
treeshaker's queue does not visit functions compiled outside the queue.

What do you think?

https://codereview.chromium.org/3003253002/diff/1/runtime/vm/precompiler.cc#n...
runtime/vm/precompiler.cc:2296: // This visit misses closures from duplicated
finally clauses.
Ditto comment here.

[rmacnak, 2017-08-23 23:25:58]

https://codereview.chromium.org/3003253002/diff/1/runtime/vm/precompiler.cc
File runtime/vm/precompiler.cc (right):

https://codereview.chromium.org/3003253002/diff/1/runtime/vm/precompiler.cc#n...
runtime/vm/precompiler.cc:2198: // This visit misses closures from duplicated
finally clauses.
On 2017/08/23 20:11:37, siva wrote:
> Maybe the comment should be more clear , stating that two iterations are
needed
> to ensure we visit all functions as the ProgramVisitor's VisitFunctions does
not
> not visit closures from duplicated finally clauses and an iterations over the
> treeshaker's queue does not visit functions compiled outside the queue.
> 
> What do you think?

Yes, I like that better.

[rmacnak, 2017-08-23 23:42:43]

Description was changed from

==========
[vm, aot] Ensure fixups applied after the world has been compiled are applied to
closures in finally clauses.

Finally clauses are inlined into try and catch blocks. This causes the parser to
allocate at least two functions for any closure in a finally cause, only one of
which will be registered by the flow graph builder. The unregistered function
gets compiled, but the program visitor is unable to iterate it. To fix this, we
use both the program visitor and the tree-shaker's work queue to apply fixups.

Fixes #30522
==========

to

==========
[vm, aot] Ensure fixups applied after the world has been compiled are applied to
closures in finally clauses.

Finally clauses are inlined into try and catch blocks. This causes the parser to
allocate at least two functions for any closure in a finally cause, only one of
which will be registered by the flow graph builder. The unregistered function
gets compiled, but the program visitor is unable to iterate it. To fix this, we
use both the program visitor and the tree-shaker's work queue to apply fixups.

Fixes #30522

R=asiva@google.com, cbernaschina@google.com

Committed:
https://github.com/dart-lang/sdk/commit/20002d14b1edeadb2c1da26eedf8c32acfab675c
==========

[rmacnak, 2017-08-23 23:42:44]

Committed patchset #2 (id:20001) manually as
20002d14b1edeadb2c1da26eedf8c32acfab675c.