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

src/codegen-ia32.cc

Index: src/codegen-ia32.cc
===================================================================
--- src/codegen-ia32.cc	(revision 1041)
+++ src/codegen-ia32.cc	(working copy)
@@ -286,15 +286,33 @@
 #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();
+      }
     }
   }
 
@@ -1647,28 +1665,38 @@
     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);
-      }
+      GenerateReturnSequence();
+    }
+  }
+}
 
-      // 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();
+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);
+  }
 
-      // 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));
-    }
-  }
+  // 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));
 }