Avoid assert fault when using --trace-type-checks

Avoid assert fault when using --trace-type-checks

R=rmacnak@google.com

Committed: https://github.com/dart-lang/sdk/commit/525d66e1bdcd353c978ae7e0412076c5c74f7cf5

[regis, 2017-06-12 23:06:40]

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

[regis, 2017-06-12 23:06:41]

Hi Diogenes,

First, welcome!

Since you are working on the gc new/old spaces, you will probably understand
what is going on here. I have not used the flag --trace-type-checks in a while,
but notice today that it is broken. Unless I apply the fix in this cl, I hit an
assert fault that a class object (of a runtime type only allocated when the flag
is given) is not in the old space. I would expect type canonicalization to move
the type into old space, but maybe not its type class?

Thanks,
Regis

[rmacnak, 2017-06-12 23:46:32]

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

https://codereview.chromium.org/2933983002/diff/1/runtime/vm/runtime_entry.cc...
runtime/vm/runtime_entry.cc:392:
AbstractType::Handle(instance.GetType(Heap::kOld));
We implement Object.runtimeType in terms of Instance::GetType(Heap::kNew),
specifically into new space because Flutter calls it often, so I think we need
this to work. Perhaps somewhere in type instantiation we use the passed-in space
to allocate a class when we should use old?

[danunez, 2017-06-13 20:09:04]

On 2017/06/12 23:46:32, rmacnak wrote:
> https://codereview.chromium.org/2933983002/diff/1/runtime/vm/runtime_entry.cc
> File runtime/vm/runtime_entry.cc (right):
> 
>
https://codereview.chromium.org/2933983002/diff/1/runtime/vm/runtime_entry.cc...
> runtime/vm/runtime_entry.cc:392:
> AbstractType::Handle(instance.GetType(Heap::kOld));
> We implement Object.runtimeType in terms of Instance::GetType(Heap::kNew),
> specifically into new space because Flutter calls it often, so I think we need
> this to work. Perhaps somewhere in type instantiation we use the passed-in
space
> to allocate a class when we should use old?

object.cc:17142 allocates a new Type object for the instantiated type into the
passed-in space.
This will also happen when instantiating type_arguments in
runtime/vm/object.cc:17135 and sig_fun in runtime/vm/object.cc:17157.

I assumed the background compiler was the only part of the VM with an issue with
allocating into new space. Is the background thread, therefore, also printing
the type checks when the flag is enabled?

[regis, 2017-06-13 21:46:09]

On 2017/06/13 20:09:04, danunez wrote:
> On 2017/06/12 23:46:32, rmacnak wrote:
> >
https://codereview.chromium.org/2933983002/diff/1/runtime/vm/runtime_entry.cc
> > File runtime/vm/runtime_entry.cc (right):
> > 
> >
>
https://codereview.chromium.org/2933983002/diff/1/runtime/vm/runtime_entry.cc...
> > runtime/vm/runtime_entry.cc:392:
> > AbstractType::Handle(instance.GetType(Heap::kOld));
> > We implement Object.runtimeType in terms of Instance::GetType(Heap::kNew),
> > specifically into new space because Flutter calls it often, so I think we
need
> > this to work. Perhaps somewhere in type instantiation we use the passed-in
> space
> > to allocate a class when we should use old?
> 
> object.cc:17142 allocates a new Type object for the instantiated type into the
> passed-in space.
> This will also happen when instantiating type_arguments in
> runtime/vm/object.cc:17135 and sig_fun in runtime/vm/object.cc:17157.
> 
> I assumed the background compiler was the only part of the VM with an issue
with
> allocating into new space. Is the background thread, therefore, also printing
> the type checks when the flag is enabled?

Ryan, I agree that passing kNew should work and that we need to find the root
cause of this problem.
Passing kNew worked the last time I used the flag --trace-type-checks.

