[compiler] Guard implicit tier-up when ensuring deopt support.

src/compiler.cc

Index: src/compiler.cc
diff --git a/src/compiler.cc b/src/compiler.cc
index 22e8b487eecbbae2cde9ae23277d6103835b64b4..076af855748f3a7a70af8b0bf43ae53ef9c046e2 100644
--- a/src/compiler.cc
+++ b/src/compiler.cc
@@ -859,7 +859,8 @@ MaybeHandle<Code> GetOptimizedCode(Handle<JSFunction> function,
   return MaybeHandle<Code>();
 }
 
-class InterpreterActivationsFinder : public ThreadVisitor {
+class InterpreterActivationsFinder : public ThreadVisitor,
+                                     public OptimizedFunctionVisitor {

[Yang, 2016/04/29 10:19:49]

Multiple inheritance. I hope that's fine with the style guide? (I haven't
checked). There is actually no need to mix these two, right? Also, is there a
way to abort visiting optimized functions if we already found an activation?

[rmcilroy, 2016/04/29 10:50:29]

Both classes are pure virtual, which is allowed by the style guide:
https://google.github.io/styleguide/cppguide.html#Multiple_Inheritance

[Michael Starzinger, 2016/04/29 10:59:10]

I don't have a strong opinion on this. Happy to separate it into two classes.
Either way is fine with me.

  public:
   SharedFunctionInfo* shared_;
   bool has_activations_;
@@ -875,12 +876,25 @@ class InterpreterActivationsFinder : public ThreadVisitor {
       if (frame->function()->shared() == shared_) has_activations_ = true;
     }
   }
+
+  void VisitFunction(JSFunction* function) {
+    if (function->Inlines(shared_)) has_activations_ = true;

[Yang, 2016/04/29 10:19:49]

We could be more precise here and only mark has_activations_ if the optimized
code actually deopts to bytecode.

[Michael Starzinger, 2016/04/29 10:59:10]

Not sure I understand entirely. The only way optimized code could exist without
the SharedFunctionInfo being in tier 2, is when it has been generated directly
from tier 1. That means all deopt points building a frame for {shared} pretty
much are guaranteed to be interpreter frames. So IMHO the check is already as
precise as it can be. Conversely in the --no-turbo-from-bytecode case we are
guaranteed to not have any optimized code inlining the SharedFunctionInfo in
question. Maybe I am missing something here.

+  }
+
+  void EnterContext(Context* context) {}
+  void LeaveContext(Context* context) {}
 };
 
 bool HasInterpreterActivations(Isolate* isolate, SharedFunctionInfo* shared) {
   InterpreterActivationsFinder activations_finder(shared);
   activations_finder.VisitThread(isolate, isolate->thread_local_top());
   isolate->thread_manager()->IterateArchivedThreads(&activations_finder);
+  if (FLAG_turbo_from_bytecode) {
+    // If we are able to optimize functions directly from bytecode, then there
+    // might be optimized functions that rely on bytecode being around. We need
+    // to prevent switching the given function to baseline code in those cases.
+    Deoptimizer::VisitAllOptimizedFunctions(isolate, &activations_finder);
+  }
   return activations_finder.has_activations_;
 }
 
@@ -1327,15 +1341,20 @@ bool Compiler::EnsureDeoptimizationSupport(CompilationInfo* info) {
   DCHECK_NOT_NULL(info->literal());
   DCHECK_NOT_NULL(info->scope());
   Handle<SharedFunctionInfo> shared = info->shared_info();
-  if (shared->HasBytecodeArray() &&
-      HasInterpreterActivations(info->isolate(), *shared)) {
-    // Do not tier up from here if we have bytecode on the stack.
-    return false;
-  }
   if (!shared->has_deoptimization_support()) {
     Zone zone(info->isolate()->allocator());
     CompilationInfo unoptimized(info->parse_info(), info->closure());
     unoptimized.EnableDeoptimizationSupport();
+    // TODO(4280): For now we disable switching to baseline code in the presence
+    // of interpreter activations of the given function. The reasons are:
+    //  1) The debugger assumes each function is either full-code or bytecode.
+    //  2) The underlying bytecode is cleared below, breaking stack unwinding.
+    // The expensive check for activations only needs to be done when the given
+    // function has bytecode, otherwise we can be sure there are no activations.
+    if (shared->HasBytecodeArray() &&
+        HasInterpreterActivations(info->isolate(), *shared)) {
+      return false;
+    }
     // If the current code has reloc info for serialization, also include
     // reloc info for serialization for the new code, so that deopt support
     // can be added without losing IC state.
@@ -1346,7 +1365,10 @@ bool Compiler::EnsureDeoptimizationSupport(CompilationInfo* info) {
     EnsureFeedbackVector(&unoptimized);
     if (!FullCodeGenerator::MakeCode(&unoptimized)) return false;
 
-    shared->EnableDeoptimizationSupport(*unoptimized.code());
+    // TODO(4280): For now we play it safe and remove the bytecode array when we
+    // switch to baseline code. We might consider keeping around the bytecode so
+    // that it can be used as the "source of truth" eventually.
+    shared->ClearBytecodeArray();
 
     // The scope info might not have been set if a lazily compiled
     // function is inlined before being called for the first time.
@@ -1354,6 +1376,9 @@ bool Compiler::EnsureDeoptimizationSupport(CompilationInfo* info) {
       InstallSharedScopeInfo(info, shared);
     }
 
+    // Install compilation result on the shared function info
+    shared->EnableDeoptimizationSupport(*unoptimized.code());

[Yang, 2016/04/29 10:19:49]

Is there a reason this has been moved to after installing scope info?

[Michael Starzinger, 2016/04/29 10:59:10]

No strong reason other than to bring the steps in the same order as in the other
compile functions (i.e. GetUnoptimizedCode and GetBaselineCode). Eventually I
would like to unify all these copies of the pipeline into one.

+
     // The existing unoptimized code was replaced with the new one.
     RecordFunctionCompilation(Logger::LAZY_COMPILE_TAG, &unoptimized);
   }