[async-await] (simpler) fix for Return in try/finally in async functions

src/interpreter/bytecode-generator.cc

 // Copyright 2015 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/interpreter/bytecode-generator.h"
 
 #include "src/ast/compile-time-value.h"
 #include "src/ast/scopes.h"
 #include "src/builtins/builtins-constructor.h"
 #include "src/code-stubs.h"
@@ skipping 688 matching lines @@
   // since we skip some obviously dead code. Hence the generated bytecode may
   // contain jumps to unbound labels (resume points that will never be used).
   // We bind these now.
   for (auto& label : generator_resume_points_) {
     if (!label.is_bound()) builder()->Bind(&label);
   }
 
   // Emit an implicit return instruction in case control flow can fall off the
   // end of the function without an explicit return being present on all paths.
   if (builder()->RequiresImplicitReturn()) {
+    // Do a hard return as execution_control is the top level and will not
+    // issue a deferred return command.
+    DCHECK(execution_control() == &control);
     builder()->LoadUndefined();
-    BuildReturn();
+    BuildHardReturn();

[Dan Ehrenberg, 2017/02/09 06:40:00]

Why is this a hard return? I would expect implicit returns to resolve the
promise with 'undefined', rather than actually return 'undefined'.

[caitp, 2017/02/09 12:53:07]

This change is just too shrink bytecode a bit.

The parser inserts an actual "implicit return" as the last statement in the try
block, so this thing is actually unreachable. Even so, all blocks need a return,
so this generates "LdaUndefined Return" instead of the full promise resolution
thing, because for async functions and generators it doesn't actually matter.

[Dan Ehrenberg, 2017/02/09 13:23:36]

I thought this would be reached in a case like "async function() { }". How does
that end up resolving the Promise with undefined?

[rmcilroy, 2017/02/09 13:33:49]

I didn't realize this was only a to optimize bytecode size, I thought it was
necessary with the promise resolution. I'm a bit less happy with having this as
a hard return just because it's dead code in async code. Could we just make it a
normal return for now?

Going forward, if the parser is smart enough to add implicit returns for async
functions, could it add implicit returns for all functions? If so we could get
rid of this entirely.

[caitp, 2017/02/09 13:38:47]

My understanding is that this is needed to ensure that the last block has an
exit, in order to be a valid graph, which is presumably important for TF.

So, ordinary functions could have an inserted `return undefined` in the parser,
but the return that the parser inserts for async functions doesn't end up being
the exit for the last block in the function, due to the implicit try/catch. So
in that situation, the implicit return here would still be needed.

   }
   DCHECK(!builder()->RequiresImplicitReturn());
 }
 
 void BytecodeGenerator::GenerateBytecodeBody() {
   // Build the arguments object if it is used.
   VisitArgumentsObject(scope()->arguments());
 
   // Build rest arguments array if it is used.
   Variable* rest_parameter = scope()->rest_parameter();
@@ skipping 330 matching lines @@
 }
 
 void BytecodeGenerator::VisitBreakStatement(BreakStatement* stmt) {
   builder()->SetStatementPosition(stmt);
   execution_control()->Break(stmt->target());
 }
 
 void BytecodeGenerator::VisitReturnStatement(ReturnStatement* stmt) {
   builder()->SetStatementPosition(stmt);
   VisitForAccumulatorValue(stmt->expression());
+
+  if (stmt->is_hard_return()) {
+    // TODO(caitp): remove hard_return handling and ReturnStatement AST node
+    // flags once BytecodeGenerator performs all implicit control flow for
+    // async functions.
+    return BuildHardReturn();
+  }
+
   execution_control()->ReturnAccumulator();
 }
 
 void BytecodeGenerator::VisitWithStatement(WithStatement* stmt) {
   builder()->SetStatementPosition(stmt);
   VisitForAccumulatorValue(stmt->expression());
   BuildNewLocalWithContext(stmt->scope());
   VisitInScope(stmt->statement(), stmt->scope());
 }
 
@@ skipping 913 matching lines @@
 }
 
 void BytecodeGenerator::BuildVariableLoadForAccumulatorValue(
     Variable* variable, FeedbackSlot slot, HoleCheckMode hole_check_mode,
     TypeofMode typeof_mode) {
   ValueResultScope accumulator_result(this);
   BuildVariableLoad(variable, slot, hole_check_mode, typeof_mode);
 }
 
 void BytecodeGenerator::BuildReturn() {
+  if (IsAsyncFunction(info()->literal()->kind())) {
+    RegisterAllocationScope register_scope(this);
+    RegisterList args = register_allocator()->NewRegisterList(3);
+    Register receiver = args[0];
+    Register promise = args[1];
+    Register return_value = args[2];
+    builder()->StoreAccumulatorInRegister(return_value);
+
+    Variable* var_promise = scope()->promise_var();
+    DCHECK_NOT_NULL(var_promise);
+    BuildVariableLoad(var_promise, FeedbackSlot::Invalid(),
+                      HoleCheckMode::kElided);
+    builder()
+        ->StoreAccumulatorInRegister(promise)
+        .LoadUndefined()
+        .StoreAccumulatorInRegister(receiver)
+        .CallJSRuntime(Context::PROMISE_RESOLVE_INDEX, args)
+        .LoadAccumulatorWithRegister(promise);
+  }
+
+  BuildHardReturn();
+}
+
+void BytecodeGenerator::BuildHardReturn() {
   if (FLAG_trace) {
     RegisterAllocationScope register_scope(this);
     Register result = register_allocator()->NewRegister();
     // Runtime returns {result} value, preserving accumulator.
     builder()->StoreAccumulatorInRegister(result).CallRuntime(
         Runtime::kTraceExit, result);
   }
   builder()->Return();
 }
 
@@ skipping 1377 matching lines @@
 }
 
 Runtime::FunctionId BytecodeGenerator::StoreKeyedToSuperRuntimeId() {
   return is_strict(language_mode()) ? Runtime::kStoreKeyedToSuper_Strict
                                     : Runtime::kStoreKeyedToSuper_Sloppy;
 }
 
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8