Diogenes, the background compiler is not involved in this problem. The reason we
pass kNew is that the instantiated type is only needed for printing behind the
flag and will not be preserved (except that canonicalization forces cloning to
old space).

Any command (pretty much) will reproduce the assert fault. Here is one:
out/DebugX64/dart --no-background-compilation --trace_type_checks
--enable_asserts --enable_type_checks --packages=.packages
tests/language/type_check_test.dart

Interestingly, the crash always happens when compiling "Function '==':."
The compiler tries to emit a push of the constant "Type: class
'_SendPortImpl@1026248'", but Assembler::CanLoadFromObjectPool complains that
the type is allocated in new space. Note that the type is canonical, as
expected. However, any canonical type should have been cloned into old space, if
initially allocated in new space.

Do the names '==' and '_SendPortImpl' ring a bell? Are they subject to some
special treatment? If not, I'll dig deeper. We may have a bug in type
canonicalization.

Thanks,
Regis

[regis, 2017-06-14 17:10:16]

On 2017/06/13 21:46:09, regis wrote:
> On 2017/06/13 20:09:04, danunez wrote:
> > On 2017/06/12 23:46:32, rmacnak wrote:
> > >
> https://codereview.chromium.org/2933983002/diff/1/runtime/vm/runtime_entry.cc
> > > File runtime/vm/runtime_entry.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2933983002/diff/1/runtime/vm/runtime_entry.cc...
> > > runtime/vm/runtime_entry.cc:392:
> > > AbstractType::Handle(instance.GetType(Heap::kOld));
> > > We implement Object.runtimeType in terms of Instance::GetType(Heap::kNew),
> > > specifically into new space because Flutter calls it often, so I think we
> need
> > > this to work. Perhaps somewhere in type instantiation we use the passed-in
> > space
> > > to allocate a class when we should use old?
> > 
> > object.cc:17142 allocates a new Type object for the instantiated type into
the
> > passed-in space.
> > This will also happen when instantiating type_arguments in
> > runtime/vm/object.cc:17135 and sig_fun in runtime/vm/object.cc:17157.
> > 
> > I assumed the background compiler was the only part of the VM with an issue
> with
> > allocating into new space. Is the background thread, therefore, also
printing
> > the type checks when the flag is enabled?
> 
> Ryan, I agree that passing kNew should work and that we need to find the root
> cause of this problem.
> Passing kNew worked the last time I used the flag --trace-type-checks.
> 
> Diogenes, the background compiler is not involved in this problem. The reason
we
> pass kNew is that the instantiated type is only needed for printing behind the
> flag and will not be preserved (except that canonicalization forces cloning to
> old space).
> 
> Any command (pretty much) will reproduce the assert fault. Here is one:
> out/DebugX64/dart --no-background-compilation --trace_type_checks
> --enable_asserts --enable_type_checks --packages=.packages
> tests/language/type_check_test.dart
> 
> Interestingly, the crash always happens when compiling "Function '==':."
> The compiler tries to emit a push of the constant "Type: class
> '_SendPortImpl@1026248'", but Assembler::CanLoadFromObjectPool complains that
> the type is allocated in new space. Note that the type is canonical, as
> expected. However, any canonical type should have been cloned into old space,
if
> initially allocated in new space.
> 
> Do the names '==' and '_SendPortImpl' ring a bell? Are they subject to some
> special treatment? If not, I'll dig deeper. We may have a bug in type
> canonicalization.
> 
> Thanks,
> Regis

Ryan,
I looked a bit deeper and noticed serious inconsistencies. Type::Canonicalize()
is not guaranteeing anymore that the canonicalized type is cloned into old
space. Sometimes it does, sometimes it does not. Was that changed recently
because of what you wrote here?
> We implement Object.runtimeType in terms of
> Instance::GetType(Heap::kNew), specifically into new space because
> Flutter calls it often, so I think we need this to work.

