Experimental: fix a bug where we could have returns from a function...

src/codegen-ia32.cc

 // Copyright 2006-2008 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 268 matching lines @@
       bool is_builtin = Bootstrapper::IsActive();
       bool should_trace =
           is_builtin ? FLAG_trace_builtin_calls : FLAG_trace_calls;
       if (should_trace) {
         frame_->CallRuntime(Runtime::kDebugTrace, 0);
         // Ignore the return value.
       }
 #endif
       VisitStatements(body);
 
-      // Generate a return statement if necessary.  A NULL frame indicates
-      // that control flow leaves the body on all paths and cannot fall
-      // through.
+      // Handle the return from the function.  If there is a valid frame,
+      // control flow can fall off the end of the body.  Compiling a return
+      // statement will jump to the return sequence if it is already
+      // generated or generate it if not.
       if (has_valid_frame()) {
+        ASSERT(!function_return_is_shadowed_);
         Literal undefined(Factory::undefined_value());
         ReturnStatement statement(&undefined);
         statement.set_statement_pos(fun->end_position());
         VisitReturnStatement(&statement);
       }
+
+      // If the return target has dangling jumps to it, then we have not yet
+      // generated the return sequence.  This can happen when (a) control
+      // does not flow off the end of the body so we did not compile an
+      // artificial return statement just above, and (b) there are return
+      // statements in the body but (c) they are all shadowed.
+      if (function_return_.is_linked()) {

[Kasper Lund, 2009/01/08 06:55:13]

Should (or could) this really be an else if?

[Kevin Millikin (Chromium), 2009/01/08 09:32:08]

(It could and) maybe it should.  I've changed it.

+        // There is no valid frame here.  It is safe (also necessary) to
+        // load the return value into eax because the frame we will pick up by
+        // binding the function return target is prepared for the return by
+        // spilling all registers, and binding it will not generate merge
+        // code that could use eax.
+        ASSERT(!has_valid_frame());
+        __ mov(eax, Immediate(Factory::undefined_value()));
+        GenerateReturnSequence();
+      }
     }
   }
 
   // Adjust for function-level loop nesting.
   loop_nesting_ -= fun->loop_nesting();
 
   // Code generation state must be reset.
   ASSERT(state_ == NULL);
   ASSERT(loop_nesting() == 0);
   ASSERT(!function_return_is_shadowed_);
@@ skipping 1332 matching lines @@
     result.ToRegister(eax);
     // TODO(): Instead of explictly calling Unuse on the result, it
     // might be better to pass the result to Jump and Bind below.
     result.Unuse();
 
     // If the function return label is already bound, we reuse the
     // code by jumping to the return site.
     if (function_return_.is_bound()) {
       function_return_.Jump();
     } else {
-      function_return_.Bind();
-      if (FLAG_trace) {
-        frame_->Push(eax);  // Materialize result on the stack.
-        frame_->CallRuntime(Runtime::kTraceExit, 1);
-      }
-
-      // Add a label for checking the size of the code used for returning.
-      Label check_exit_codesize;
-      __ bind(&check_exit_codesize);
-
-      // Leave the frame and return popping the arguments and the
-      // receiver.
-      frame_->Exit();
-      __ ret((scope_->num_parameters() + 1) * kPointerSize);
-      DeleteFrame();
-
-      // Check that the size of the code used for returning matches what is
-      // expected by the debugger.
-      ASSERT_EQ(Debug::kIa32JSReturnSequenceLength,
-                __ SizeOfCodeGeneratedSince(&check_exit_codesize));
+      GenerateReturnSequence();
     }
   }
 }
 
 
+void CodeGenerator::GenerateReturnSequence() {
+  // The return value is a live (but not currently reference counted)
+  // reference to eax.  This is safe because the frame we will pick up by
+  // binding the function return target does not contain a reference to eax
+  // (it is prepared for the return by spilling all registers) and binding
+  // will not emit merge code that could potentially use eax.
+  function_return_.Bind();
+  if (FLAG_trace) {
+    frame_->Push(eax);  // Materialize result on the stack.

[Kasper Lund, 2009/01/08 06:55:13]

Is this really okay when the frame is invalid? Can you run the regression test
with --trace?

[Kevin Millikin (Chromium), 2009/01/08 09:32:08]

It's OK here because we actually have a valid frame.  I've refactored to make it
clearer and tested --trace.

+    frame_->CallRuntime(Runtime::kTraceExit, 1);
+  }
+
+  // Add a label for checking the size of the code used for returning.
+  Label check_exit_codesize;

[Kasper Lund, 2009/01/08 06:55:13]

The more I look at this size checking construct, there more I feel it needs an
abstraction. How about having a stack-allocated helper that verifies the
generated code size? It seems like it's only used in this one place, but
whenever I read this code I wonder who jumps to the label that we explicitly
bind.

[Kevin Millikin (Chromium), 2009/01/08 09:32:08]

I think it's OK.  The label's lifetime ends pretty quickly, so it's easy to see
that nobody jumps to it.  With JumpTargets labeling basic blocks, the vanilla
Labels are only used for local things like this, so that should also help the
reader.

+  __ bind(&check_exit_codesize);
+
+  // Leave the frame and return popping the arguments and the
+  // receiver.
+  frame_->Exit();
+  __ ret((scope_->num_parameters() + 1) * kPointerSize);
+  DeleteFrame();
+
+  // Check that the size of the code used for returning matches what is
+  // expected by the debugger.
+  ASSERT_EQ(Debug::kIa32JSReturnSequenceLength,
+            __ SizeOfCodeGeneratedSince(&check_exit_codesize));
+}
+
+
 void CodeGenerator::VisitWithEnterStatement(WithEnterStatement* node) {
   ASSERT(!in_spilled_code());
   Comment cmnt(masm_, "[ WithEnterStatement");
   CodeForStatement(node);
   Load(node->expression());
   Result context(this);
   if (node->is_catch_block()) {
     context = frame_->CallRuntime(Runtime::kPushCatchContext, 1);
   } else {
     context = frame_->CallRuntime(Runtime::kPushContext, 1);
@@ skipping 4282 matching lines @@
 
   // Slow-case: Go through the JavaScript implementation.
   __ bind(&slow);
   __ InvokeBuiltin(Builtins::INSTANCE_OF, JUMP_FUNCTION);
 }
 
 
 #undef __
 
 } }  // namespace v8::internal