Fix bug with guarded fields and deserialization.

Fix bug with guarded fields and deserialization.

Since deserialization does not involve the normal object construction
procedure, any values written there won't be reflected in the guarded field
type. This results in incorrect optimized code because deoptimization of
dependent code objects in not triggered.

This CL adds tracking of field types and guarded list length when creating
objects via deserialization.

R=iposva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=29741

[Florian Schneider, 2013-10-30 14:02:22]

https://codereview.chromium.org/50243004/diff/1/tests/standalone/standalone.s...
File tests/standalone/standalone.status (right):

https://codereview.chromium.org/50243004/diff/1/tests/standalone/standalone.s...
tests/standalone/standalone.status:12: issue14236_test: Pass, Fail, Crash #
Issue 14516
I added a new member to RawClass and as a result the checked-in snapshot is not
working anymore. See Dart issue 14516.

[srdjan, 2013-10-30 16:19:16]

DBC

https://codereview.chromium.org/50243004/diff/90001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/50243004/diff/90001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:1746: intptr_t length = raw_ptr()->instance_size_in_words_;
const

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

https://codereview.chromium.org/50243004/diff/90001/runtime/vm/object.h#newco...
runtime/vm/object.h:867: // inherited ones.
The comment is unclear. E.g.: it returns an array with index being field's
offest in words, for all fields of this class and its superclasses. The name of
the function could be better as well: e.g., CreateOffsetToFieldMapping

https://codereview.chromium.org/50243004/diff/90001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/50243004/diff/90001/runtime/vm/raw_object.h#n...
runtime/vm/raw_object.h:479: RawArray* offset_to_field_;
Maybe: offset_in_words_to_field_ so that it is clearer?

https://codereview.chromium.org/50243004/diff/90001/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://codereview.chromium.org/50243004/diff/90001/runtime/vm/snapshot.cc#ne...
runtime/vm/snapshot.cc:808: }
I would split this i two while loops, one for kMessage, and there I would hoist
out the loop invariants.

[Florian Schneider, 2013-10-31 20:36:19]

https://codereview.chromium.org/50243004/diff/90001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/50243004/diff/90001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:1746: intptr_t length = raw_ptr()->instance_size_in_words_;
On 2013/10/30 16:19:17, srdjan wrote:
> const

Done.

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

https://codereview.chromium.org/50243004/diff/90001/runtime/vm/object.h#newco...
runtime/vm/object.h:867: // inherited ones.
On 2013/10/30 16:19:17, srdjan wrote:
> The comment is unclear. E.g.: it returns an array with index being field's
> offest in words, for all fields of this class and its superclasses. The name
of
> the function could be better as well: e.g., CreateOffsetToFieldMapping

Done.

https://codereview.chromium.org/50243004/diff/90001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/50243004/diff/90001/runtime/vm/raw_object.h#n...
runtime/vm/raw_object.h:479: RawArray* offset_to_field_;
On 2013/10/30 16:19:17, srdjan wrote:
> Maybe: offset_in_words_to_field_ so that it is clearer?

Done.

https://codereview.chromium.org/50243004/diff/90001/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://codereview.chromium.org/50243004/diff/90001/runtime/vm/snapshot.cc#ne...
runtime/vm/snapshot.cc:808: }
On 2013/10/30 16:19:17, srdjan wrote:
> I would split this i two while loops, one for kMessage, and there I would
hoist
> out the loop invariants.

I can do that, but not sure how much can be gained: None of the temporary
handles used (cls_, array_, field_ is loop invariant because the call to
ReadObjectRef may change them, so hoisting those expressions of the loop would
result in additional handle allocation.

[Florian Schneider, 2013-10-31 20:38:42]

This is blocking https://codereview.chromium.org/27307005/ from landing since it
fixes a crasher revealed by the additional inlining from that CL.

[Ivan Posva, 2013-11-01 04:50:04]

LGTM after addressing comments.

-Ivan

https://codereview.chromium.org/50243004/diff/280001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/50243004/diff/280001/runtime/vm/object.cc#new...
runtime/vm/object.cc:1747: array = Array::New(length);
This array should be allocated in old space. It will survive as long as this
class exists and classes are allocated in old space.

https://codereview.chromium.org/50243004/diff/280001/runtime/vm/object.cc#new...
runtime/vm/object.cc:1756: array.SetAt(f.Offset() / kWordSize, f);
f.Offset() >> kWordSizeLog2

https://codereview.chromium.org/50243004/diff/280001/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://codereview.chromium.org/50243004/diff/280001/runtime/vm/snapshot.cc#n...
runtime/vm/snapshot.cc:799: cls_ = isolate()->class_table()->At(result_cid);
You want to look this up only once and not for every field in the object, but I
also understand that you cannot share the handle in that case. Would be OK to
add a TODO and investigate this with Siva after having addressed the crash fix.

https://codereview.chromium.org/50243004/diff/280001/runtime/vm/snapshot.cc#n...
runtime/vm/snapshot.cc:800: array_ = cls_.OffsetToFieldMap();
ditto

https://codereview.chromium.org/50243004/diff/280001/runtime/vm/snapshot.cc#n...
runtime/vm/snapshot.cc:801: field_ ^= array_.At(offset / kWordSize);
offset >> kWordSizeLog2

https://codereview.chromium.org/50243004/diff/280001/runtime/vm/snapshot.cc#n...
runtime/vm/snapshot.cc:808: }
We also wanted to assert that when deserializing other snapshots these values
were correct. At least add a TODO.

