Improve internal compiler API so that OSR code is never installed on function.

runtime/vm/compiler.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/compiler.h"
 
 #include "vm/assembler.h"
 
 #include "vm/ast_printer.h"
 #include "vm/block_scheduler.h"
@@ skipping 186 matching lines @@
     return new (zone) DartCompilationPipeline();
   }
 }
 
 
 // Compile a function. Should call only if the function has not been compiled.
 //   Arg0: function object.
 DEFINE_RUNTIME_ENTRY(CompileFunction, 1) {
   const Function& function = Function::CheckedHandle(arguments.ArgAt(0));
   ASSERT(!function.HasCode());
-  const Error& error =
-      Error::Handle(Compiler::CompileFunction(thread, function));
-  if (!error.IsNull()) {
-    if (error.IsLanguageError()) {
-      Exceptions::ThrowCompileTimeError(LanguageError::Cast(error));
+  const Object& result =
+      Object::Handle(Compiler::CompileFunction(thread, function));
+  if (result.IsError()) {
+    if (result.IsLanguageError()) {
+      Exceptions::ThrowCompileTimeError(LanguageError::Cast(result));
       UNREACHABLE();
     }
-    Exceptions::PropagateError(error);
+    Exceptions::PropagateError(Error::Cast(result));
   }
 }
 
 
 bool Compiler::CanOptimizeFunction(Thread* thread, const Function& function) {
   if (FLAG_support_debugger) {
     Isolate* isolate = thread->isolate();
     if (isolate->debugger()->IsStepping() ||
         isolate->debugger()->HasBreakpoint(function, thread->zone())) {
       // We cannot set breakpoints and single step in optimized code,
@@ skipping 264 matching lines @@
   CompileParsedFunctionHelper(ParsedFunction* parsed_function,
                               bool optimized,
                               intptr_t osr_id)
       : parsed_function_(parsed_function),
         optimized_(optimized),
         osr_id_(osr_id),
         thread_(Thread::Current()),
         loading_invalidation_gen_at_start_(
             isolate()->loading_invalidation_gen()) {}
 
-  bool Compile(CompilationPipeline* pipeline);
+  RawObject* Compile(CompilationPipeline* pipeline);
 
  private:
   ParsedFunction* parsed_function() const { return parsed_function_; }
   bool optimized() const { return optimized_; }
   intptr_t osr_id() const { return osr_id_; }
   Thread* thread() const { return thread_; }
   Isolate* isolate() const { return thread_->isolate(); }
   intptr_t loading_invalidation_gen_at_start() const {
     return loading_invalidation_gen_at_start_;
   }
-  void FinalizeCompilation(Assembler* assembler,
-                           FlowGraphCompiler* graph_compiler,
-                           FlowGraph* flow_graph);
+  RawObject* FinalizeCompilation(Assembler* assembler,
+                                 FlowGraphCompiler* graph_compiler,
+                                 FlowGraph* flow_graph);
   void CheckIfBackgroundCompilerIsBeingStopped();
 
   ParsedFunction* parsed_function_;
   const bool optimized_;
   const intptr_t osr_id_;
   Thread* const thread_;
   const intptr_t loading_invalidation_gen_at_start_;
 
   DISALLOW_COPY_AND_ASSIGN(CompileParsedFunctionHelper);
 };
 
 
-void CompileParsedFunctionHelper::FinalizeCompilation(
+RawObject* CompileParsedFunctionHelper::FinalizeCompilation(

[Vyacheslav Egorov (Google), 2017/03/30 09:58:52]

It looks like this function always returns a Code - so make it RawCode*

[erikcorry, 2017/03/30 12:26:33]

Done.

     Assembler* assembler,
     FlowGraphCompiler* graph_compiler,
     FlowGraph* flow_graph) {
   ASSERT(!FLAG_precompiled_mode);
   const Function& function = parsed_function()->function();
   Zone* const zone = thread()->zone();
 
   CSTAT_TIMER_SCOPE(thread(), codefinalizer_timer);
   // CreateDeoptInfo uses the object pool and needs to be done before
   // FinalizeCode.
@@ skipping 44 matching lines @@
   graph_compiler->FinalizePcDescriptors(code);
   code.set_deopt_info_array(deopt_info_array);
 
   graph_compiler->FinalizeStackMaps(code);
   graph_compiler->FinalizeVarDescriptors(code);
   graph_compiler->FinalizeExceptionHandlers(code);
   graph_compiler->FinalizeCatchEntryStateMap(code);
   graph_compiler->FinalizeStaticCallTargetsTable(code);
   graph_compiler->FinalizeCodeSourceMap(code);
 
+  bool code_was_compiled = false;

[Vyacheslav Egorov (Google), 2017/03/30 09:58:52]

Maybe you don't need this? 

How about just using code and setting it to `null` if validation failed.

[erikcorry, 2017/03/30 12:26:33]

Done.

+
   if (optimized()) {
-    bool code_was_installed = false;
     // Installs code while at safepoint.
     if (thread()->IsMutatorThread()) {
       const bool is_osr = osr_id() != Compiler::kNoOSRDeoptId;
-      function.InstallOptimizedCode(code, is_osr);
-      code_was_installed = true;
+      if (!is_osr) {

[Vyacheslav Egorov (Google), 2017/03/30 09:58:52]

InstallOptimizedCode() also does set the owner on the code (in the AttachCode).

Who does that for the OSR code? Maybe this needs to be done manually. 

[erikcorry, 2017/03/30 12:26:34]

That's done on line 545.  I added an assert.

+        function.InstallOptimizedCode(code);
+      }
+      code_was_compiled = true;
     } else {
       // Background compilation.
       // Before installing code check generation counts if the code may
       // have become invalid.
       const bool trace_compiler =
           FLAG_trace_compiler || FLAG_trace_optimizing_compiler;
       bool code_is_valid = true;
       if (!flow_graph->parsed_function().guarded_fields()->is_empty()) {
         const ZoneGrowableArray<const Field*>& guarded_fields =
             *flow_graph->parsed_function().guarded_fields();
@@ skipping 23 matching lines @@
         code_is_valid = false;
         if (trace_compiler) {
           THR_Print("--> FAIL: Class hierarchy has new subclasses.");
         }
       }
 
       // Setting breakpoints at runtime could make a function non-optimizable.
       if (code_is_valid && Compiler::CanOptimizeFunction(thread(), function)) {
         const bool is_osr = osr_id() != Compiler::kNoOSRDeoptId;
         ASSERT(!is_osr);  // OSR is not compiled in background.
-        function.InstallOptimizedCode(code, is_osr);
-        code_was_installed = true;
+        function.InstallOptimizedCode(code);
+        code_was_compiled = true;
       }

[Vyacheslav Egorov (Google), 2017/03/30 09:58:52]

} else {
  code = Code::null();
}

and use !code.IsNull() everywhere instead of code_was_compiled.

[erikcorry, 2017/03/30 12:26:34]

Done.

       if (function.usage_counter() < 0) {
         // Reset to 0 so that it can be recompiled if needed.
         if (code_is_valid) {
           function.set_usage_counter(0);
         } else {
           // Trigger another optimization pass soon.
           function.set_usage_counter(FLAG_optimization_counter_threshold - 100);
         }
       }
     }
 
-    if (code_was_installed) {
+    if (code_was_compiled) {
       // The generated code was compiled under certain assumptions about
       // class hierarchy and field types. Register these dependencies
       // to ensure that the code will be deoptimized if they are violated.
       thread()->cha()->RegisterDependencies(code);
 
       const ZoneGrowableArray<const Field*>& guarded_fields =
           *flow_graph->parsed_function().guarded_fields();
       Field& field = Field::Handle();
       for (intptr_t i = 0; i < guarded_fields.length(); i++) {
         field = guarded_fields[i]->Original();
         field.RegisterDependentCode(code);
       }
     }
   } else {  // not optimized.
     if (function.ic_data_array() == Array::null()) {
       function.SaveICDataMap(
           graph_compiler->deopt_id_to_ic_data(),
           Array::Handle(zone, graph_compiler->edge_counters_array()));
     }
     function.set_unoptimized_code(code);
     function.AttachCode(code);
+    code_was_compiled = true;
   }
   if (parsed_function()->HasDeferredPrefixes()) {
     ASSERT(!FLAG_load_deferred_eagerly);
     ZoneGrowableArray<const LibraryPrefix*>* prefixes =
         parsed_function()->deferred_prefixes();
     for (intptr_t i = 0; i < prefixes->length(); i++) {
       (*prefixes)[i]->RegisterDependentCode(code);
     }
   }
+  if (code_was_compiled) {
+    return code.raw();
+  }
+  return Object::null();

[Vyacheslav Egorov (Google), 2017/03/30 09:58:52]

here you then will be able to just return code.raw().

[erikcorry, 2017/03/30 12:26:34]

Done.

 }
 
 
 void CompileParsedFunctionHelper::CheckIfBackgroundCompilerIsBeingStopped() {
   ASSERT(Compiler::IsBackgroundCompilation());
   if (!isolate()->background_compiler()->is_running()) {
     // The background compiler is being stopped.
     Compiler::AbortBackgroundCompilation(
         Thread::kNoDeoptId, "Background compilation is being stopped");
   }
 }
 
 
-// Return false if bailed out.
+// Return null if bailed out.
 // If optimized_result_code is not NULL then it is caller's responsibility
 // to install code.
-bool CompileParsedFunctionHelper::Compile(CompilationPipeline* pipeline) {
+RawObject* CompileParsedFunctionHelper::Compile(CompilationPipeline* pipeline) {

[Vyacheslav Egorov (Google), 2017/03/30 09:58:52]

This returns Code or null. So maybe this should be RawCode*?

[erikcorry, 2017/03/30 12:26:33]

Done.

   ASSERT(!FLAG_precompiled_mode);
   const Function& function = parsed_function()->function();
   if (optimized() && !function.IsOptimizable()) {
-    return false;
+    return Object::null();
   }
-  bool is_compiled = false;
   Zone* const zone = thread()->zone();
   NOT_IN_PRODUCT(TimelineStream* compiler_timeline =
                      Timeline::GetCompilerStream());
   CSTAT_TIMER_SCOPE(thread(), codegen_timer);
   HANDLESCOPE(thread());
 
   // We may reattempt compilation if the function needs to be assembled using
   // far branches on ARM and MIPS. In the else branch of the setjmp call,
   // done is set to false, and use_far_branches is set to true if there is a
   // longjmp from the ARM or MIPS assemblers. In all other paths through this
   // while loop, done is set to true. use_far_branches is always false on ia32
   // and x64.
   volatile bool done = false;
   // volatile because the variable may be clobbered by a longjmp.
   volatile bool use_far_branches = false;
   const bool use_speculative_inlining = false;
 
+  Code& result = Code::ZoneHandle(zone);

[Vyacheslav Egorov (Google), 2017/03/30 09:58:52]

this variable needs to be volatile because of setjmp(...).

[erikcorry, 2017/03/30 12:26:33]

Yuck!

   while (!done) {
+    result = Code::null();
     const intptr_t prev_deopt_id = thread()->deopt_id();
     thread()->set_deopt_id(0);
     LongJumpScope jump;
     const intptr_t val = setjmp(*jump.Set());
     if (val == 0) {
       FlowGraph* flow_graph = NULL;
 
       // Class hierarchy analysis is registered with the thread in the
       // constructor and unregisters itself upon destruction.
       CHA cha(thread());
@@ skipping 392 matching lines @@
         CSTAT_TIMER_SCOPE(thread(), graphcompiler_timer);
         NOT_IN_PRODUCT(TimelineDurationScope tds(thread(), compiler_timeline,
                                                  "CompileGraph"));
         graph_compiler.CompileGraph();
         pipeline->FinalizeCompilation(flow_graph);
       }
       {
         NOT_IN_PRODUCT(TimelineDurationScope tds(thread(), compiler_timeline,
                                                  "FinalizeCompilation"));
         if (thread()->IsMutatorThread()) {
-          FinalizeCompilation(&assembler, &graph_compiler, flow_graph);
+          result ^=
+              FinalizeCompilation(&assembler, &graph_compiler, flow_graph);

[Vyacheslav Egorov (Google), 2017/03/30 09:58:52]

This can become result = FinalizeCompilation() if you change FinalizeCompilation
to just return RawCode* - which I think it always does.

[erikcorry, 2017/03/30 12:26:33]

Done.

         } else {
           // This part of compilation must be at a safepoint.
           // Stop mutator thread before creating the instruction object and
           // installing code.
           // Mutator thread may not run code while we are creating the
           // instruction object, since the creation of instruction object
           // changes code page access permissions (makes them temporary not
           // executable).
           {
             CheckIfBackgroundCompilerIsBeingStopped();
             SafepointOperationScope safepoint_scope(thread());
             // Do not Garbage collect during this stage and instead allow the
             // heap to grow.
             NoHeapGrowthControlScope no_growth_control;
             CheckIfBackgroundCompilerIsBeingStopped();
-            FinalizeCompilation(&assembler, &graph_compiler, flow_graph);
+            result ^=
+                FinalizeCompilation(&assembler, &graph_compiler, flow_graph);
           }
           // TODO(srdjan): Enable this and remove the one from
           // 'BackgroundCompiler::CompileOptimized' once cause of time-outs
           // is resolved.
           // if (isolate()->heap()->NeedsGarbageCollection()) {
           //   isolate()->heap()->CollectAllGarbage();
           // }
         }
       }
       // Exit the loop and the function with the correct result value.
-      is_compiled = true;
       done = true;
     } else {
       // We bailed out or we encountered an error.
       const Error& error = Error::Handle(thread()->sticky_error());
 
       if (error.raw() == Object::branch_offset_error().raw()) {
         // Compilation failed due to an out of range branch offset in the
         // assembler. We try again (done = false) with far branches enabled.
         done = false;
         ASSERT(!use_far_branches);
         use_far_branches = true;
       } else if (error.raw() == Object::speculative_inlining_error().raw()) {
         // Can only happen with precompilation.
         UNREACHABLE();
       } else {
         // If the error isn't due to an out of range branch offset, we don't
-        // try again (done = true), and indicate that we did not finish
-        // compiling (is_compiled = false).
+        // try again (done = true).
         if (FLAG_trace_bailout) {
           THR_Print("%s\n", error.ToErrorCString());
         }
         done = true;
       }
 
       // If is is not a background compilation, clear the error if it was not a
       // real error, but just a bailout. If we're it a background compilation
       // this will be dealt with in the caller.
       if (!Compiler::IsBackgroundCompilation() && error.IsLanguageError() &&
           (LanguageError::Cast(error).kind() == Report::kBailout)) {
         thread()->clear_sticky_error();
       }
-      is_compiled = false;
     }
     // Reset global isolate state.
     thread()->set_deopt_id(prev_deopt_id);
   }
-  return is_compiled;
+  return result.raw();
 }
 
 
-static RawError* CompileFunctionHelper(CompilationPipeline* pipeline,
-                                       const Function& function,
-                                       bool optimized,
-                                       intptr_t osr_id) {
+static RawObject* CompileFunctionHelper(CompilationPipeline* pipeline,
+                                        const Function& function,
+                                        bool optimized,
+                                        intptr_t osr_id) {
   ASSERT(!FLAG_precompiled_mode);
   ASSERT(!optimized || function.was_compiled());
   LongJumpScope jump;
   if (setjmp(*jump.Set()) == 0) {
     Thread* const thread = Thread::Current();
     Isolate* const isolate = thread->isolate();
     StackZone stack_zone(thread);
     Zone* const zone = stack_zone.GetZone();
     const bool trace_compiler =
         FLAG_trace_compiler || (FLAG_trace_optimizing_compiler && optimized);
@@ skipping 35 matching lines @@
           (loading_invalidation_gen_at_start !=
            isolate->loading_invalidation_gen())) {
         // Loading occured while parsing. We need to abort here because state
         // changed while compiling.
         Compiler::AbortBackgroundCompilation(
             Thread::kNoDeoptId,
             "Invalidated state during parsing because of script loading");
       }
     }
 
-    const bool success = helper.Compile(pipeline);
-    if (success) {
+    Object& result = Object::Handle(helper.Compile(pipeline));

[Vyacheslav Egorov (Google), 2017/03/30 09:58:52]

This probably needs to be Code because you expect it to be either Code object or
null.

[erikcorry, 2017/03/30 12:26:33]

Done.

+    if (!result.IsNull()) {
       if (!optimized) {
         function.set_was_compiled(true);
       }
     } else {
       if (optimized) {
         if (Compiler::IsBackgroundCompilation()) {
           // Try again later, background compilation may abort because of
           // state change during compilation.
           if (FLAG_trace_compiler) {
             THR_Print("Aborted background compilation: %s\n",
@@ skipping 21 matching lines @@
         if (trace_compiler) {
           THR_Print("--> disabling optimizations for '%s'\n",
                     function.ToFullyQualifiedCString());
         } else if (FLAG_trace_failed_optimization_attempts) {
           THR_Print("Cannot optimize: %s\n",
                     function.ToFullyQualifiedCString());
         }
         function.SetIsOptimizable(false);
         return Error::null();
       } else {
+        ASSERT(!optimized);

[Vyacheslav Egorov (Google), 2017/03/30 09:58:52]

I don't get this code. We are inside 

if (optimized) {
  ...
}

condition. 

[erikcorry, 2017/03/30 12:26:33]

No.  I added the assert exactly to clarify which "{" the "}" belonged to.

         // Encountered error.
         Error& error = Error::Handle();
         // We got an error during compilation.
         error = thread->sticky_error();
         thread->clear_sticky_error();
         // The non-optimizing compiler can get an unhandled exception
         // due to OOM or Stack overflow errors, it should not however
         // bail out.
         ASSERT(error.IsUnhandledException() ||
                (error.IsLanguageError() &&
                 LanguageError::Cast(error).kind() != Report::kBailout));
         return error.raw();
       }
+      UNREACHABLE();
     }
 
     per_compile_timer.Stop();
 
-    if (trace_compiler && success) {
+    if (trace_compiler) {
       THR_Print("--> '%s' entry: %#" Px " size: %" Pd " time: %" Pd64 " us\n",
                 function.ToFullyQualifiedCString(),
                 Code::Handle(function.CurrentCode()).PayloadStart(),
                 Code::Handle(function.CurrentCode()).Size(),
                 per_compile_timer.TotalElapsedTime());
     }
 
     if (FLAG_support_debugger) {
       isolate->debugger()->NotifyCompilation(function);
     }
 
     if (FLAG_disassemble && FlowGraphPrinter::ShouldPrint(function)) {
       Disassembler::DisassembleCode(function, optimized);
     } else if (FLAG_disassemble_optimized && optimized &&
                FlowGraphPrinter::ShouldPrint(function)) {
       Disassembler::DisassembleCode(function, true);
     }
 
-    return Error::null();
+    return result.raw();
   } else {
     Thread* const thread = Thread::Current();
     StackZone stack_zone(thread);
     Error& error = Error::Handle();
     // We got an error during compilation or it is a bailout from background
     // compilation (e.g., during parsing with EnsureIsFinalized).
     error = thread->sticky_error();
     thread->clear_sticky_error();
     if (error.raw() == Object::background_compilation_error().raw()) {
       // Exit compilation, retry it later.
@@ skipping 62 matching lines @@
     // Unoptimized compilation or precompilation may encounter compile-time
     // errors, but regular optimized compilation should not.
     ASSERT(!optimized);
     return error.raw();
   }
   UNREACHABLE();
   return Error::null();
 }
 
 
-RawError* Compiler::CompileFunction(Thread* thread, const Function& function) {
+RawObject* Compiler::CompileFunction(Thread* thread, const Function& function) {
 #ifdef DART_PRECOMPILER
   if (FLAG_precompiled_mode) {
     return Precompiler::CompileFunction(
         /* precompiler = */ NULL, thread, thread->zone(), function);
   }
 #endif
 
   Isolate* isolate = thread->isolate();
 
 #if !defined(PRODUCT)
@@ skipping 42 matching lines @@
                                           const Function& function) {
   if (function.unoptimized_code() != Object::null()) {
     return Error::null();
   }
   Code& original_code = Code::ZoneHandle(thread->zone());
   if (function.HasCode()) {
     original_code = function.CurrentCode();
   }
   CompilationPipeline* pipeline =
       CompilationPipeline::New(thread->zone(), function);
-  const Error& error = Error::Handle(
+  const Object& result = Object::Handle(
       CompileFunctionHelper(pipeline, function, false, /* not optimized */
                             kNoOSRDeoptId));
-  if (!error.IsNull()) {
-    return error.raw();
+  if (result.IsError()) {
+    return Error::Cast(result).raw();
   }
   // Since CompileFunctionHelper replaces the current code, re-attach the
   // the original code if the function was already compiled.
-  if (!original_code.IsNull() &&
-      (original_code.raw() != function.CurrentCode())) {
+  if (!original_code.IsNull() && result.raw() == function.CurrentCode() &&
+      !original_code.IsDisabled()) {
     function.AttachCode(original_code);
   }
   ASSERT(function.unoptimized_code() != Object::null());
+  ASSERT(function.unoptimized_code() == result.raw());
   if (FLAG_trace_compiler) {
     THR_Print("Ensure unoptimized code for %s\n", function.ToCString());
   }
   return Error::null();
 }
 
 
-RawError* Compiler::CompileOptimizedFunction(Thread* thread,
-                                             const Function& function,
-                                             intptr_t osr_id) {
+RawObject* Compiler::CompileOptimizedFunction(Thread* thread,
+                                              const Function& function,
+                                              intptr_t osr_id) {
 #if !defined(PRODUCT)
   VMTagScope tagScope(thread, VMTag::kCompileOptimizedTagId);
   const char* event_name;
   if (osr_id != kNoOSRDeoptId) {
     event_name = "CompileFunctionOptimizedOSR";
   } else if (IsBackgroundCompilation()) {
     event_name = "CompileFunctionOptimizedBackground";
   } else {
     event_name = "CompileFunctionOptimized";
   }
@@ skipping 62 matching lines @@
   } else {
     // Only possible with background compilation.
     ASSERT(Compiler::IsBackgroundCompilation());
   }
 }
 
 
 RawError* Compiler::CompileAllFunctions(const Class& cls) {
   Thread* thread = Thread::Current();
   Zone* zone = thread->zone();
-  Error& error = Error::Handle(zone);
+  Object& result = Object::Handle(zone);
   Array& functions = Array::Handle(zone, cls.functions());
   Function& func = Function::Handle(zone);
   // Class dynamic lives in the vm isolate. Its array fields cannot be set to
   // an empty array.
   if (functions.IsNull()) {
     ASSERT(cls.IsDynamicClass());
-    return error.raw();
+    return Error::null();
   }
   // Compile all the regular functions.
   for (int i = 0; i < functions.Length(); i++) {
     func ^= functions.At(i);
     ASSERT(!func.IsNull());
     if (!func.HasCode() && !func.is_abstract() &&
         !func.IsRedirectingFactory()) {
       if ((cls.is_mixin_app_alias() || cls.IsMixinApplication()) &&
           func.HasOptionalParameters()) {
         // Skipping optional parameters in mixin application.
         continue;
       }
-      error = CompileFunction(thread, func);
-      if (!error.IsNull()) {
-        return error.raw();
+      result = CompileFunction(thread, func);
+      if (result.IsError()) {
+        return Error::Cast(result).raw();
       }
+      ASSERT(!result.IsNull());
       func.ClearICDataArray();
       func.ClearCode();
     }
   }
-  return error.raw();
+  return Error::null();
 }
 
 
 RawError* Compiler::ParseAllFunctions(const Class& cls) {
   Thread* thread = Thread::Current();
   Zone* zone = thread->zone();
   Error& error = Error::Handle(zone);
   Array& functions = Array::Handle(zone, cls.functions());
   Function& func = Function::Handle(zone);
   // Class dynamic lives in the vm isolate. Its array fields cannot be set to
@@ skipping 556 matching lines @@
   return Error::null();
 }
 
 
 RawError* Compiler::CompileClass(const Class& cls) {
   UNREACHABLE();
   return Error::null();
 }
 
 
-RawError* Compiler::CompileFunction(Thread* thread, const Function& function) {
+RawObject* Compiler::CompileFunction(Thread* thread, const Function& function) {
   UNREACHABLE();
   return Error::null();
 }
 
 
 RawError* Compiler::ParseFunction(Thread* thread, const Function& function) {
   UNREACHABLE();
   return Error::null();
 }
 
@@ skipping 86 matching lines @@
 
 
 bool BackgroundCompiler::IsDisabled() {
   UNREACHABLE();
   return true;
 }
 
 #endif  // DART_PRECOMPILED_RUNTIME
 
 }  // namespace dart