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

Experimental: fix a bug where we could have returns from a function
but never generate the return sequence.

Committed: http://code.google.com/p/v8/source/detail?r=1043

[Kevin Millikin (Chromium), 2009-01-07 18:37:46]

Kasper, could you take a look at this?  You were looking at some of the same
code recently.

[Kasper Lund, 2009-01-08 06:55:12]

LGTM if the --trace option still works in all cases. Comments:

http://codereview.chromium.org/16579/diff/1/3
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/16579/diff/1/3#newcode306
Line 306: if (function_return_.is_linked()) {
Should (or could) this really be an else if?

http://codereview.chromium.org/16579/diff/1/3#newcode1682
Line 1682: frame_->Push(eax);  // Materialize result on the stack.
Is this really okay when the frame is invalid? Can you run the regression test
with --trace?

http://codereview.chromium.org/16579/diff/1/3#newcode1687
Line 1687: Label check_exit_codesize;
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]

http://codereview.chromium.org/16579/diff/1/3
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/16579/diff/1/3#newcode306
Line 306: if (function_return_.is_linked()) {
On 2009/01/08 06:55:13, Kasper Lund wrote:
> Should (or could) this really be an else if?

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

http://codereview.chromium.org/16579/diff/1/3#newcode1682
Line 1682: frame_->Push(eax);  // Materialize result on the stack.
On 2009/01/08 06:55:13, Kasper Lund wrote:
> Is this really okay when the frame is invalid? Can you run the regression test
> with --trace?

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

http://codereview.chromium.org/16579/diff/1/3#newcode1687
Line 1687: Label check_exit_codesize;
On 2009/01/08 06:55:13, Kasper Lund wrote:
> 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.

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.