Great work, Zaheer. I think we're getting really close.
http://codereview.chromium.org/6170001/diff/44002/src/arm/frames-arm.cc File src/arm/frames-arm.cc (right): http://codereview.chromium.org/6170001/diff/44002/src/arm/frames-arm.cc#newcode46 src/arm/frames-arm.cc:46: if (marker == reinterpret_cast<void*>(ExitApiFrameConstants::kMarker)) { nits: a) unnecessary space after == b) reinterpret_cast<Address> http://codereview.chromium.org/6170001/diff/90001/src/arm/code-stubs-arm.cc File src/arm/code-stubs-arm.cc (right): http://codereview.chromium.org/6170001/diff/90001/src/arm/code-stubs-arm.cc#newcode2710 src/arm/code-stubs-arm.cc:2710: // popping the args. I think you should add a reference to EnterExitFrame, something like "EnterExitFrame below will store ip as sp on exit"---it was obvious when it was in the same macro, but not now http://codereview.chromium.org/6170001/diff/90001/src/arm/code-stubs-arm.h File src/arm/code-stubs-arm.h (right): http://codereview.chromium.org/6170001/diff/90001/src/arm/code-stubs-arm.h#newcode459 src/arm/code-stubs-arm.h:459: // Stub used when returning from direct native call to generated code. English is not native to me, so feel free to ignore. May you rework the comment? It's not obvious what's the difference between the code stub and the stub is. Maybe something along these lines: "Trampoline stub to call into native code.\n\nTo call safely into native code in the presence of compacting GC (which can move code objects) we need to keep the code which called into native pinned in the memory. Currently the simplest approach is to generate such stub early enough so it can never be moved by GC". My wording is far from perfect, just a stub :) http://codereview.chromium.org/6170001/diff/90001/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/90001/src/arm/macro-assembler-arm.cc#newcode547 src/arm/macro-assembler-arm.cc:547: int pending_pushes = stack_space + 4; // 4 pushes in this function. May you explain: it looks like only pending_pushes % 2 are used in this function which looks odd and this + 4 addition looks strange as well. What do I miss? BTW, do we have tests for API function invocations with many (> 3) arguments? http://codereview.chromium.org/6170001/diff/90001/src/arm/macro-assembler-arm.h File src/arm/macro-assembler-arm.h (right): http://codereview.chromium.org/6170001/diff/90001/src/arm/macro-assembler-arm.h#newcode259 src/arm/macro-assembler-arm.h:259: // |stack_space| pending pushes, used for alignment before call to C. Erik, it was my request. I saw this style in various parts of v8 and assumed it's the default. Please, tell us your ultimate word on it. Zaheer, sorry for luring you into this. On 2011/01/21 14:28:40, Erik Corry wrote:
We haven't used this |parameter_name| style elsewhere as far as I can
see.
Also, this comment has not been updated to reflect that the ip has sp
that
should be restored. Also the comment is out of date because it
mentions r6 as
being set up by the EnterExitFrame, whereas in fact it needs to be set
up by the
caller.
http://codereview.chromium.org/6170001/diff/90001/src/arm/simulator-arm.cc File src/arm/simulator-arm.cc (right): http://codereview.chromium.org/6170001/diff/90001/src/arm/simulator-arm.cc#newcode1543 src/arm/simulator-arm.cc:1543: // This signature supports direct call in to js function native callback I'd use API function instead of js function here http://codereview.chromium.org/6170001/diff/90001/src/arm/simulator-arm.cc#newcode1590 src/arm/simulator-arm.cc:1590: SimulatorRuntimeApiCall target = please, consider refactoring the common logic of logging and stack alignment checks http://codereview.chromium.org/6170001/diff/90001/src/arm/stub-cache-arm.cc File src/arm/stub-cache-arm.cc (right): http://codereview.chromium.org/6170001/diff/90001/src/arm/stub-cache-arm.cc#newcode622 src/arm/stub-cache-arm.cc:622: __ stm(ib, sp, r5.bit() | r6.bit()); Please, add a comment http://codereview.chromium.org/6170001/diff/90001/src/arm/stub-cache-arm.cc#newcode624 src/arm/stub-cache-arm.cc:624: // r2 points to calldata as expected by Arguments class (refer layout above) nit: either call_data or call data, please http://codereview.chromium.org/6170001/diff/90001/src/heap.h File src/heap.h (right): http://codereview.chromium.org/6170001/diff/90001/src/heap.h#newcode121 src/heap.h:121: #if V8_TARGET_ARCH_ARM Maybe we should restructure that differently: I'd prefer to have a single STRONG_ROOT_LIST_ARM macro which could be empty, or keep DirectCEntryStub, or both. The code could look like #if V8_TARGET_ARCH_ARM && !V8_INTERPRETED_REGEXP ... #elif V8_TARGET_ARCH_ARM #else #endif What do you think? http://codereview.chromium.org/6170001/diff/90001/src/top.h File src/top.h (right): http://codereview.chromium.org/6170001/diff/90001/src/top.h#newcode36 src/top.h:36: #ifdef USE_SIMULATOR why this change? http://codereview.chromium.org/6170001/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
