1. Fix the type arguments recursion problem that gets introduced when canonicalization of type argu…

1. Fix the type arguments recursion problem that gets introduced when canonicalization of type arguments is deferred while reading a script snapshot. This problem was shows up as random stack overflow crashes when running pub

2. Patching of the type arguments field in a GrowableObjectArray was missed

BUG=23930
R=regis@google.com, rmacnak@google.com

Committed: https://github.com/dart-lang/sdk/commit/521dee763c95a374031e06572d3f56ceb8e8d91f

[siva, 2015-08-06 00:55:45]

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

[regis, 2015-08-06 16:36:08]

LGTM

https://codereview.chromium.org/1276753002/diff/40001/runtime/vm/raw_object_s...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/1276753002/diff/40001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:2341: const intptr_t typeargs_offset =
Use GrowableObjectArray::type_arguments_offset()

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

https://codereview.chromium.org/1276753002/diff/40001/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:1171: newobj = typeobj.Canonicalize();
Ideally, in the case of a resursive type, Canonicalize() should return the input
node, and not a different (but equivalent) node in the cyclic graph. This would
make patching safe in all cases.

This would be easier to use than IsDisjoint() or IsEquivalentButDisjoint() new
functions as we discussed. In other words, this new functionality should be
encapsulated in the Canonicalize() call.

I'll work on that next.

[rmacnak, 2015-08-06 16:48:45]

lgtm

[siva, 2015-08-06 17:28:45]

https://codereview.chromium.org/1276753002/diff/40001/runtime/vm/raw_object_s...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/1276753002/diff/40001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:2341: const intptr_t typeargs_offset =
On 2015/08/06 16:36:08, regis wrote:
> Use GrowableObjectArray::type_arguments_offset()

It needs to be the offset in words and hence the pointer arithmetic, I could use
GrowableObjectArray::type_arguments_offset() / kWordSize;

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

https://codereview.chromium.org/1276753002/diff/40001/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:1171: newobj = typeobj.Canonicalize();
On 2015/08/06 16:36:08, regis wrote:
> Ideally, in the case of a resursive type, Canonicalize() should return the
input
> node, and not a different (but equivalent) node in the cyclic graph. This
would
> make patching safe in all cases.
> 
> This would be easier to use than IsDisjoint() or IsEquivalentButDisjoint() new
> functions as we discussed. In other words, this new functionality should be
> encapsulated in the Canonicalize() call.
> 
> I'll work on that next.

I was thinking of another approach wherein we always use TypeRef nodes in a
recursive implementation so that it is always uniform and do not make a Type
node be equivalent to a type ref node pointing to the type node. We can discuss
this in person.

[siva, 2015-08-06 17:55:53]

Committed patchset #4 (id:60001) manually as
521dee763c95a374031e06572d3f56ceb8e8d91f (presubmit successful).

[regis, 2015-08-06 18:24:35]

https://codereview.chromium.org/1276753002/diff/40001/runtime/vm/raw_object_s...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/1276753002/diff/40001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:2341: const intptr_t typeargs_offset =
On 2015/08/06 17:28:44, siva wrote:
> On 2015/08/06 16:36:08, regis wrote:
> > Use GrowableObjectArray::type_arguments_offset()
> 
> It needs to be the offset in words and hence the pointer arithmetic, I could
use
> GrowableObjectArray::type_arguments_offset() / kWordSize;

I missed that detail.

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

https://codereview.chromium.org/1276753002/diff/40001/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:1171: newobj = typeobj.Canonicalize();
On 2015/08/06 17:28:44, siva wrote:
> On 2015/08/06 16:36:08, regis wrote:
> > Ideally, in the case of a resursive type, Canonicalize() should return the
> input
> > node, and not a different (but equivalent) node in the cyclic graph. This
> would
> > make patching safe in all cases.
> > 
> > This would be easier to use than IsDisjoint() or IsEquivalentButDisjoint()
new
> > functions as we discussed. In other words, this new functionality should be
> > encapsulated in the Canonicalize() call.
> > 
> > I'll work on that next.
> 
> I was thinking of another approach wherein we always use TypeRef nodes in a
> recursive implementation so that it is always uniform and do not make a Type
> node be equivalent to a type ref node pointing to the type node. We can
discuss
> this in person.

We still have to make sure type tests succeed. Yes, let's talk.