https://codereview.chromium.org/50243004/diff/280001/tests/language/vm/optimi...
File tests/language/vm/optimized_guarded_field_isolates_test.dart (right):

https://codereview.chromium.org/50243004/diff/280001/tests/language/vm/optimi...
tests/language/vm/optimized_guarded_field_isolates_test.dart:33: for (var i = 0;
i < 200; i++) test_b(b);
{
  test_b(b);
}

https://codereview.chromium.org/50243004/diff/280001/tests/language/vm/optimi...
tests/language/vm/optimized_guarded_field_isolates_test.dart:56: List list = new
List(10000);
Where is list being used?

https://codereview.chromium.org/50243004/diff/280001/tests/language/vm/optimi...
tests/language/vm/optimized_guarded_field_isolates_test.dart:58: for (var i = 0;
i < 200; i++) test_c(c);
ditto:
{
  test_c(c);
}

https://codereview.chromium.org/50243004/diff/280001/tests/language/vm/optimi...
File tests/language/vm/optimized_guarded_field_test.dart (right):

https://codereview.chromium.org/50243004/diff/280001/tests/language/vm/optimi...
tests/language/vm/optimized_guarded_field_test.dart:39: if (identical(s, null))
throw "FAIL";
Why not just s == null?

https://codereview.chromium.org/50243004/diff/280001/tests/language/vm/optimi...
tests/language/vm/optimized_guarded_field_test.dart:49: for (var i=0; i<20; i++)
check_stacktrace(e);
ditto

https://codereview.chromium.org/50243004/diff/280001/tests/language/vm/optimi...
tests/language/vm/optimized_guarded_field_test.dart:73: for (var i = 0; i < 20;
i++) test_deopt(42, 1);
ditto

[fschneider, 2013-11-01 10:39:24]

Committed patchset #4 manually as r29741.

[Florian Schneider, 2013-11-01 10:39:26]

https://codereview.chromium.org/50243004/diff/280001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/50243004/diff/280001/runtime/vm/object.cc#new...
runtime/vm/object.cc:1747: array = Array::New(length);
On 2013/11/01 04:50:05, Ivan Posva wrote:
> This array should be allocated in old space. It will survive as long as this
> class exists and classes are allocated in old space.

Done.

https://codereview.chromium.org/50243004/diff/280001/runtime/vm/object.cc#new...
runtime/vm/object.cc:1756: array.SetAt(f.Offset() / kWordSize, f);
On 2013/11/01 04:50:05, Ivan Posva wrote:
> f.Offset() >> kWordSizeLog2

Done.

https://codereview.chromium.org/50243004/diff/280001/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://codereview.chromium.org/50243004/diff/280001/runtime/vm/snapshot.cc#n...
runtime/vm/snapshot.cc:799: cls_ = isolate()->class_table()->At(result_cid);
On 2013/11/01 04:50:05, Ivan Posva wrote:
> You want to look this up only once and not for every field in the object, but
I
> also understand that you cannot share the handle in that case. Would be OK to
> add a TODO and investigate this with Siva after having addressed the crash
fix.

Done. Added TODO.

https://codereview.chromium.org/50243004/diff/280001/runtime/vm/snapshot.cc#n...
runtime/vm/snapshot.cc:800: array_ = cls_.OffsetToFieldMap();
On 2013/11/01 04:50:05, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/50243004/diff/280001/runtime/vm/snapshot.cc#n...
runtime/vm/snapshot.cc:801: field_ ^= array_.At(offset / kWordSize);
On 2013/11/01 04:50:05, Ivan Posva wrote:
> offset >> kWordSizeLog2

Done.

https://codereview.chromium.org/50243004/diff/280001/runtime/vm/snapshot.cc#n...
runtime/vm/snapshot.cc:808: }
On 2013/11/01 04:50:05, Ivan Posva wrote:
> We also wanted to assert that when deserializing other snapshots these values
> were correct. At least add a TODO.

Added TODO. I'll think about how to assert this for the other kinds.

https://codereview.chromium.org/50243004/diff/280001/tests/language/vm/optimi...
File tests/language/vm/optimized_guarded_field_isolates_test.dart (right):

https://codereview.chromium.org/50243004/diff/280001/tests/language/vm/optimi...
tests/language/vm/optimized_guarded_field_isolates_test.dart:33: for (var i = 0;
i < 200; i++) test_b(b);
On 2013/11/01 04:50:05, Ivan Posva wrote:
> {
>   test_b(b);
> }

Done.

https://codereview.chromium.org/50243004/diff/280001/tests/language/vm/optimi...
File tests/language/vm/optimized_guarded_field_test.dart (right):

https://codereview.chromium.org/50243004/diff/280001/tests/language/vm/optimi...
tests/language/vm/optimized_guarded_field_test.dart:39: if (identical(s, null))
throw "FAIL";
On 2013/11/01 04:50:05, Ivan Posva wrote:
> Why not just s == null?

I can't have an IC call here: it will introduce a CheckClass and defeat cid
propagation. Instead cid-propagation will illegally optimize the
identical-comparison away.

https://codereview.chromium.org/50243004/diff/280001/tests/language/vm/optimi...
tests/language/vm/optimized_guarded_field_test.dart:49: for (var i=0; i<20; i++)
check_stacktrace(e);
On 2013/11/01 04:50:05, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/50243004/diff/280001/tests/language/vm/optimi...
tests/language/vm/optimized_guarded_field_test.dart:73: for (var i = 0; i < 20;
i++) test_deopt(42, 1);
On 2013/11/01 04:50:05, Ivan Posva wrote:
> ditto

Done.