Background compiler should validate CHA decisions before committing the code.

Background compiler should validate CHA decisions before committing the code.

CHA::HasOverride skips non-finalized classes when looking for overrides which means that we will install incorrect code if some subclass with an override was finalized while compilation was in progress.

To catch situations like this we record the number of finalized subclasses that class had
when CHA made the first negative decision about it (e.g. that it has no subclasses or that it has no overrides for some function) and before installing the code we check that number of subclasses matches.

Additionally renamed "leaf classes" to "guarded classes" because those classes are not necessarily leaf.

R=fschneider@google.com
BUG=

Committed: https://github.com/dart-lang/sdk/commit/6bb73bd2e41f9847e4b7a24bbd4d50d41a6536cf

[Florian Schneider, 2016-05-20 11:23:28]

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

https://codereview.chromium.org/2002583002/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:2070:
thread()->cha()->AddToGuardedClasses(type_class, /*subclass_count=*/0);
You can remove this, or add ASSERT(!FLAG_use_cha_deopt) here. This should never
be reached, since use_cha_deopt is hardcoded to false for AOT.

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

https://codereview.chromium.org/2002583002/diff/1/runtime/vm/cha.cc#newcode113
runtime/vm/cha.cc:113: if (is_finalized) count++;
else continue?

Non-finalized class can't have finalized subclasses.

https://codereview.chromium.org/2002583002/diff/1/runtime/vm/cha.cc#newcode163
runtime/vm/cha.cc:163: if (HasOverride(direct_subclass, function_name,
subclasses_count)) {
Same here: No need to recurse if !is_finalized.

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

https://codereview.chromium.org/2002583002/diff/1/runtime/vm/flow_graph.cc#ne...
runtime/vm/flow_graph.cc:454: thread()->cha()->AddToGuardedClasses(cls,
subclass_count);
if (FLAG_use_cha_deopt) {
  ...
}

[Vyacheslav Egorov (Google), 2016-05-20 11:31:21]

PTAL

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

https://codereview.chromium.org/2002583002/diff/1/runtime/vm/aot_optimizer.cc...
runtime/vm/aot_optimizer.cc:2070:
thread()->cha()->AddToGuardedClasses(type_class, /*subclass_count=*/0);
On 2016/05/20 11:23:28, Florian Schneider wrote:
> You can remove this, or add ASSERT(!FLAG_use_cha_deopt) here. This should
never
> be reached, since use_cha_deopt is hardcoded to false for AOT.

Done.

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

https://codereview.chromium.org/2002583002/diff/1/runtime/vm/cha.cc#newcode113
runtime/vm/cha.cc:113: if (is_finalized) count++;
On 2016/05/20 11:23:28, Florian Schneider wrote:
> else continue?
> 
> Non-finalized class can't have finalized subclasses.

Done.

https://codereview.chromium.org/2002583002/diff/1/runtime/vm/cha.cc#newcode163
runtime/vm/cha.cc:163: if (HasOverride(direct_subclass, function_name,
subclasses_count)) {
On 2016/05/20 11:23:28, Florian Schneider wrote:
> Same here: No need to recurse if !is_finalized.

Done.

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

https://codereview.chromium.org/2002583002/diff/1/runtime/vm/flow_graph.cc#ne...
runtime/vm/flow_graph.cc:454: thread()->cha()->AddToGuardedClasses(cls,
subclass_count);
On 2016/05/20 11:23:28, Florian Schneider wrote:
> if (FLAG_use_cha_deopt) {
>   ...
> }

already checked above.

[Florian Schneider, 2016-05-20 11:35:42]

LGTM!

[Vyacheslav Egorov (Google), 2016-05-20 11:48:32]

Description was changed from

==========
Background compiler should validate CHA decisions before committing the code.

CHA::HasOverride skips non-finalized classes when looking for overrides which
means that we will install incorrect code if some subclass with an override was
finalized while compilation was in progress.

To catch situations like this we record the number of finalized subclasses that
class had
when CHA made the first negative decision about it (e.g. that it has no
subclasses or that it has no overrides for some function) and before installing
the code we check that number of subclasses matches.

Additionally renamed "leaf classes" to "guarded classes" because those classes
are not necessarily leaf.

R=fschneider@google.com
BUG=
==========

to

==========
Background compiler should validate CHA decisions before committing the code.

CHA::HasOverride skips non-finalized classes when looking for overrides which
means that we will install incorrect code if some subclass with an override was
finalized while compilation was in progress.

To catch situations like this we record the number of finalized subclasses that
class had
when CHA made the first negative decision about it (e.g. that it has no
subclasses or that it has no overrides for some function) and before installing
the code we check that number of subclasses matches.

Additionally renamed "leaf classes" to "guarded classes" because those classes
are not necessarily leaf.

R=fschneider@google.com
BUG=

Committed:
https://github.com/dart-lang/sdk/commit/6bb73bd2e41f9847e4b7a24bbd4d50d41a6536cf
==========

[Vyacheslav Egorov (Google), 2016-05-20 11:48:34]

Committed patchset #2 (id:20001) manually as
6bb73bd2e41f9847e4b7a24bbd4d50d41a6536cf (presubmit successful).