API call code refactoring (ia32).

API call code refactoring (ia32).


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

[antonm, 2010-11-16 14:32:26]

LGTM

http://codereview.chromium.org/5055004/diff/1/src/ia32/macro-assembler-ia32.cc
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/5055004/diff/1/src/ia32/macro-assembler-ia32.c...
src/ia32/macro-assembler-ia32.cc:429: // Restore current context from top and
clear it in debug mode.
I'd suggest (but not insisting) to factor out common exit frame management. 
Should be simple if push(ecx) is moved out of common code.

http://codereview.chromium.org/5055004/diff/1/src/ia32/macro-assembler-ia32.h
File src/ia32/macro-assembler-ia32.h (right):

http://codereview.chromium.org/5055004/diff/1/src/ia32/macro-assembler-ia32.h...
src/ia32/macro-assembler-ia32.h:511: // Returning removes stack_space *
kPointerSize (GCed).
I'd use 'on return' instead of 'returning'

http://codereview.chromium.org/5055004/diff/1/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/5055004/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:502: __ PrepareCallApiFunction(kApiArgc +
kApiStackSpace, ebx);
nice that the bug was spotted.  but I am slightly uneasy that our tests didn't
catch it.  can you add a test that demonstrates the presence of the bug?

http://codereview.chromium.org/5055004/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:2173: // before call any runtime function.
before call <to> any runtime

[SeRya, 2010-11-16 14:53:28]

http://codereview.chromium.org/5055004/diff/1/src/ia32/macro-assembler-ia32.cc
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/5055004/diff/1/src/ia32/macro-assembler-ia32.c...
src/ia32/macro-assembler-ia32.cc:429: // Restore current context from top and
clear it in debug mode.
On 2010/11/16 14:32:26, antonm wrote:
> I'd suggest (but not insisting) to factor out common exit frame management. 
> Should be simple if push(ecx) is moved out of common code.

Done.

http://codereview.chromium.org/5055004/diff/1/src/ia32/macro-assembler-ia32.h
File src/ia32/macro-assembler-ia32.h (right):

http://codereview.chromium.org/5055004/diff/1/src/ia32/macro-assembler-ia32.h...
src/ia32/macro-assembler-ia32.h:511: // Returning removes stack_space *
kPointerSize (GCed).
On 2010/11/16 14:32:26, antonm wrote:
> I'd use 'on return' instead of 'returning'

Done.

http://codereview.chromium.org/5055004/diff/1/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/5055004/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:502: __ PrepareCallApiFunction(kApiArgc +
kApiStackSpace, ebx);
On 2010/11/16 14:32:26, antonm wrote:
> nice that the bug was spotted.  but I am slightly uneasy that our tests didn't
> catch it.  can you add a test that demonstrates the presence of the bug?

I don't see any bug. This line changed not to fix a bug but because signature of
PrepareCallApiFunction had changed. It doesn't receive number of JS references
anymore since now that references are removed in TryCallApiFunctionAndReturn.

http://codereview.chromium.org/5055004/diff/1/src/ia32/stub-cache-ia32.cc#new...
src/ia32/stub-cache-ia32.cc:2173: // before call any runtime function.
On 2010/11/16 14:32:26, antonm wrote:
> before call <to> any runtime

Done.