VM: Fix race between background compiler and guarded cid update.

VM: Fix race between background compiler and guarded cid update.

Rework how we check guarded state consistency in background compiler.

Background compiler was storing original fields inside guarded fields list. This caused a race during inlining when inliner would copy guarded fields one by one from the callee function into the caller, because ParsedFunction::AddToGuardedFields looks at the guarded_cid to filter out those fields that should not be guarded.

As a result if some guarded field transitioned to unguarded (kDynamicCid) after callee graph construction but before list of guarded fields were copied then AddToGuardedFields would simply skip that field because it now has guarded_cid() == kDynamicCid.

We fix this race by always placing copies into the list of guarded fields and unwrapping them only in FinalizeCode.

Placing the copies also allows us to simplify a lot of code that was trying to verify guarded state consistency before committing the generated optimized code - now that we store copies in the list we can just compare their state to the originals and abort if the state is different.

Additionally fix deduplication check that was comparing original fields with copies - resulting in adding the same field into the list multiple times.

Add an assertion that verifies that we are not trying to access guarded_cid of original field from background compiler.


R=fschneider@google.com
BUG=

Committed: https://github.com/dart-lang/sdk/commit/65f2e015ca6da96589ef97441efa14ff6ff163a5

[Florian Schneider, 2016-05-23 19:26:32]

LGTM

https://codereview.chromium.org/2006793002/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/2006793002/diff/1/runtime/vm/object.h#newcode...
runtime/vm/object.h:3136: void set_guarded_cid(intptr_t cid) const {
Add the same assert as for guarded_cid()?

[Vyacheslav Egorov (Google), 2016-05-24 10:57:49]

Please take another look. 

I reworked how background compiler validates the background compiled code with
respect to the guarded state. It is enough to just check copies against
originals - no need to keep the list of disabling fields and all other related
stuff.

This allows to streamline a lot of code.

https://codereview.chromium.org/2006793002/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/2006793002/diff/1/runtime/vm/object.h#newcode...
runtime/vm/object.h:3136: void set_guarded_cid(intptr_t cid) const {
On 2016/05/23 19:26:32, Florian Schneider wrote:
> Add the same assert as for guarded_cid()?

Done.

[Florian Schneider, 2016-05-24 12:35:51]

lgtm

[Vyacheslav Egorov (Google), 2016-05-24 12:44:28]

Description was changed from

==========
VM: Fix race between background compiler and guarded cid update.

Background compiler was storing original fields inside guarded fields list. This
caused a race during inlining when inliner would copy guarded fields one by one
from the callee function into the caller, because
ParsedFunction::AddToGuardedFields looks at the guarded_cid to filter out those
fields that should not be guarded.

As a result if some guarded field transitioned to unguarded (kDynamicCid) after
callee graph construction but before list of guarded fields were copied then
AddToGuardedFields would simply skip that field because it now has guarded_cid()
== kDynamicCid.

We fix this race by always placing copies into the list of guarded fields and
unwrapping them only in FinalizeCode.

Additionally fix deduplication check that was comparing original fields with
copies - resulting in adding the same field into the list multiple times.

Add an assertion that verifies that we are not trying to access guarded_cid of
original field from background compiler.


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

to

==========
VM: Fix race between background compiler and guarded cid update.

Rework how we check guarded state consistency in background compiler.

Background compiler was storing original fields inside guarded fields list. This
caused a race during inlining when inliner would copy guarded fields one by one
from the callee function into the caller, because
ParsedFunction::AddToGuardedFields looks at the guarded_cid to filter out those
fields that should not be guarded.

As a result if some guarded field transitioned to unguarded (kDynamicCid) after
callee graph construction but before list of guarded fields were copied then
AddToGuardedFields would simply skip that field because it now has guarded_cid()
== kDynamicCid.

We fix this race by always placing copies into the list of guarded fields and
unwrapping them only in FinalizeCode. 

Placing the copies also allows us to simplify a lot of code that was trying to
verify guarded state consistency before committing the generated optimized code
- now that we store copies in the list we can just compare their state to the
originals and abort if the state is different.

Additionally fix deduplication check that was comparing original fields with
copies - resulting in adding the same field into the list multiple times.

Add an assertion that verifies that we are not trying to access guarded_cid of
original field from background compiler.


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

[Vyacheslav Egorov (Google), 2016-05-24 12:52:07]

Description was changed from

==========
VM: Fix race between background compiler and guarded cid update.

Rework how we check guarded state consistency in background compiler.

Background compiler was storing original fields inside guarded fields list. This
caused a race during inlining when inliner would copy guarded fields one by one
from the callee function into the caller, because
ParsedFunction::AddToGuardedFields looks at the guarded_cid to filter out those
fields that should not be guarded.

As a result if some guarded field transitioned to unguarded (kDynamicCid) after
callee graph construction but before list of guarded fields were copied then
AddToGuardedFields would simply skip that field because it now has guarded_cid()
== kDynamicCid.

We fix this race by always placing copies into the list of guarded fields and
unwrapping them only in FinalizeCode. 

Placing the copies also allows us to simplify a lot of code that was trying to
verify guarded state consistency before committing the generated optimized code
- now that we store copies in the list we can just compare their state to the
originals and abort if the state is different.

Additionally fix deduplication check that was comparing original fields with
copies - resulting in adding the same field into the list multiple times.

Add an assertion that verifies that we are not trying to access guarded_cid of
original field from background compiler.


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

to

==========
VM: Fix race between background compiler and guarded cid update.

Rework how we check guarded state consistency in background compiler.

Background compiler was storing original fields inside guarded fields list. This
caused a race during inlining when inliner would copy guarded fields one by one
from the callee function into the caller, because
ParsedFunction::AddToGuardedFields looks at the guarded_cid to filter out those
fields that should not be guarded.

As a result if some guarded field transitioned to unguarded (kDynamicCid) after
callee graph construction but before list of guarded fields were copied then
AddToGuardedFields would simply skip that field because it now has guarded_cid()
== kDynamicCid.

We fix this race by always placing copies into the list of guarded fields and
unwrapping them only in FinalizeCode.

Placing the copies also allows us to simplify a lot of code that was trying to
verify guarded state consistency before committing the generated optimized code
- now that we store copies in the list we can just compare their state to the
originals and abort if the state is different.

Additionally fix deduplication check that was comparing original fields with
copies - resulting in adding the same field into the list multiple times.

Add an assertion that verifies that we are not trying to access guarded_cid of
original field from background compiler.


R=fschneider@google.com
BUG=

Committed:
https://github.com/dart-lang/sdk/commit/65f2e015ca6da96589ef97441efa14ff6ff163a5
==========

[Vyacheslav Egorov (Google), 2016-05-24 12:52:09]

Committed patchset #2 (id:20001) manually as
65f2e015ca6da96589ef97441efa14ff6ff163a5 (presubmit successful).