Fast-codegen: Implementing try/finally on top of nesting context.

Fast-codegen: Implementing try/finally on top of nesting context.

[Lasse Reichstein, 2009-12-10 09:24:59]

Depends on http://codereview.chromium.org/466033/show

[Kevin Millikin (Chromium), 2009-12-10 13:51:12]

LGTM.

I like the code for try/finally much better than the optimizing compiler.

http://codereview.chromium.org/492002/diff/24/27
File src/fast-codegen.cc (right):

http://codereview.chromium.org/492002/diff/24/27#newcode492
src/fast-codegen.cc:492: // The try-finally construct can enter the finally
block in three ways:
Consider some judicious code generator comments, to possibly help a reader
understand the generated code.

http://codereview.chromium.org/492002/diff/24/27#newcode499
src/fast-codegen.cc:499: // the fially block through finally_entry label.
"fially" ==> "finally"

http://codereview.chromium.org/492002/diff/24/27#newcode504
src/fast-codegen.cc:504: // and a value in the result register (rax/eax/r0),
both of which must
Probably should note that the value may be the return value or an exception
value.

http://codereview.chromium.org/492002/diff/24/27#newcode525
src/fast-codegen.cc:525: Visit(stmt->finally_block());
Isn't it OK to VisitStatements the body of this block?  (We do in the optimizing
backend.)  The difference would only be breaks targeting the block, right?

We should do the same thing for the try and finally blocks (unless there's a
reason), otherwise the reader will wonder.

http://codereview.chromium.org/492002/diff/24/27#newcode526
src/fast-codegen.cc:526: ReturnFromFinallyBlock();  // Performs a return to the
calling code.
Can we call this ExitFinallyBlock, to parallel EnterFinallyBlock and because
"return" is ambiguous?

http://codereview.chromium.org/492002/diff/24/27#newcode532
src/fast-codegen.cc:532: __ PushTryHandler(IN_JAVASCRIPT, TRY_FINALLY_HANDLER);
It seems nicer to properly nest the Push/Pop of the handler with the nesting
stack:

{
  TryFinally try_block(this);
  __ PushTryHandler(...);
  /* ... */
  __ PopTryHandler();
}

http://codereview.chromium.org/492002/diff/24/29
File src/ia32/fast-codegen-ia32.cc (right):

http://codereview.chromium.org/492002/diff/24/29#newcode1642
src/ia32/fast-codegen-ia32.cc:1642: __ mov(edx, Operand(esp, 0));
Assert result_register is not edx?

http://codereview.chromium.org/492002/diff/24/29#newcode1649
src/ia32/fast-codegen-ia32.cc:1649: __ push(eax);
__ push(result_register())

http://codereview.chromium.org/492002/diff/24/29#newcode1657
src/ia32/fast-codegen-ia32.cc:1657: __ mov(edx, Operand(esp, 0));
Is this really better than:

__ pop(edx);
__ sar(edx, 1);
__ add(Operand(edx), Immediate(masm_->CodeObject()));
__ jmp(Operand(edx));

[Lasse Reichstein, 2009-12-16 08:43:30]

http://codereview.chromium.org/492002/diff/24/27
File src/fast-codegen.cc (right):

http://codereview.chromium.org/492002/diff/24/27#newcode499
src/fast-codegen.cc:499: // the fially block through finally_entry label.
Fixed

http://codereview.chromium.org/492002/diff/24/27#newcode504
src/fast-codegen.cc:504: // and a value in the result register (rax/eax/r0),
both of which must
Done

http://codereview.chromium.org/492002/diff/24/27#newcode525
src/fast-codegen.cc:525: Visit(stmt->finally_block());
It should be ok (you can't label the blocks, so there is no reason to visit the
blocks themselves). Will change.

http://codereview.chromium.org/492002/diff/24/27#newcode526
src/fast-codegen.cc:526: ReturnFromFinallyBlock();  // Performs a return to the
calling code.
Fixed.
(The point was that it actually did a return at the end, so it was named to show
that control didn't fall through. I'll just use a comment).

http://codereview.chromium.org/492002/diff/24/27#newcode532
src/fast-codegen.cc:532: __ PushTryHandler(IN_JAVASCRIPT, TRY_FINALLY_HANDLER);
Done

http://codereview.chromium.org/492002/diff/24/29
File src/ia32/fast-codegen-ia32.cc (right):

http://codereview.chromium.org/492002/diff/24/29#newcode1642
src/ia32/fast-codegen-ia32.cc:1642: __ mov(edx, Operand(esp, 0));
Good idea. Done.

http://codereview.chromium.org/492002/diff/24/29#newcode1649
src/ia32/fast-codegen-ia32.cc:1649: __ push(eax);
Indeed. Half an abstractions is ... useless.

http://codereview.chromium.org/492002/diff/24/29#newcode1657
src/ia32/fast-codegen-ia32.cc:1657: __ mov(edx, Operand(esp, 0));
The CPU has a call/return stack that expects calls/returns to be properly
nested. If they aren't, the stack gets out of sync, and return becomes slower.

I don't know if messing with stack values break it anyway (I'm guess it won't,
which is why I cook and uncook without popping, so that the next expected return
value is never outside the stack).

We definitely get out of sync with the try-handler chain management, so maybe
it's not worth keeping it here.
I'll keep it for now, and try to do a little research on whether we break it,
and how much it matters.