I am not sure it makes sense to keep canonical objects in new space, since they
are anchored in lists in old space, or in this case (non-generic), in the class
of the type, which is also in old space. So they will end up in old space
anyway. Maybe I do not understand exactly what is the problem with Flutter here.
I understand that it is desirable to allocate the temporary result of GetType in
new space, but the canonical result should still be returned in old space.

Not canonicalizing the result of Instance::GetType is dangerous, since pointer
equality will not work anymore. So that is not an option.

Now, the crash happens when storing this type in the constant pool. Cloning the
type from new to old space at this late stage makes me nervous. I think the fix
of this cl is much safer, and performance is a non issue when using the
--trace-type-checks flag.

But let me correct it the way I think it should be, and you can tell me if this
will cause a performance issue with flutter. Stay tuned...


Thanks,
Regis

[regis, 2017-06-14 17:25:40]

On 2017/06/14 17:10:16, regis wrote:
> On 2017/06/13 21:46:09, regis wrote:
> > On 2017/06/13 20:09:04, danunez wrote:
> > > On 2017/06/12 23:46:32, rmacnak wrote:
> > > >
> >
https://codereview.chromium.org/2933983002/diff/1/runtime/vm/runtime_entry.cc
> > > > File runtime/vm/runtime_entry.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2933983002/diff/1/runtime/vm/runtime_entry.cc...
> > > > runtime/vm/runtime_entry.cc:392:
> > > > AbstractType::Handle(instance.GetType(Heap::kOld));
> > > > We implement Object.runtimeType in terms of
Instance::GetType(Heap::kNew),
> > > > specifically into new space because Flutter calls it often, so I think
we
> > need
> > > > this to work. Perhaps somewhere in type instantiation we use the
passed-in
> > > space
> > > > to allocate a class when we should use old?
> > > 
> > > object.cc:17142 allocates a new Type object for the instantiated type into
> the
> > > passed-in space.
> > > This will also happen when instantiating type_arguments in
> > > runtime/vm/object.cc:17135 and sig_fun in runtime/vm/object.cc:17157.
> > > 
> > > I assumed the background compiler was the only part of the VM with an
issue
> > with
> > > allocating into new space. Is the background thread, therefore, also
> printing
> > > the type checks when the flag is enabled?
> > 
> > Ryan, I agree that passing kNew should work and that we need to find the
root
> > cause of this problem.
> > Passing kNew worked the last time I used the flag --trace-type-checks.
> > 
> > Diogenes, the background compiler is not involved in this problem. The
reason
> we
> > pass kNew is that the instantiated type is only needed for printing behind
the
> > flag and will not be preserved (except that canonicalization forces cloning
to
> > old space).
> > 
> > Any command (pretty much) will reproduce the assert fault. Here is one:
> > out/DebugX64/dart --no-background-compilation --trace_type_checks
> > --enable_asserts --enable_type_checks --packages=.packages
> > tests/language/type_check_test.dart
> > 
> > Interestingly, the crash always happens when compiling "Function '==':."
> > The compiler tries to emit a push of the constant "Type: class
> > '_SendPortImpl@1026248'", but Assembler::CanLoadFromObjectPool complains
that
> > the type is allocated in new space. Note that the type is canonical, as
> > expected. However, any canonical type should have been cloned into old
space,
> if
> > initially allocated in new space.
> > 
> > Do the names '==' and '_SendPortImpl' ring a bell? Are they subject to some
> > special treatment? If not, I'll dig deeper. We may have a bug in type
> > canonicalization.
> > 
> > Thanks,
> > Regis
> 
> Ryan,
> I looked a bit deeper and noticed serious inconsistencies.
Type::Canonicalize()
> is not guaranteeing anymore that the canonicalized type is cloned into old
> space. Sometimes it does, sometimes it does not. Was that changed recently
> because of what you wrote here?
> > We implement Object.runtimeType in terms of
> > Instance::GetType(Heap::kNew), specifically into new space because
> > Flutter calls it often, so I think we need this to work.
> 
> I am not sure it makes sense to keep canonical objects in new space, since
they
> are anchored in lists in old space, or in this case (non-generic), in the
class
> of the type, which is also in old space. So they will end up in old space
> anyway. Maybe I do not understand exactly what is the problem with Flutter
here.
> I understand that it is desirable to allocate the temporary result of GetType
in
> new space, but the canonical result should still be returned in old space.
> 
> Not canonicalizing the result of Instance::GetType is dangerous, since pointer
> equality will not work anymore. So that is not an option.
> 
> Now, the crash happens when storing this type in the constant pool. Cloning
the
> type from new to old space at this late stage makes me nervous. I think the
fix
> of this cl is much safer, and performance is a non issue when using the
> --trace-type-checks flag.
> 
> But let me correct it the way I think it should be, and you can tell me if
this
> will cause a performance issue with flutter. Stay tuned...
> 
> 
> Thanks,
> Regis

