Context objects don't have a compile-type. Return dynamic-type in this case.

Context objects don't have a compile-type. Return dynamic-type in this case.

This is lifted from
https://codereview.chromium.org/678763004/diff/120001/runtime/vm/flow_graph_type_propagator.cc
to fix issue 21455.


BUG=21455
R=vegorov@google.com

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

[Vyacheslav Egorov (Google), 2014-10-29 18:36:15]

LGTM!

https://codereview.chromium.org/689713003/diff/1/runtime/vm/flow_graph_type_p...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/689713003/diff/1/runtime/vm/flow_graph_type_p...
runtime/vm/flow_graph_type_propagator.cc:553: if (cid_ == kFunctionCid || cid_
== kContextCid) {
(cid_ == kFunctionCid) || (cid_ == kContextCid)

I wonder if there is a more robust way to catch all classes don't have
meaningful AbstractType associated with them?

[koda, 2014-10-29 19:28:06]

https://codereview.chromium.org/689713003/diff/1/runtime/vm/flow_graph_type_p...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/689713003/diff/1/runtime/vm/flow_graph_type_p...
runtime/vm/flow_graph_type_propagator.cc:553: if (cid_ == kFunctionCid || cid_
== kContextCid) {
On 2014/10/29 18:36:15, Vyacheslav Egorov (Google) wrote:
> (cid_ == kFunctionCid) || (cid_ == kContextCid)
> 
> I wonder if there is a more robust way to catch all classes don't have
> meaningful AbstractType associated with them?

Not sure. Maybe everything below kInstanceCid should be considered special?

Added a regression test based on the repro code in the issue.

[Vyacheslav Egorov (Google), 2014-10-29 19:37:20]

https://codereview.chromium.org/689713003/diff/1/runtime/vm/flow_graph_type_p...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/689713003/diff/1/runtime/vm/flow_graph_type_p...
runtime/vm/flow_graph_type_propagator.cc:553: if (cid_ == kFunctionCid || cid_
== kContextCid) {
> Not sure. Maybe everything below kInstanceCid should be considered special?

Wouldn't that exclude Error's?

But I think it SGTM if you change this to cid_ < kInstanceCid.

[koda, 2014-10-29 21:35:07]

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

[Vyacheslav Egorov (Google), 2014-10-29 21:55:49]

Hmm. Instead of assertion I thought you would just do 

if (cid < kInstanceCid) {
  // set type to dynamic
  return;
}

Also my stylistic comment was ignored as far as I see: I thought Dart VM style
is to parenthesize it like this (x == y) || (z == a), not x == y || z == a.

[koda, 2014-10-29 22:28:36]

On 2014/10/29 21:55:49, Vyacheslav Egorov (Google) wrote:
> Hmm. Instead of assertion I thought you would just do 
> 
> if (cid < kInstanceCid) {
>   // set type to dynamic
>   return;
> }
> 
> Also my stylistic comment was ignored as far as I see: I thought Dart VM style
> is to parenthesize it like this (x == y) || (z == a), not x == y || z == a.

Sorry, missed that.

Follow-up CL:
https://codereview.chromium.org/684313003/diff/20001/runtime/vm/flow_graph_ty...