Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(313)

Issue 16579: Experimental: fix a bug where we could have returns from a function... (Closed)

Created:
11 years, 11 months ago by Kevin Millikin (Chromium)
Modified:
9 years, 7 months ago
Reviewers:
Kasper Lund
CC:
v8-dev
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+56 lines, -22 lines) Patch
M src/codegen-ia32.h View 1 chunk +6 lines, -0 lines 0 comments Download
M src/codegen-ia32.cc View 2 chunks +50 lines, -22 lines 6 comments Download

Messages

Total messages: 3 (0 generated)
Kevin Millikin (Chromium)
Kasper, could you take a look at this? You were looking at some of the ...
11 years, 11 months ago (2009-01-07 18:37:46 UTC) #1
Kasper Lund
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): ...
11 years, 11 months ago (2009-01-08 06:55:12 UTC) #2
Kevin Millikin (Chromium)
11 years, 11 months ago (2009-01-08 09:32:08 UTC) #3
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.

Powered by Google App Engine
This is Rietveld 408576698