Ryan,

How about this fix instead? The temporary type allocated in Instance::GetType
can be allocated in new space, but the returned canonical one will be in old
space.

Thanks,
Regis

[rmacnak, 2017-06-14 17:28:01]

On 2017/06/14 17:25:40, regis wrote:
> On 2017/06/14 17:10:16, regis wrote:
> > On 2017/06/13 21:46:09, regis wrote:
> > > On 2017/06/13 20:09:04, danunez wrote:
> > > > On 2017/06/12 23:46:32, rmacnak wrote:
> > > > >
> > >
> https://codereview.chromium.org/2933983002/diff/1/runtime/vm/runtime_entry.cc
> > > > > File runtime/vm/runtime_entry.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2933983002/diff/1/runtime/vm/runtime_entry.cc...
> > > > > runtime/vm/runtime_entry.cc:392:
> > > > > AbstractType::Handle(instance.GetType(Heap::kOld));
> > > > > We implement Object.runtimeType in terms of
> Instance::GetType(Heap::kNew),
> > > > > specifically into new space because Flutter calls it often, so I think
> we
> > > need
> > > > > this to work. Perhaps somewhere in type instantiation we use the
> passed-in
> > > > space
> > > > > to allocate a class when we should use old?
> > > > 
> > > > object.cc:17142 allocates a new Type object for the instantiated type
into
> > the
> > > > passed-in space.
> > > > This will also happen when instantiating type_arguments in
> > > > runtime/vm/object.cc:17135 and sig_fun in runtime/vm/object.cc:17157.
> > > > 
> > > > I assumed the background compiler was the only part of the VM with an
> issue
> > > with
> > > > allocating into new space. Is the background thread, therefore, also
> > printing
> > > > the type checks when the flag is enabled?
> > > 
> > > Ryan, I agree that passing kNew should work and that we need to find the
> root
> > > cause of this problem.
> > > Passing kNew worked the last time I used the flag --trace-type-checks.
> > > 
> > > Diogenes, the background compiler is not involved in this problem. The
> reason
> > we
> > > pass kNew is that the instantiated type is only needed for printing behind
> the
> > > flag and will not be preserved (except that canonicalization forces
cloning
> to
> > > old space).
> > > 
> > > Any command (pretty much) will reproduce the assert fault. Here is one:
> > > out/DebugX64/dart --no-background-compilation --trace_type_checks
> > > --enable_asserts --enable_type_checks --packages=.packages
> > > tests/language/type_check_test.dart
> > > 
> > > Interestingly, the crash always happens when compiling "Function '==':."
> > > The compiler tries to emit a push of the constant "Type: class
> > > '_SendPortImpl@1026248'", but Assembler::CanLoadFromObjectPool complains
> that
> > > the type is allocated in new space. Note that the type is canonical, as
> > > expected. However, any canonical type should have been cloned into old
> space,
> > if
> > > initially allocated in new space.
> > > 
> > > Do the names '==' and '_SendPortImpl' ring a bell? Are they subject to
some
> > > special treatment? If not, I'll dig deeper. We may have a bug in type
> > > canonicalization.
> > > 
> > > Thanks,
> > > Regis
> > 
> > Ryan,
> > I looked a bit deeper and noticed serious inconsistencies.
> Type::Canonicalize()
> > is not guaranteeing anymore that the canonicalized type is cloned into old
> > space. Sometimes it does, sometimes it does not. Was that changed recently
> > because of what you wrote here?
> > > We implement Object.runtimeType in terms of
> > > Instance::GetType(Heap::kNew), specifically into new space because
> > > Flutter calls it often, so I think we need this to work.
> > 
> > I am not sure it makes sense to keep canonical objects in new space, since
> they
> > are anchored in lists in old space, or in this case (non-generic), in the
> class
> > of the type, which is also in old space. So they will end up in old space
> > anyway. Maybe I do not understand exactly what is the problem with Flutter
> here.
> > I understand that it is desirable to allocate the temporary result of
GetType
> in
> > new space, but the canonical result should still be returned in old space.
> > 
> > Not canonicalizing the result of Instance::GetType is dangerous, since
pointer
> > equality will not work anymore. So that is not an option.
> > 
> > Now, the crash happens when storing this type in the constant pool. Cloning
> the
> > type from new to old space at this late stage makes me nervous. I think the
> fix
> > of this cl is much safer, and performance is a non issue when using the
> > --trace-type-checks flag.
> > 
> > But let me correct it the way I think it should be, and you can tell me if
> this
> > will cause a performance issue with flutter. Stay tuned...
> > 
> > 
> > Thanks,
> > Regis
> 
> Ryan,
> 
> How about this fix instead? The temporary type allocated in Instance::GetType
> can be allocated in new space, but the returned canonical one will be in old
> space.
> 
> Thanks,
> Regis

