Keep a trail while checking upper bounds in the VM in order to properly handle

Keep a trail while checking upper bounds in the VM in order to properly handle
cycles via bounds (fixes #25568).
Add a regression test.
Improve handling of recursive types with trails.
Make sure we do not modify already canonicalized types.

R=rmacnak@google.com

Committed: https://github.com/dart-lang/sdk/commit/7ab578d21ffec146e7889edd847a408eeb8a5e26

[regis, 2016-02-08 21:17:25]

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

[rmacnak, 2016-02-09 00:50:23]

https://chromiumcodereview.appspot.com/1674383002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/1674383002/diff/1/runtime/vm/object.cc...
runtime/vm/object.cc:4838: ASSERT(IsOld());
I don't like the IsOld assertions. The type system shouldn't care if canonical
objects are always old or not.

https://chromiumcodereview.appspot.com/1674383002/diff/1/runtime/vm/object.cc...
runtime/vm/object.cc:16030: // are detected and disallowed. This is in contract
to regular types, where
contrast

Shouldn't this be ASSERT(!IsCanonical()) then?

[regis, 2016-02-09 17:17:38]

PTAL

Thanks!

https://chromiumcodereview.appspot.com/1674383002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/1674383002/diff/1/runtime/vm/object.cc...
runtime/vm/object.cc:4838: ASSERT(IsOld());
On 2016/02/09 00:50:22, rmacnak wrote:
> I don't like the IsOld assertions. The type system shouldn't care if canonical
> objects are always old or not.

Unfortunately, the type system does care. If you look below at line 4859, we
clone new canonical types to the old space before inserting them in the table.

This ASSERT would catch cases where this type was somehow canonicalized, but not
via the code below. I agree this assert is confusing and probably not very
useful, so I removed this assert here and in 3 other similar locations where the
same test is performed.

Now, I agree with your comment in general and I do not like the cloning code
below either. It is possible that this code is not required anymore, now that we
pass a space argument to all calls allocating new types. This would require some
investigation and cleanup, but in a separate cl.

https://chromiumcodereview.appspot.com/1674383002/diff/1/runtime/vm/object.cc...
runtime/vm/object.cc:16030: // are detected and disallowed. This is in contract
to regular types, where
On 2016/02/09 00:50:22, rmacnak wrote:
> contrast
> 
> Shouldn't this be ASSERT(!IsCanonical()) then?

Good catch! The comment on lines 16028-16031 is outdated and not true. I deleted
it. The code and comments below are correct. I first thought that function type
cannot be recursive, but they can as explained below.

[rmacnak, 2016-02-09 18:11:20]

LGTM

https://chromiumcodereview.appspot.com/1674383002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/1674383002/diff/1/runtime/vm/object.cc...
runtime/vm/object.cc:4838: ASSERT(IsOld());
On 2016/02/09 17:17:38, regis wrote:
> On 2016/02/09 00:50:22, rmacnak wrote:
> > I don't like the IsOld assertions. The type system shouldn't care if
canonical
> > objects are always old or not.
> 
> Unfortunately, the type system does care. If you look below at line 4859, we
> clone new canonical types to the old space before inserting them in the table.
> 
> This ASSERT would catch cases where this type was somehow canonicalized, but
not
> via the code below. I agree this assert is confusing and probably not very
> useful, so I removed this assert here and in 3 other similar locations where
the
> same test is performed.
> 
> Now, I agree with your comment in general and I do not like the cloning code
> below either. It is possible that this code is not required anymore, now that
we
> pass a space argument to all calls allocating new types. This would require
some
> investigation and cleanup, but in a separate cl.

I mean everything should still work if we didn't put canonical objects in old
space because doing so is a GC optimization and not an essential part of the
type rules.

[regis, 2016-02-09 18:48:59]

On 2016/02/09 18:11:20, rmacnak wrote:
> LGTM
> 
> https://chromiumcodereview.appspot.com/1674383002/diff/1/runtime/vm/object.cc
> File runtime/vm/object.cc (right):
> 
>
https://chromiumcodereview.appspot.com/1674383002/diff/1/runtime/vm/object.cc...
> runtime/vm/object.cc:4838: ASSERT(IsOld());
> On 2016/02/09 17:17:38, regis wrote:
> > On 2016/02/09 00:50:22, rmacnak wrote:
> > > I don't like the IsOld assertions. The type system shouldn't care if
> canonical
> > > objects are always old or not.
> > 
> > Unfortunately, the type system does care. If you look below at line 4859, we
> > clone new canonical types to the old space before inserting them in the
table.
> > 
> > This ASSERT would catch cases where this type was somehow canonicalized, but
> not
> > via the code below. I agree this assert is confusing and probably not very
> > useful, so I removed this assert here and in 3 other similar locations where
> the
> > same test is performed.
> > 
> > Now, I agree with your comment in general and I do not like the cloning code
> > below either. It is possible that this code is not required anymore, now
that
> we
> > pass a space argument to all calls allocating new types. This would require
> some
> > investigation and cleanup, but in a separate cl.
> 
> I mean everything should still work if we didn't put canonical objects in old
> space because doing so is a GC optimization and not an essential part of the
> type rules.

We agree. I'll revisit this after this cl and see if this code is still
relevant.

Thanks!

[regis, 2016-02-09 18:49:47]

Description was changed from

==========
Keep a trail while checking upper bounds in the VM in order to properly handle
cycles via bounds (fixes #25568).
Add a regression test.
Improve handling of recursive types with trails.
Make sure we do not modify already canonicalized types.
==========

to

==========
Keep a trail while checking upper bounds in the VM in order to properly handle
cycles via bounds (fixes #25568).
Add a regression test.
Improve handling of recursive types with trails.
Make sure we do not modify already canonicalized types.

R=rmacnak@google.com

Committed:
https://github.com/dart-lang/sdk/commit/7ab578d21ffec146e7889edd847a408eeb8a5e26
==========

[regis, 2016-02-09 18:49:48]

Committed patchset #2 (id:20001) manually as
7ab578d21ffec146e7889edd847a408eeb8a5e26 (presubmit successful).