Reapply "Shuffle around deopt id allocation...

runtime/vm/flow_graph_compiler_dbc.cc

Index: runtime/vm/flow_graph_compiler_dbc.cc
diff --git a/runtime/vm/flow_graph_compiler_dbc.cc b/runtime/vm/flow_graph_compiler_dbc.cc
index 47cfb97c4d0c605ba11660405c98e1837f27ef24..d623467fd736361aada26262962b93a5cc157a52 100644
--- a/runtime/vm/flow_graph_compiler_dbc.cc
+++ b/runtime/vm/flow_graph_compiler_dbc.cc
@@ -184,22 +184,28 @@ void FlowGraphCompiler::RecordAfterCallHelper(TokenPosition token_pos,
   RecordSafepoint(locs);
   // Marks either the continuation point in unoptimized code or the
   // deoptimization point in optimized code, after call.
-  const intptr_t deopt_id_after = Thread::ToDeoptAfter(deopt_id);
   if (is_optimizing()) {
     // Return/ReturnTOS instruction drops incoming arguments so
     // we have to drop outgoing arguments from the innermost environment.
     // On all other architectures caller drops outgoing arguments itself
     // hence the difference.
     pending_deoptimization_env_->DropArguments(argument_count);
-    CompilerDeoptInfo* info = AddDeoptIndexAtCall(deopt_id_after);
-    if (result == kHasResult) {
-      info->mark_lazy_deopt_with_result();
+    intptr_t deopt_id_after;

[Vyacheslav Egorov (Google), 2017/05/24 19:14:05]

This is a bit suspicious. What kind of instruction contains a call inside but
does not have a deopt_id?

If this call can trigger lazy deoptimization it *must* have a deopt id otherwise
lazy deoptimization will fail. 

[rmacnak, 2017/05/24 19:17:31]

A CreateArrayInstr. The other architectures are factored differently so I had
allowed specifically CreateArrayInstr to have no deopt id when optimizing.
Another way to address this would be to have CreateArrayInstr check for no deopt
id on DBC as well.

[Vyacheslav Egorov (Google), 2017/05/24 19:25:17]

I think it is possible to cause lazy deoptimization with CreateArray because it
invokes exception constructors, so it is not safe to omit deopt_id for it. 

Let me try to make a repro.

[Vyacheslav Egorov (Google), 2017/05/24 19:50:32]

Actually these days a simplest repro like this would do it - because we use
deoptimization to materialize state for exception handler (see
GetMetaDataFromDeopt in exceptions.cc - the stuff that Dmitry implemented).

[This repro also triggers crash in the simulator_dbc.cc becuase CreateArrayOpt
does not account for possibility of non-smi array lengthes - which I guess need
to be fixed separately]

 const NeverInline = "NeverInline";

 @NeverInline
 foo(int n) {
   try {
     return new List(n);
   } catch (e) {
   }
 }

 main () {
   for (var i = 0; i < 100000; i++) foo(5);
   foo("");
 }

+    if (deopt_id == Thread::kNoDeoptId) {
+      deopt_id_after = deopt_id;
+    } else {
+      deopt_id_after = Thread::ToDeoptAfter(deopt_id);
+      CompilerDeoptInfo* info = AddDeoptIndexAtCall(deopt_id_after);
+      if (result == kHasResult) {
+        info->mark_lazy_deopt_with_result();
+      }
     }
     // This descriptor is needed for exception handling in optimized code.
     AddCurrentDescriptor(RawPcDescriptors::kOther, deopt_id_after, token_pos);
   } else {
     // Add deoptimization continuation point after the call and before the
     // arguments are removed.
+    const intptr_t deopt_id_after = Thread::ToDeoptAfter(deopt_id);
     AddCurrentDescriptor(RawPcDescriptors::kDeopt, deopt_id_after, token_pos);
   }
 }