Yes that's what I though was happening :)

LGTM

[regis, 2017-06-14 17:32:57]

On 2017/06/14 17:28:01, rmacnak wrote:
> On 2017/06/14 17:25:40, regis wrote:
> > On 2017/06/14 17:10:16, regis wrote:
> > > On 2017/06/13 21:46:09, regis wrote:
> > > > On 2017/06/13 20:09:04, danunez wrote:
> > > > > On 2017/06/12 23:46:32, rmacnak wrote:
> > > > > >
> > > >
> >
https://codereview.chromium.org/2933983002/diff/1/runtime/vm/runtime_entry.cc
> > > > > > File runtime/vm/runtime_entry.cc (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2933983002/diff/1/runtime/vm/runtime_entry.cc...
> > > > > > runtime/vm/runtime_entry.cc:392:
> > > > > > AbstractType::Handle(instance.GetType(Heap::kOld));
> > > > > > We implement Object.runtimeType in terms of
> > Instance::GetType(Heap::kNew),
> > > > > > specifically into new space because Flutter calls it often, so I
think
> > we
> > > > need
> > > > > > this to work. Perhaps somewhere in type instantiation we use the
> > passed-in
> > > > > space
> > > > > > to allocate a class when we should use old?
> > > > > 
> > > > > object.cc:17142 allocates a new Type object for the instantiated type
> into
> > > the
> > > > > passed-in space.
> > > > > This will also happen when instantiating type_arguments in
> > > > > runtime/vm/object.cc:17135 and sig_fun in runtime/vm/object.cc:17157.
> > > > > 
> > > > > I assumed the background compiler was the only part of the VM with an
> > issue
> > > > with
> > > > > allocating into new space. Is the background thread, therefore, also
> > > printing
> > > > > the type checks when the flag is enabled?
> > > > 
> > > > Ryan, I agree that passing kNew should work and that we need to find the
> > root
> > > > cause of this problem.
> > > > Passing kNew worked the last time I used the flag --trace-type-checks.
> > > > 
> > > > Diogenes, the background compiler is not involved in this problem. The
> > reason
> > > we
> > > > pass kNew is that the instantiated type is only needed for printing
behind
> > the
> > > > flag and will not be preserved (except that canonicalization forces
> cloning
> > to
> > > > old space).
> > > > 
> > > > Any command (pretty much) will reproduce the assert fault. Here is one:
> > > > out/DebugX64/dart --no-background-compilation --trace_type_checks
> > > > --enable_asserts --enable_type_checks --packages=.packages
> > > > tests/language/type_check_test.dart
> > > > 
> > > > Interestingly, the crash always happens when compiling "Function '==':."
> > > > The compiler tries to emit a push of the constant "Type: class
> > > > '_SendPortImpl@1026248'", but Assembler::CanLoadFromObjectPool complains
> > that
> > > > the type is allocated in new space. Note that the type is canonical, as
> > > > expected. However, any canonical type should have been cloned into old
> > space,
> > > if
> > > > initially allocated in new space.
> > > > 
> > > > Do the names '==' and '_SendPortImpl' ring a bell? Are they subject to
> some
> > > > special treatment? If not, I'll dig deeper. We may have a bug in type
> > > > canonicalization.
> > > > 
> > > > Thanks,
> > > > Regis
> > > 
> > > Ryan,
> > > I looked a bit deeper and noticed serious inconsistencies.
> > Type::Canonicalize()
> > > is not guaranteeing anymore that the canonicalized type is cloned into old
> > > space. Sometimes it does, sometimes it does not. Was that changed recently
> > > because of what you wrote here?
> > > > We implement Object.runtimeType in terms of
> > > > Instance::GetType(Heap::kNew), specifically into new space because
> > > > Flutter calls it often, so I think we need this to work.
> > > 
> > > I am not sure it makes sense to keep canonical objects in new space, since
> > they
> > > are anchored in lists in old space, or in this case (non-generic), in the
> > class
> > > of the type, which is also in old space. So they will end up in old space
> > > anyway. Maybe I do not understand exactly what is the problem with Flutter
> > here.
> > > I understand that it is desirable to allocate the temporary result of
> GetType
> > in
> > > new space, but the canonical result should still be returned in old space.
> > > 
> > > Not canonicalizing the result of Instance::GetType is dangerous, since
> pointer
> > > equality will not work anymore. So that is not an option.
> > > 
> > > Now, the crash happens when storing this type in the constant pool.
Cloning
> > the
> > > type from new to old space at this late stage makes me nervous. I think
the
> > fix
> > > of this cl is much safer, and performance is a non issue when using the
> > > --trace-type-checks flag.
> > > 
> > > But let me correct it the way I think it should be, and you can tell me if
> > this
> > > will cause a performance issue with flutter. Stay tuned...
> > > 
> > > 
> > > Thanks,
> > > Regis
> > 
> > Ryan,
> > 
> > How about this fix instead? The temporary type allocated in
Instance::GetType
> > can be allocated in new space, but the returned canonical one will be in old
> > space.
> > 
> > Thanks,
> > Regis
> 
> Yes that's what I though was happening :)
> 
> LGTM

OK, cool, we agree. Let me finish testing and I'll commit.

Thanks,
Regis

[regis, 2017-06-14 17:49:24]

Description was changed from

==========
Avoid assert fault when using --trace-type-checks
==========

to

==========
Avoid assert fault when using --trace-type-checks

R=rmacnak@google.com

Committed:
https://github.com/dart-lang/sdk/commit/525d66e1bdcd353c978ae7e0412076c5c74f7cf5
==========

[regis, 2017-06-14 17:49:25]

Committed patchset #2 (id:20001) manually as
525d66e1bdcd353c978ae7e0412076c5c74f7cf5 (presubmit successful).