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

src/compiler.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/compiler.h"
 
 #include <algorithm>
 
 #include "src/ast/ast-numbering.h"
 #include "src/ast/prettyprinter.h"
@@ skipping 841 matching lines @@
     }
   } else {
     info->set_osr_frame(osr_frame);
     if (GetOptimizedCodeNow(job.get())) return info->code();
   }
 
   if (isolate->has_pending_exception()) isolate->clear_pending_exception();
   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_;
 
   explicit InterpreterActivationsFinder(SharedFunctionInfo* shared)
       : shared_(shared), has_activations_(false) {}
 
   void VisitThread(Isolate* isolate, ThreadLocalTop* top) {
     JavaScriptFrameIterator it(isolate, top);
     for (; !it.done() && !has_activations_; it.Advance()) {
       JavaScriptFrame* frame = it.frame();
       if (!frame->is_interpreted()) continue;
       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_;
 }
 
 MaybeHandle<Code> GetBaselineCode(Handle<JSFunction> function) {
   Isolate* isolate = function->GetIsolate();
   VMState<COMPILER> state(isolate);
   PostponeInterruptsScope postpone(isolate);
   Zone zone(isolate->allocator());
   ParseInfo parse_info(&zone, function);
   CompilationInfo info(&parse_info, function);
@@ skipping 426 matching lines @@
   DCHECK(!isolate->has_pending_exception());
   return true;
 }
 
 // TODO(turbofan): In the future, unoptimized code with deopt support could
 // be generated lazily once deopt is triggered.
 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.
     if (shared->code()->kind() == Code::FUNCTION &&
         shared->code()->has_reloc_info_for_serialization()) {
       unoptimized.PrepareForSerializing();
     }
     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.
     if (shared->scope_info() == ScopeInfo::Empty(info->isolate())) {
       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);
   }
   return true;
 }
 
 MaybeHandle<JSFunction> Compiler::GetFunctionFromEval(
     Handle<String> source, Handle<SharedFunctionInfo> outer_info,
     Handle<Context> context, LanguageMode language_mode,
     ParseRestriction restriction, int eval_scope_position, int eval_position,
@@ skipping 447 matching lines @@
     MaybeHandle<Code> code;
     if (cached.code != nullptr) code = handle(cached.code);
     Handle<Context> native_context(function->context()->native_context());
     SharedFunctionInfo::AddToOptimizedCodeMap(shared, native_context, code,
                                               literals, BailoutId::None());
   }
 }
 
 }  // namespace internal
 }  // namespace v8