Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6170001:
Direct call api functions (arm implementation) (Closed)
|
Created:
9 years, 11 months ago by Zaheer Modified:
9 years, 7 months ago CC:
v8-dev Visibility:
Public. |
DescriptionDirect call api functions (arm implementation)
Patch Set 1 #
Total comments: 39
Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 2
Patch Set 4 : '' #
Total comments: 1
Patch Set 5 : '' #
Total comments: 20
Patch Set 6 : '' #Patch Set 7 : '' #Patch Set 8 : '' #Patch Set 9 : '' #
Total comments: 2
Patch Set 10 : '' #
Total comments: 8
Patch Set 11 : '' #Patch Set 12 : '' #
Total comments: 6
Patch Set 13 : '' #
Total comments: 30
Patch Set 14 : '' #
Total comments: 8
Patch Set 15 : '' #Patch Set 16 : '' #Patch Set 17 : '' #
Total comments: 14
Patch Set 18 : '' #
Total comments: 23
Patch Set 19 : '' #
Total comments: 2
Messages
Total messages: 38 (0 generated)
Zaheer, that's almost lgtm, but I have a couple of questions. Plus I would really appreciate if frame handling can be refactored to be as close as possible to ia32 implementation. Thanks a lot for porting this, it's most appreciated. http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1414: mov(scratch, Operand(unwind_space)); it looks like a lot of code below is shared with MacroAssembler::EnterExitFrame. Is it possible to refactor the common code and make it more like ia32 implementation (which just invokes EnterApiExitFrame + some magic for some platforms? http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1461: // patch the exit frame pc and Call the api function! nit: [P]atch and [c]all. http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1461: // patch the exit frame pc and Call the api function! once again, can we factor out frame handling? http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1463: str(ip, MemOperand(fp, -2 * kPointerSize)); do not we have a named constant for -2? http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1494: ldr(r4, MemOperand(ip)); Shouldn't it be LoadRoot(r4, Heap::kTheHoleValueRootIndex)? http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1499: LeaveExitFrame(0); LeaveExitFrame apparently takes bool, not int http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.h File src/arm/macro-assembler-arm.h (right): http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.h#n... src/arm/macro-assembler-arm.h:644: // creates the Exit frame, pushes the arguments and aligns the stack nit: [C]reates. And you probably do not need [E] in exit frame. Overall, why doc is that different from ia32 version? And, please, provide documentation for arg_stack_space and unwind_space parameters. http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc File src/arm/stub-cache-arm.cc (right): http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc#newco... src/arm/stub-cache-arm.cc:645: const CallOptimization& optimization, nit: indentation is slightly off http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc#newco... src/arm/stub-cache-arm.cc:647: Failure** failure) { may you add a stack map like in ia32 version? http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc#newco... src/arm/stub-cache-arm.cc:662: __ stm(ib, sp, r5.bit() | r6.bit()); please, add a comment what are you doing here. http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc#newco... src/arm/stub-cache-arm.cc:674: __ str(r2, MemOperand(sp)); I don't know if it'd be faster, but maybe you can use memoperand that post increment pointer to arrange this sequence of stores, something like: __ mov(r4, sp); __ str(r2, MemOperand(r4, kPointerSize, PostIndex); __ add(ip, r2, ...); __ str(ip, MemOperand(r4, kPointerSize, PostIndex); // etc. http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc#newco... src/arm/stub-cache-arm.cc:679: // v8::Arguments::length_ = argc for me comments are one line below they should be: I'd prefer to have // v8::Arguments::length_ = argc followed by mov(ip, Operand(argc)) and str(ip, ...) http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc#newco... src/arm/stub-cache-arm.cc:2319: #ifdef USE_SIMULATOR do we need a special case for simulator? http://codereview.chromium.org/6170001/diff/1/src/v8utils.h File src/v8utils.h (right): http://codereview.chromium.org/6170001/diff/1/src/v8utils.h#newcode31 src/v8utils.h:31: #include <stdarg.h> why this new include?
Thanks Anton for the review! I have answered major comments and expecting feedback. i will submit the updated patch with the fixed comments asap. http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1414: mov(scratch, Operand(unwind_space)); On 2011/01/11 14:11:33, antonm wrote: > it looks like a lot of code below is shared with MacroAssembler::EnterExitFrame. > Is it possible to refactor the common code and make it more like ia32 > implementation (which just invokes EnterApiExitFrame + some magic for some > platforms? The behavior is different from EnterExitFrame - v8::arguments has to be allocated above exit frame to miss gc - The alignment has to happen after allocating space for the args and it should handle aligned/unaligned case based on arg_stack_space (unlike EnterExitFrame which knows the number of pushes - 5) - exit frame pc patching has to be handled - argv and builtin function is n/a in this case http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1461: // patch the exit frame pc and Call the api function! On 2011/01/11 14:11:33, antonm wrote: > nit: [P]atch and [c]all. Done http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1461: // patch the exit frame pc and Call the api function! On 2011/01/11 14:11:33, antonm wrote: > once again, can we factor out frame handling? same issues as mentioned before for refactoring http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1463: str(ip, MemOperand(fp, -2 * kPointerSize)); On 2011/01/11 14:11:33, antonm wrote: > do not we have a named constant for -2? check ExitFrame::FillState(..) it seems to be hard coded (pc = sp - 1 = fp -2). This value is already used for kMarkerOffset so i wasnt sure to introduce a new constant. http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1494: ldr(r4, MemOperand(ip)); On 2011/01/11 14:11:33, antonm wrote: > Shouldn't it be LoadRoot(r4, Heap::kTheHoleValueRootIndex)? Done http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1499: LeaveExitFrame(0); On 2011/01/11 14:11:33, antonm wrote: > LeaveExitFrame apparently takes bool, not int done http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.h File src/arm/macro-assembler-arm.h (right): http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.h#n... src/arm/macro-assembler-arm.h:644: // creates the Exit frame, pushes the arguments and aligns the stack On 2011/01/11 14:11:33, antonm wrote: > nit: [C]reates. And you probably do not need [E] in exit frame. > > Overall, why doc is that different from ia32 version? And, please, provide > documentation for arg_stack_space and unwind_space parameters. Done http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc File src/arm/stub-cache-arm.cc (right): http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc#newco... src/arm/stub-cache-arm.cc:645: const CallOptimization& optimization, On 2011/01/11 14:11:33, antonm wrote: > nit: indentation is slightly off Done http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc#newco... src/arm/stub-cache-arm.cc:647: Failure** failure) { On 2011/01/11 14:11:33, antonm wrote: > may you add a stack map like in ia32 version? Done http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc#newco... src/arm/stub-cache-arm.cc:674: __ str(r2, MemOperand(sp)); On 2011/01/11 14:11:33, antonm wrote: > I don't know if it'd be faster, but maybe you can use memoperand that post > increment pointer to arrange this sequence of stores, something like: > > __ mov(r4, sp); > __ str(r2, MemOperand(r4, kPointerSize, PostIndex); > __ add(ip, r2, ...); > __ str(ip, MemOperand(r4, kPointerSize, PostIndex); > // etc. will check http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc#newco... src/arm/stub-cache-arm.cc:679: // v8::Arguments::length_ = argc On 2011/01/11 14:11:33, antonm wrote: > for me comments are one line below they should be: I'd prefer to have // > v8::Arguments::length_ = argc followed by mov(ip, Operand(argc)) and str(ip, > ...) Mistake! fixed http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc#newco... src/arm/stub-cache-arm.cc:2319: #ifdef USE_SIMULATOR On 2011/01/11 14:11:33, antonm wrote: > do we need a special case for simulator? currently the simulator interface to the native api expects a argc, argv and direct call breaks that (it has a v8::Arguments&). I wasnt sure how to fix this in the simulator, but it would be great if we can do it.
http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1415: add(ip, sp, Operand(scratch, LSL, kPointerSizeLog2)); add(ip, sp, Operand(unwind_space * kPointerSize)); ? http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1432: push(scratch, nz); ASSERT(frame_alignment == 2 * kPointerSize); http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1435: mov(ip, Operand(ExternalReference(Top::k_c_entry_fp_address))); ia32 code allocates C arguments below c_entry_fp_address. It allows to not care that C arguments could be reached by GC. I think this semantic should be preserved here as well. By the way, you put argc into the argumets stack space what could be misinterpreted as an object reference (if argc is odd) and crash GC if it happens in the called function. Also stack alignment placeholder don't need to be initialized if it's below c_entry_fp_address. http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1452: mov(ip, Operand(next_address)); x64 implementation use offsets to eliminate 2 of 3 ldr instructions: static int Offset(ExternalReference ref0, ExternalReference ref1) { int64_t offset = (ref0.address() - ref1.address()); // Check that fits into int. ASSERT(static_cast<int>(offset) == offset); return static_cast<int>(offset); } ExternalReference next_address = ExternalReference::handle_scope_next_address(); const int kNextOffset = 0; const int kLimitOffset = Offset( ExternalReference::handle_scope_limit_address(), next_address); const int kLevelOffset = Offset( ExternalReference::handle_scope_level_address(), next_address); May be it would work here as well? http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1476: ldr(r0, MemOperand(r0)); LoadRoot(r0, Heap::kUndefinedValueRootIndex, eq); ldr(r0, MemOperand(r0), ne); How about it?
And Zaheed, you didn't upload new version of the patch. http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1414: mov(scratch, Operand(unwind_space)); Zaheer, I am not sure it's something which cannot be solved. I do agree with Sergey and think we should allocate v8::Arguments below exit frame (exactly like we do for ia32)---one of the reasons I asked you to refactor the code was to make these things immediately visible. We deal with alignment on x64 as well and it looks fine (in the worst case you would need the same and_ trick, see PrepareCallCFunction). And so on. On 2011/01/11 15:44:35, zaheer wrote: > On 2011/01/11 14:11:33, antonm wrote: > > it looks like a lot of code below is shared with > MacroAssembler::EnterExitFrame. > > Is it possible to refactor the common code and make it more like ia32 > > implementation (which just invokes EnterApiExitFrame + some magic for some > > platforms? > The behavior is different from EnterExitFrame > - v8::arguments has to be allocated above exit frame to miss gc > - The alignment has to happen after allocating space for the args and it should > handle aligned/unaligned case based on arg_stack_space (unlike EnterExitFrame > which knows the number of pushes - 5) > - exit frame pc patching has to be handled > - argv and builtin function is n/a in this case http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc File src/arm/stub-cache-arm.cc (right): http://codereview.chromium.org/6170001/diff/1/src/arm/stub-cache-arm.cc#newco... src/arm/stub-cache-arm.cc:2319: #ifdef USE_SIMULATOR On 2011/01/11 15:44:35, zaheer wrote: > On 2011/01/11 14:11:33, antonm wrote: > > do we need a special case for simulator? > currently the simulator interface to the native api expects a argc, argv and > direct call breaks that (it has a v8::Arguments&). I wasnt sure how to fix this > in the simulator, but it would be great if we can do it. I see, let's ask Erik, maybe he can suggest something. I am concerned with the testability of this change---I think most of the people use simulator to run tests so we can miss errors.
Thanks Sergey, Anton for your comments. Could you please review the updated patch http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1414: mov(scratch, Operand(unwind_space)); On 2011/01/11 19:27:40, antonm wrote: > Zaheer, > > I am not sure it's something which cannot be solved. > > I do agree with Sergey and think we should allocate v8::Arguments below exit > frame (exactly like we do for ia32)---one of the reasons I asked you to refactor > the code was to make these things immediately visible. split the code as EnterApiExitFramePrologue/Epilogue and yes, the arguments is below the exit frame exit frame arguments <stack grows here> > > We deal with alignment on x64 as well and it looks fine (in the worst case you > would need the same and_ trick, see PrepareCallCFunction). I can reuse PrepareCallCFunction (instead of EnterExitFrameEpilogue) but it seems to be creating one extra space and storing restore sp which is not required (i.e. two instructions redundant) > > And so on. > > On 2011/01/11 15:44:35, zaheer wrote: > > On 2011/01/11 14:11:33, antonm wrote: > > > it looks like a lot of code below is shared with > > MacroAssembler::EnterExitFrame. > > > Is it possible to refactor the common code and make it more like ia32 > > > implementation (which just invokes EnterApiExitFrame + some magic for some > > > platforms? > > The behavior is different from EnterExitFrame > > - v8::arguments has to be allocated above exit frame to miss gc > > - The alignment has to happen after allocating space for the args and it > should > > handle aligned/unaligned case based on arg_stack_space (unlike EnterExitFrame > > which knows the number of pushes - 5) > > - exit frame pc patching has to be handled > > - argv and builtin function is n/a in this case > http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1415: add(ip, sp, Operand(scratch, LSL, kPointerSizeLog2)); On 2011/01/11 16:34:50, SeRya wrote: > add(ip, sp, Operand(unwind_space * kPointerSize)); ? Done http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1432: push(scratch, nz); On 2011/01/11 16:34:50, SeRya wrote: > ASSERT(frame_alignment == 2 * kPointerSize); Done http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1435: mov(ip, Operand(ExternalReference(Top::k_c_entry_fp_address))); On 2011/01/11 16:34:50, SeRya wrote: > ia32 code allocates C arguments below c_entry_fp_address. It allows to not care > that C arguments could be reached by GC. I think this semantic should be > preserved here as well. > i do allocate the args on top of the exit frame similar to ia32. Do i miss something [c args] [Exit Frame] <- c_entry_fp [fast api args] [js args] Maybe the ordering of code is confusing? i have split it in to EnterApiExitFramePrologue/Epilogue > By the way, you put argc into the argumets stack space what could be > misinterpreted as an object reference (if argc is odd) and crash GC if it > happens in the called function. I did verify in Filecycler that there is no issue with gc invoked from called function. > > Also stack alignment placeholder don't need to be initialized if it's below > c_entry_fp_address. removed scratch argument. pushed ip (assumed dummy) http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1452: mov(ip, Operand(next_address)); On 2011/01/11 16:34:50, SeRya wrote: > x64 implementation use offsets to eliminate 2 of 3 ldr instructions: > > static int Offset(ExternalReference ref0, ExternalReference ref1) { > int64_t offset = (ref0.address() - ref1.address()); > // Check that fits into int. > ASSERT(static_cast<int>(offset) == offset); > return static_cast<int>(offset); > } > > ExternalReference next_address = > ExternalReference::handle_scope_next_address(); > const int kNextOffset = 0; > const int kLimitOffset = Offset( > ExternalReference::handle_scope_limit_address(), > next_address); > const int kLevelOffset = Offset( > ExternalReference::handle_scope_level_address(), > next_address); > > May be it would work here as well? Done! but i could remove only the mov not ldr instructions. Also i have fixed r7 for next address removing two mov's. http://codereview.chromium.org/6170001/diff/1/src/arm/macro-assembler-arm.cc#... src/arm/macro-assembler-arm.cc:1476: ldr(r0, MemOperand(r0)); On 2011/01/11 16:34:50, SeRya wrote: > LoadRoot(r0, Heap::kUndefinedValueRootIndex, eq); > ldr(r0, MemOperand(r0), ne); > > How about it? Done! Thanks for the pointer, it simplifies the code very much.
Looks good to me http://codereview.chromium.org/6170001/diff/22001/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/22001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1455: const int kNextOffset = 0; Apparently it's not used. Remove it or reference in MemOperand(r7 + kNextOffset) (the last one looks more consistent for me). http://codereview.chromium.org/6170001/diff/22001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1488: ldr(r6, MemOperand(r7, kLevelOffset)); Apparently r6 still contains valid level at this point. If so we could only check int for debugging purposes: if (FLAG_debug_code) { ldr(r1, MemOperand(r7, kLevelOffset)); cmp(r1, r6); Check(eq, ...); }
Thanks for the review. This patch uses the old path for the simulator which iam hoping there is a solution (waiting for Erik's comments). On 2011/01/12 14:43:40, SeRya wrote: > Looks good to me > > http://codereview.chromium.org/6170001/diff/22001/src/arm/macro-assembler-arm.cc > File src/arm/macro-assembler-arm.cc (right): > > http://codereview.chromium.org/6170001/diff/22001/src/arm/macro-assembler-arm... > src/arm/macro-assembler-arm.cc:1455: const int kNextOffset = 0; > Apparently it's not used. Remove it or reference in MemOperand(r7 + kNextOffset) > (the last one looks more consistent for me). Done, the latter option. > > http://codereview.chromium.org/6170001/diff/22001/src/arm/macro-assembler-arm... > src/arm/macro-assembler-arm.cc:1488: ldr(r6, MemOperand(r7, kLevelOffset)); > Apparently r6 still contains valid level at this point. > If so we could only check int for debugging purposes: > > if (FLAG_debug_code) { > ldr(r1, MemOperand(r7, kLevelOffset)); > cmp(r1, r6); > Check(eq, ...); > } Done
http://codereview.chromium.org/6170001/diff/26001/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/26001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1493: ldr(r6, MemOperand(r7, kLevelOffset)); You've just checked that r6 does contain the same value as MemOperand(r7, kLevelOffset). There is no sense to preserve this instruction.
On 2011/01/13 09:36:21, SeRya wrote: > http://codereview.chromium.org/6170001/diff/26001/src/arm/macro-assembler-arm.cc > File src/arm/macro-assembler-arm.cc (right): > > http://codereview.chromium.org/6170001/diff/26001/src/arm/macro-assembler-arm... > src/arm/macro-assembler-arm.cc:1493: ldr(r6, MemOperand(r7, kLevelOffset)); > You've just checked that r6 does contain the same value as MemOperand(r7, > kLevelOffset). There is no sense to preserve this instruction. Yep, sorry. Fixed
Next round of comments. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1410: void MacroAssembler::EnterExitFramePrologue(int unwind_space) { Thanks a lot for introducing this macros. But you still keep the code duplicated (see ::EnterExitFrame above) http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1412: stm(db_w, sp, fp.bit() | ip.bit() | lr.bit()); Please, retain the original comment which describes the semantics of this push http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1426: // Create space for the args nit: period at the end of the comment, please http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1433: tst(sp, Operand(frame_alignment_mask)); please, keep the comment http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1471: // patch the exit frame pc and Call the api function! nit: [P]atch and [c]all http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1481: // otherwise set it to undefined nit: period at the end of the comment. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1482: LoadRoot(r0, Heap::kUndefinedValueRootIndex, eq); it looks like you forgot cmp(r0, Operand(0)) http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1501: mov(ip, Operand(ExternalReference::the_hole_value_location())); please, use LoadRoot. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.h File src/arm/macro-assembler-arm.h (right): http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.h:646: // arg_stack_space - space allocated for the structure (e.g. nit: we use |<parameter name>| convention. Ditto for unwind_space below http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.h:646: // arg_stack_space - space allocated for the structure (e.g. maybe some other name? allocation_size or something like that?
Thanks Anton for furthur review. updated patch uploaded. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1410: void MacroAssembler::EnterExitFramePrologue(int unwind_space) { On 2011/01/13 12:47:08, antonm wrote: > Thanks a lot for introducing this macros. But you still keep the code > duplicated (see ::EnterExitFrame above) EnterExitFrame satisfies the builtin (argc/argv) interface, registers are hard coded and does alignment. in our case, the interface is different and alignment needs to happen after pushing the additional space for v8::arguments and the alignment needs to be generic (as i intend to reuse this for load callback as well). given this i wasnt sure how to reuse this. let me know if you have some idea. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1412: stm(db_w, sp, fp.bit() | ip.bit() | lr.bit()); On 2011/01/13 12:47:08, antonm wrote: > Please, retain the original comment which describes the semantics of this push Done. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1426: // Create space for the args On 2011/01/13 12:47:08, antonm wrote: > nit: period at the end of the comment, please Done. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1433: tst(sp, Operand(frame_alignment_mask)); On 2011/01/13 12:47:08, antonm wrote: > please, keep the comment Done. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1471: // patch the exit frame pc and Call the api function! On 2011/01/13 12:47:08, antonm wrote: > nit: [P]atch and [c]all Done. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1481: // otherwise set it to undefined On 2011/01/13 12:47:08, antonm wrote: > nit: period at the end of the comment. Done. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1482: LoadRoot(r0, Heap::kUndefinedValueRootIndex, eq); On 2011/01/13 12:47:08, antonm wrote: > it looks like you forgot cmp(r0, Operand(0)) sorry, i realized that in my tests. fixed. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1501: mov(ip, Operand(ExternalReference::the_hole_value_location())); On 2011/01/13 12:47:08, antonm wrote: > please, use LoadRoot. Done. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm.h File src/arm/macro-assembler-arm.h (right): http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.h:646: // arg_stack_space - space allocated for the structure (e.g. On 2011/01/13 12:47:08, antonm wrote: > nit: we use |<parameter name>| convention. Ditto for unwind_space below Done. http://codereview.chromium.org/6170001/diff/33001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.h:646: // arg_stack_space - space allocated for the structure (e.g. On 2011/01/13 12:47:08, antonm wrote: > maybe some other name? allocation_size or something like that? hmm arg_stack_size is taken from x64. do you insist on changing it
Please review new patch set. This fixes the case when the stub moves during gc from called native function I create an additional slot for the stack in the exit frame and point it to the stack address just before the call to native function. The lr to be patched (pushed by the called native function) will be at sp-1. I use the marker to differentiate this case from existing scenarios (CEntryStub) so existing Exit Frames are not affected.
http://codereview.chromium.org/6170001/diff/44002/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/44002/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1478: Call(function->address(), RelocInfo::RUNTIME_ENTRY); This will be translated into mov(lr, Operand(pc), LeaveCC, cond); mov(pc, Operand(target), LeaveCC, cond); (or equivalent with blx). The register is unreachable for GC which needs to fix the return address if the code has been moved, isn't it? Apparently the only way to survive code relocation is to make the lr register to point to an unmovable piece of code.
Thanks serya for comments. I checked this patch with a moving stub test and it seems to work. details below. On 2011/01/18 19:01:57, SeRya wrote: > http://codereview.chromium.org/6170001/diff/44002/src/arm/macro-assembler-arm.cc > File src/arm/macro-assembler-arm.cc (right): > > http://codereview.chromium.org/6170001/diff/44002/src/arm/macro-assembler-arm... > src/arm/macro-assembler-arm.cc:1478: Call(function->address(), > RelocInfo::RUNTIME_ENTRY); > This will be translated into > mov(lr, Operand(pc), LeaveCC, cond); > mov(pc, Operand(target), LeaveCC, cond); > (or equivalent with blx). > > The register is unreachable for GC which needs to fix the return address if the > code has been moved, isn't it? The first instruction in the native callback pushes the lr in the first stack slot. I point the exit frame pc to it and it gets patched. callback is native callback (gdb) bt #0 callback (args=@0xbec33bb8) at external/v8/v8_latest/directcall.cc:59 #1 0x4087d0b0 in ?? () The first instruction pushes lr which is pointed to by exit frame pc Dump of assembler code for function _Z8callbackRKN2v89ArgumentsE: 0x0001d9ac <_Z8callbackRKN2v89ArgumentsE+0>: push {r4, r5, r6, lr} (gdb) x/4x $sp 0xbec33ba8: 0x001ac80c 0x001ad6b8 0x00000003 0x4087d0b0 (lr) GC moves the stub forward 0x4087cfc0 -> 0x4087cc40. lr is patched (gdb) x/4x 0xbec33ba8 0xbec33ba8: 0x001ac80c 0x001ad6b8 0x00000003 0x4087cd30 (patched) (gdb) bt #0 0x0001d9e0 in callback (args=<value optimized out>) at external/v8/v8_latest/directcall.cc:68 #1 0x4087cd30 in ?? () --> modified return address native function uses modified lr to return 0x0001d9e0 <_Z8callbackRKN2v89ArgumentsE+52>: pop {r4, r5, r6, pc} > > Apparently the only way to survive code relocation is to make the lr register to > point to an unmovable piece of code. in this case how do we return to a moving stub?
On 2011/01/19 06:19:44, zaheer wrote: > Thanks serya for comments. I checked this patch with a moving stub test and it > seems to work. details below. > > On 2011/01/18 19:01:57, SeRya wrote: > > > http://codereview.chromium.org/6170001/diff/44002/src/arm/macro-assembler-arm.cc > > File src/arm/macro-assembler-arm.cc (right): > > > > > http://codereview.chromium.org/6170001/diff/44002/src/arm/macro-assembler-arm... > > src/arm/macro-assembler-arm.cc:1478: Call(function->address(), > > RelocInfo::RUNTIME_ENTRY); > > This will be translated into > > mov(lr, Operand(pc), LeaveCC, cond); > > mov(pc, Operand(target), LeaveCC, cond); > > (or equivalent with blx). > > > > The register is unreachable for GC which needs to fix the return address if > the > > code has been moved, isn't it? > The first instruction in the native callback pushes the lr in the first stack > slot. I point the exit frame pc to it and it gets patched. > > callback is native callback > (gdb) bt > #0 callback (args=@0xbec33bb8) at external/v8/v8_latest/directcall.cc:59 > #1 0x4087d0b0 in ?? () > > The first instruction pushes lr which is pointed to by exit frame pc > Dump of assembler code for function _Z8callbackRKN2v89ArgumentsE: > 0x0001d9ac <_Z8callbackRKN2v89ArgumentsE+0>: push {r4, r5, r6, lr} > > (gdb) x/4x $sp > 0xbec33ba8: 0x001ac80c 0x001ad6b8 0x00000003 0x4087d0b0 (lr) > > GC moves the stub > forward 0x4087cfc0 -> 0x4087cc40. > > lr is patched > (gdb) x/4x 0xbec33ba8 > 0xbec33ba8: 0x001ac80c 0x001ad6b8 0x00000003 0x4087cd30 (patched) > > (gdb) bt > #0 0x0001d9e0 in callback (args=<value optimized out>) at > external/v8/v8_latest/directcall.cc:68 > #1 0x4087cd30 in ?? () --> modified return address > > native function uses modified lr to return > 0x0001d9e0 <_Z8callbackRKN2v89ArgumentsE+52>: pop {r4, r5, r6, pc} I can't find a place in ARM ABI docs what would require this behavior. Is this a compiler specific stuff? If so it's not a good idea to rely on it. > > Apparently the only way to survive code relocation is to make the lr register > to > > point to an unmovable piece of code. > in this case how do we return to a moving stub? I actually see 2 ways to deal with it (it only make sense if the stuff above is really compiler specific): a. 1. Push return address on stack (it will make it accessible to GC). 2. Set up the lr register pointing to the "pop pc" instruction in unmovable code. b. 1. Make the rest of code above Call(function->address(), RelocInfo::RUNTIME_ENTRY); to be unmovable. Apparently it doesn't depend on args. 2. Put a pointer to that code to lr. 3. Jump to the C function. The second approach would share some piece of code and probably faster. But I'm afraid there is no good method to make a code unmovable for now. In other case there wouldble be such a comment in CEntryStub::Generate: // TODO(1242173): To let the GC traverse the return address of the exit // frames, we need to know where the return address is. Right now, // we push it on the stack to be able to find it again, but we never // restore from it in case of changes, which makes it impossible to // support moving the C entry code stub. This should be fixed, but currently // this is OK because the CEntryStub gets generated so early in the V8 boot // sequence that it is not moving ever.
On 2011/01/19 11:23:26, SeRya wrote: > On 2011/01/19 06:19:44, zaheer wrote: > > Thanks serya for comments. I checked this patch with a moving stub test and it > > seems to work. details below. > > > > On 2011/01/18 19:01:57, SeRya wrote: > > > > > > http://codereview.chromium.org/6170001/diff/44002/src/arm/macro-assembler-arm.cc > > > File src/arm/macro-assembler-arm.cc (right): > > > > > > > > > http://codereview.chromium.org/6170001/diff/44002/src/arm/macro-assembler-arm... > > > src/arm/macro-assembler-arm.cc:1478: Call(function->address(), > > > RelocInfo::RUNTIME_ENTRY); > > > This will be translated into > > > mov(lr, Operand(pc), LeaveCC, cond); > > > mov(pc, Operand(target), LeaveCC, cond); > > > (or equivalent with blx). > > > > > > The register is unreachable for GC which needs to fix the return address if > > the > > > code has been moved, isn't it? > > The first instruction in the native callback pushes the lr in the first stack > > slot. I point the exit frame pc to it and it gets patched. > > > > callback is native callback > > (gdb) bt > > #0 callback (args=@0xbec33bb8) at external/v8/v8_latest/directcall.cc:59 > > #1 0x4087d0b0 in ?? () > > > > The first instruction pushes lr which is pointed to by exit frame pc > > Dump of assembler code for function _Z8callbackRKN2v89ArgumentsE: > > 0x0001d9ac <_Z8callbackRKN2v89ArgumentsE+0>: push {r4, r5, r6, lr} > > > > (gdb) x/4x $sp > > 0xbec33ba8: 0x001ac80c 0x001ad6b8 0x00000003 0x4087d0b0 (lr) > > > > GC moves the stub > > forward 0x4087cfc0 -> 0x4087cc40. > > > > lr is patched > > (gdb) x/4x 0xbec33ba8 > > 0xbec33ba8: 0x001ac80c 0x001ad6b8 0x00000003 0x4087cd30 (patched) > > > > (gdb) bt > > #0 0x0001d9e0 in callback (args=<value optimized out>) at > > external/v8/v8_latest/directcall.cc:68 > > #1 0x4087cd30 in ?? () --> modified return address > > > > native function uses modified lr to return > > 0x0001d9e0 <_Z8callbackRKN2v89ArgumentsE+52>: pop {r4, r5, r6, pc} > > I can't find a place in ARM ABI docs what would require this behavior. Is this a > compiler specific stuff? If so it's not a good idea to rely on it. Yes, i agree the return should be caller controlled. > > > > Apparently the only way to survive code relocation is to make the lr > register > > to > > > point to an unmovable piece of code. > > in this case how do we return to a moving stub? > > I actually see 2 ways to deal with it (it only make sense if the stuff above is > really compiler specific): > > a. > 1. Push return address on stack (it will make it accessible to GC). > 2. Set up the lr register pointing to the "pop pc" instruction > in unmovable code. I guess this would need to create a new stub with single instruction similar to centry > > b. > 1. Make the rest of code above > Call(function->address(), RelocInfo::RUNTIME_ENTRY); > to be unmovable. > Apparently it doesn't depend on args. > 2. Put a pointer to that code to lr. > 3. Jump to the C function. since this is not feasible will try a) > > The second approach would share some piece of code and probably faster. But I'm > afraid there is no good method to make a code unmovable for now. In other case > there wouldble be such a comment in CEntryStub::Generate: > > // TODO(1242173): To let the GC traverse the return address of the exit > // frames, we need to know where the return address is. Right now, > // we push it on the stack to be able to find it again, but we never > // restore from it in case of changes, which makes it impossible to > // support moving the C entry code stub. This should be fixed, but currently > // this is OK because the CEntryStub gets generated so early in the V8 boot > // sequence that it is not moving ever. curious why stub created early doesnt move.
Please review the uploaded change. - use intermediate stub to support stub code movement (option a) from previous comments) - removed EnterExitFramePrologue/Epilogue as epilogue is thin now support for simulator will be a separate CL. Thanks.
http://codereview.chromium.org/6170001/diff/60001/src/arm/code-stubs-arm.h File src/arm/code-stubs-arm.h (right): http://codereview.chromium.org/6170001/diff/60001/src/arm/code-stubs-arm.h#ne... src/arm/code-stubs-arm.h:459: class DirectCEntryStub: public CodeStub { I think it worth to comment motivation this stub (and mention that its code assumed to be unmovable). http://codereview.chromium.org/6170001/diff/60001/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/60001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1464: int frame_alignment = ActivationFrameAlignment(); Currently CEntryStub stack layout is following: <caller stack frame> <stack alignment place holder initialized by 0?> <caller frame pointer> <caller stack pointer after unwinding> <return address> <code object> <return address to the stub or 0-marker> <fp state?> Your layout: <caller stack frame> <caller frame pointer> <caller stack pointer after unwinding> <1-marker> <pointer to the return address> <stack alignment uninitialized place holder?> <return address> <code object> <return address> You can just move the stack alignment place holder above, initialize it by 0, move the maker and pointer to the return address and your layout will be fully compatible with CEntryStub. You won't need a special case in ExitFrame::ComputeStackPointer and ExitApiFrameConstants. Actually I think you should MacroAssembler::EnterExitFrame. Just move the following lines out of the methods and it will do exactly what do you need here: add(ip, sp, Operand(r0, LSL, kPointerSizeLog2)); sub(r6, ip, Operand(kPointerSize)); ... // Setup argc and the builtin function in callee-saved registers. mov(r4, Operand(r0)); mov(r5, Operand(r1)); http://codereview.chromium.org/6170001/diff/60001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1480: str(sp, MemOperand(fp, ExitApiFrameConstants::kSPOffset)); It looks like this sequence is equal to the shorter one: if (frame_alignment > kPointerSize) { ASSERT(frame_alignment == 2 * kPointerSize); tst(sp, Operand(frame_alignment_mask)); // Stack alignment place holder need not be initialized as its below // c_entry_fp_address and does not affect GC. push(ip, eq); } // Store sp in the exit frame sp slot. sp - 1 points to return address // pushed before call str(sp, MemOperand(fp, ExitApiFrameConstants::kSPOffset)); (removed sub/add instructions and condition in the push instruction is changed to the opposite one).
Thanks serya for your comments. http://codereview.chromium.org/6170001/diff/60001/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/60001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1464: int frame_alignment = ActivationFrameAlignment(); On 2011/01/20 12:09:28, SeRya wrote: > Currently CEntryStub stack layout is following: > <caller stack frame> > <stack alignment place holder initialized by 0?> > <caller frame pointer> > <caller stack pointer after unwinding> > <return address> > <code object> > <return address to the stub or 0-marker> > <fp state?> > > Your layout: > <caller stack frame> > <caller frame pointer> > <caller stack pointer after unwinding> > <1-marker> > <pointer to the return address> > <stack alignment uninitialized place holder?> > <return address> > <code object> > <return address> Explaining the issue below, my layout in bit more detail <caller stack frame> <return address> <caller stack pointer after unwinding> <caller frame pointer> <code object> <-- centry sp, cant use the same in current case <marker> <exit frame sp> . . <arguments> . . <stack alignment> <return address> <native call stack> I cannot completely reuse the exit frame layout since arguments come in between exit frame and the native entry and hence the need to put additional slot (marker/stack) to point to return address. If you already considered the above issue, maybe i miss your point. > > You can just move the stack alignment place holder above, initialize it by 0, > move the maker and pointer to the return address and your layout will be fully > compatible with CEntryStub. I think i can reuse EnterExitFrame by moving the code you mentioned below and also making frame alignment code generic. You won't need a special case in > ExitFrame::ComputeStackPointer and ExitApiFrameConstants. From above comments iam not sure how to avoid this. > > Actually I think you should MacroAssembler::EnterExitFrame. Just move the > following lines out of the methods and it will do exactly what do you need here: > > add(ip, sp, Operand(r0, LSL, kPointerSizeLog2)); > sub(r6, ip, Operand(kPointerSize)); > ... > // Setup argc and the builtin function in callee-saved registers. > mov(r4, Operand(r0)); > mov(r5, Operand(r1)); you mean i should move these back to CEntryStub::Generate and introduce a parameter (say pending_push) which is used to do frame alignment. http://codereview.chromium.org/6170001/diff/60001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1480: str(sp, MemOperand(fp, ExitApiFrameConstants::kSPOffset)); On 2011/01/20 12:09:28, SeRya wrote: > It looks like this sequence is equal to the shorter one: > > if (frame_alignment > kPointerSize) { > ASSERT(frame_alignment == 2 * kPointerSize); > tst(sp, Operand(frame_alignment_mask)); > // Stack alignment place holder need not be initialized as its below > // c_entry_fp_address and does not affect GC. > push(ip, eq); > } > > // Store sp in the exit frame sp slot. sp - 1 points to return address > // pushed before call > str(sp, MemOperand(fp, ExitApiFrameConstants::kSPOffset)); > > (removed sub/add instructions and condition in the push instruction is changed > to the opposite one). Thanks for the catch.
http://codereview.chromium.org/6170001/diff/60001/src/arm/frames-arm.h File src/arm/frames-arm.h (right): http://codereview.chromium.org/6170001/diff/60001/src/arm/frames-arm.h#newcod... src/arm/frames-arm.h:112: static const int kSPOffset = -1 * kPointerSize; It's look strange for me that kSPOffset == kCodeOffset. We have the following code: EnterExitFrame: mov(ip, Operand(CodeObject())); push(ip); // Accessed from ExitFrame::code_slot. // Save the frame pointer and the context in top. mov(ip, Operand(ExternalReference(Top::k_c_entry_fp_address))); str(fp, MemOperand(ip)); mov(ip, Operand(ExternalReference(Top::k_context_address))); str(cp, MemOperand(ip)); if (save_doubles) { mov(ip, Operand(0)); // Marker and alignment word. push(ip); ... } CEntryStub::GenerateCore: masm->add(lr, pc, Operand(4)); // Compute return address: (pc + 8) + 4 masm->push(lr); masm->Jump(r5); As I see SPOffset shares its place with marker but not with the Code object. In ia-32 they are different: static const int kCodeOffset = -2 * kPointerSize; static const int kSPOffset = -1 * kPointerSize; If the return address is never fixed from GC (since the the CEntryStub is de facto unmovable) it may explain why it doesn't crash. http://codereview.chromium.org/6170001/diff/60001/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/60001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1464: int frame_alignment = ActivationFrameAlignment(); On 2011/01/20 13:23:51, zaheer wrote: > On 2011/01/20 12:09:28, SeRya wrote: > > Currently CEntryStub stack layout is following: > > <caller stack frame> > > <stack alignment place holder initialized by 0?> > > <caller frame pointer> > > <caller stack pointer after unwinding> > > <return address> > > <code object> > > <return address to the stub or 0-marker> > > <fp state?> > > > > Your layout: > > <caller stack frame> > > <caller frame pointer> > > <caller stack pointer after unwinding> > > <1-marker> > > <pointer to the return address> > > <stack alignment uninitialized place holder?> > > <return address> > > <code object> > > <return address> > Explaining the issue below, my layout in bit more detail > <caller stack frame> > <return address> > <caller stack pointer after unwinding> > <caller frame pointer> > <code object> <-- centry sp, cant use the same in current case > <marker> > <exit frame sp> > . > . > <arguments> > . > . > <stack alignment> the placeholder above arguments. > <return address> > <native call stack> > > I cannot completely reuse the exit frame layout since arguments come in between > exit frame and the native entry and hence the need to put additional slot > (marker/stack) to point to return address. > > If you already considered the above issue, maybe i miss your point. Sorry, I forget about the arguments. But it's possible anyway to come to the compatible layout. In contrast with ia32/x64 return address don't forced to be on top of the stack. Lets put in in a fixed place instead of stack top: 1. Reserve room for the return address where it's expected to be in CEntryStub (just before arguments): sub(sp, sp, Operand((arg_stack_space + 1) * kPointerSize)); (for now there is the marker at this place, so it is not needed) 2. Don't push the return address, put it to that place: add(ip, pc, Operand(4)); str(ip, MemOperand(fp, ExitFrameConstants::kSPOffset)); Jump(r2); // Call the api function. 3. In DirectCEntryStub::Generate use this slot to get real return address: ldr(ip, MemOperand(fp, ExitFrameConstants::kSPOffset));
hi serya, Most of the comments are addressed. Could you please take another look. Thanks http://codereview.chromium.org/6170001/diff/60001/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/60001/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1464: int frame_alignment = ActivationFrameAlignment(); On 2011/01/20 15:18:47, SeRya wrote: > On 2011/01/20 13:23:51, zaheer wrote: > > On 2011/01/20 12:09:28, SeRya wrote: > > > Currently CEntryStub stack layout is following: > > > <caller stack frame> > > > <stack alignment place holder initialized by 0?> > > > <caller frame pointer> > > > <caller stack pointer after unwinding> > > > <return address> > > > <code object> > > > <return address to the stub or 0-marker> > > > <fp state?> > > > > > > Your layout: > > > <caller stack frame> > > > <caller frame pointer> > > > <caller stack pointer after unwinding> > > > <1-marker> > > > <pointer to the return address> > > > <stack alignment uninitialized place holder?> > > > <return address> > > > <code object> > > > <return address> > > Explaining the issue below, my layout in bit more detail > > <caller stack frame> > > <return address> > > <caller stack pointer after unwinding> > > <caller frame pointer> > > <code object> <-- centry sp, cant use the same in current case > > <marker> > > <exit frame sp> > > . > > . > > <arguments> > > . > > . > > <stack alignment> > the placeholder above arguments. > > <return address> > > <native call stack> > > > > I cannot completely reuse the exit frame layout since arguments come in > between > > exit frame and the native entry and hence the need to put additional slot > > (marker/stack) to point to return address. > > > > If you already considered the above issue, maybe i miss your point. > > Sorry, I forget about the arguments. But it's possible anyway to come to the > compatible layout. In contrast with ia32/x64 return address don't forced to be > on top of the stack. Lets put in in a fixed place instead of stack top: Thanks. it does simplify it a lot. > > 1. Reserve room for the return address where it's expected to be in CEntryStub > (just before arguments): sub(sp, sp, Operand((arg_stack_space + 1) * > kPointerSize)); > (for now there is the marker at this place, so it is not needed) > 2. Don't push the return address, put it to that place: > add(ip, pc, Operand(4)); > str(ip, MemOperand(fp, ExitFrameConstants::kSPOffset)); > Jump(r2); // Call the api function. Done > 3. In DirectCEntryStub::Generate use this slot to get real return address: > ldr(ip, MemOperand(fp, ExitFrameConstants::kSPOffset)); could you pls check, code object is stored at sp offset, i have introduced a new constant kPCOffset which points to fp -2
hi serya, anton, Could you please take a look. Updated with support for direct callback in simulator. now the same code executes on device/simulator. Passes unit tests (on simulator), browsing tests (on device), code movement test (simulator and device). ./tools/test.py --mode=debug --simulator=arm .. [114:45|% 100|+ 4170|- 0]: Done
Thank you for the simplifying the exit fame. LGTM with a few comments. http://codereview.chromium.org/6170001/diff/65002/src/arm/frames-arm.h File src/arm/frames-arm.h (right): http://codereview.chromium.org/6170001/diff/65002/src/arm/frames-arm.h#newcod... src/arm/frames-arm.h:116: static const int kMarkerOffset = -2 * kPointerSize; Actually the same technique (used fixed place in the stack frame for the return address) could be used to avoid this marker (and simplify exit frame layout). I don't think it worth to do it in this CL but you can update this TODO. http://codereview.chromium.org/6170001/diff/65002/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/65002/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1401: EnterExitFrame(false, arg_stack_space + 5); +1? http://codereview.chromium.org/6170001/diff/65002/src/arm/simulator-arm.cc File src/arm/simulator-arm.cc (right): http://codereview.chromium.org/6170001/diff/65002/src/arm/simulator-arm.cc#ne... src/arm/simulator-arm.cc:1590: SimulatorRuntimeApiCall target = May be to add ASSERT that where is indeed one argument? PrepareCallApiFunction looks quite general, but in simulator only one argument is actually passed.
hi Serya, May you take another look. Thanks, Zaheer http://codereview.chromium.org/6170001/diff/65002/src/arm/frames-arm.h File src/arm/frames-arm.h (right): http://codereview.chromium.org/6170001/diff/65002/src/arm/frames-arm.h#newcod... src/arm/frames-arm.h:116: static const int kMarkerOffset = -2 * kPointerSize; On 2011/01/21 09:52:36, SeRya wrote: > Actually the same technique (used fixed place in the stack frame for the return > address) could be used to avoid this marker (and simplify exit frame layout). I > don't think it worth to do it in this CL but you can update this TODO. Done. http://codereview.chromium.org/6170001/diff/65002/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/65002/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1401: EnterExitFrame(false, arg_stack_space + 5); On 2011/01/21 09:52:36, SeRya wrote: > +1? Done. updated EnterExitFrame to encapsulate the pushes in that function. Note currently it does not take in to account pushes with save_doubles=true (as the existing code) http://codereview.chromium.org/6170001/diff/65002/src/arm/simulator-arm.cc File src/arm/simulator-arm.cc (right): http://codereview.chromium.org/6170001/diff/65002/src/arm/simulator-arm.cc#ne... src/arm/simulator-arm.cc:1590: SimulatorRuntimeApiCall target = On 2011/01/21 09:52:36, SeRya wrote: > May be to add ASSERT that where is indeed one argument? PrepareCallApiFunction > looks quite general, but in simulator only one argument is actually passed. simulator also uses stack space for the arguments (ie.. behavior is exactly same as in device). r0 points to Arguments&
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#n... src/arm/code-stubs-arm.cc:2723: __ mov(r5, Operand(r1)); I don't see the benefit of moving this outside the EnterExitFrame, but if you are going to do it then you should update the comment in macro-assembler-arm.h. 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... src/arm/macro-assembler-arm.h:259: // |stack_space| pending pushes, used for alignment before call to C. 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#ne... src/arm/simulator-arm.cc:800: void* external_function, bool fp_return, ExternalReference::Type type) { You shouldn't need both the bool for the fp_return and the external reference type. They can both be folded into the external reference type.
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#newco... 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#n... 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#ne... 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... 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... 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#ne... 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#ne... 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#n... 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#n... 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?
Thanks Erik and Anton for your comments. Uploaded patch with comments addressed. Could you please take another look. 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#n... src/arm/code-stubs-arm.cc:2710: // popping the args. On 2011/01/21 17:56:36, antonm wrote: > 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 Done. http://codereview.chromium.org/6170001/diff/90001/src/arm/code-stubs-arm.cc#n... src/arm/code-stubs-arm.cc:2723: __ mov(r5, Operand(r1)); On 2011/01/21 14:28:40, Erik Corry wrote: > I don't see the benefit of moving this outside the EnterExitFrame, but if you > are going to do it then you should update the comment in macro-assembler-arm.h. Updated comment in macro-assembler-arm.h. 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#ne... src/arm/code-stubs-arm.h:459: // Stub used when returning from direct native call to generated code. On 2011/01/21 17:56:36, antonm wrote: > 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 :) Thanks. I think your description is more accurate and complete. Done. 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... src/arm/macro-assembler-arm.cc:547: int pending_pushes = stack_space + 4; // 4 pushes in this function. On 2011/01/21 17:56:36, antonm wrote: > 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? sorry i miss your comment. > > BTW, do we have tests for API function invocations with many (> 3) arguments? the call api requests space for 4 arguments (Arguments::implicit_args, values, length, is_construct) and existing tests e.g. (FastApiCallback_SimpleSignature) already cover this case. Maybe i miss your comment. 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... src/arm/macro-assembler-arm.h:259: // |stack_space| pending pushes, used for alignment before call to C. On 2011/01/21 17:56:36, antonm wrote: > 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. > i have replaced |stack_space| -> stack_space - 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#ne... src/arm/simulator-arm.cc:800: void* external_function, bool fp_return, ExternalReference::Type type) { On 2011/01/21 14:28:40, Erik Corry wrote: > You shouldn't need both the bool for the fp_return and the external reference > type. They can both be folded into the external reference type. Done. http://codereview.chromium.org/6170001/diff/90001/src/arm/simulator-arm.cc#ne... src/arm/simulator-arm.cc:1543: // This signature supports direct call in to js function native callback On 2011/01/21 17:56:36, antonm wrote: > I'd use API function instead of js function here Done. http://codereview.chromium.org/6170001/diff/90001/src/arm/simulator-arm.cc#ne... src/arm/simulator-arm.cc:1590: SimulatorRuntimeApiCall target = On 2011/01/21 17:56:36, antonm wrote: > please, consider refactoring the common logic of logging and stack alignment > checks The log is unique to each case, the stack alignment check refactoring also has a problem - moving it down will mean it will print after executing function, moving it before will impact readability of log. Any ideas. 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#n... src/arm/stub-cache-arm.cc:622: __ stm(ib, sp, r5.bit() | r6.bit()); On 2011/01/21 17:56:36, antonm wrote: > Please, add a comment Done. http://codereview.chromium.org/6170001/diff/90001/src/arm/stub-cache-arm.cc#n... src/arm/stub-cache-arm.cc:624: // r2 points to calldata as expected by Arguments class (refer layout above) On 2011/01/21 17:56:36, antonm wrote: > nit: either call_data or call data, please Done. 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 On 2011/01/21 17:56:36, antonm wrote: > 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? Agree. i simplified one more bit and removed STRONG_ROOT_LIST_ARM. #if V8_TARGET_ARCH_ARM && !V8_INTERPRETED_REGEXP #define STRONG_ROOT_LIST(V) \ UNCONDITIONAL_STRONG_ROOT_LIST(V) \ V(Code, re_c_entry_code, RegExpCEntryCode) \ V(Code, direct_c_entry_code, DirectCEntryCode) #elif V8_TARGET_ARCH_ARM #define STRONG_ROOT_LIST(V) \ UNCONDITIONAL_STRONG_ROOT_LIST(V) \ V(Code, direct_c_entry_code, DirectCEntryCode) #else #define STRONG_ROOT_LIST(V) \ UNCONDITIONAL_STRONG_ROOT_LIST(V) #endif Is this ok? 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 On 2011/01/21 17:56:36, antonm wrote: > why this change? There's a circular reference with introduction of new type ExternalReference::Type. it is declared in assembler.h needs to be used in simulator.h (RedirectExternalReference), but assembler includes simulator.h through top.h.
ARM parts LGTM. Zaheer mentioned that he had a test that crashed with an earlier version of the patch. It would be very good if that test could be made into a form where it could be committed along with this change. 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... src/arm/macro-assembler-arm.h:259: // |stack_space| pending pushes, used for alignment before call to C. On 2011/01/21 17:56:36, antonm wrote: > 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. Sorry for causing this confusion. I didn't check properly, and the style is used elsewhere in the file. We are not very consistent in the format of these comments. I think both what Zaheer originally wrote and what he has now are both fine. > > 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/55003/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/55003/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1398: You should have 2 blank lines between functions. http://codereview.chromium.org/6170001/diff/55003/src/arm/simulator-arm.cc File src/arm/simulator-arm.cc (right): http://codereview.chromium.org/6170001/diff/55003/src/arm/simulator-arm.cc#ne... src/arm/simulator-arm.cc:1600: } else { The usual pattern in V8 is to add a line here saying something like. ASSERT(redirection->type() == ExternalReference::...) which serves as a check and also a comment that aids the reader. http://codereview.chromium.org/6170001/diff/55003/src/arm/simulator-arm.h File src/arm/simulator-arm.h (right): http://codereview.chromium.org/6170001/diff/55003/src/arm/simulator-arm.h#new... src/arm/simulator-arm.h:289: v8::internal::ExternalReference::Type type); The indentation of these two parameters is wrong. http://codereview.chromium.org/6170001/diff/55003/src/arm/stub-cache-arm.cc File src/arm/stub-cache-arm.cc (right): http://codereview.chromium.org/6170001/diff/55003/src/arm/stub-cache-arm.cc#n... src/arm/stub-cache-arm.cc:595: static bool GenerateFastApiDirectCall(MacroAssembler* masm, Instead of returning a bool and using a Failure** to return the failure this function should return a MaybeObject*. This is the pattern used throughout V8 for functions that may encounter an allocation failure. You can return Heap::undefined_value() in the success cases where there is nothing to return.
Thanks Erik for review, updated as per comments. Added a test. Please review. http://codereview.chromium.org/6170001/diff/55003/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/55003/src/arm/macro-assembler-arm... src/arm/macro-assembler-arm.cc:1398: On 2011/01/24 21:45:16, Erik Corry wrote: > You should have 2 blank lines between functions. Done. http://codereview.chromium.org/6170001/diff/55003/src/arm/simulator-arm.cc File src/arm/simulator-arm.cc (right): http://codereview.chromium.org/6170001/diff/55003/src/arm/simulator-arm.cc#ne... src/arm/simulator-arm.cc:1600: } else { On 2011/01/24 21:45:16, Erik Corry wrote: > The usual pattern in V8 is to add a line here saying something like. > ASSERT(redirection->type() == ExternalReference::...) > which serves as a check and also a comment that aids the reader. Done. http://codereview.chromium.org/6170001/diff/55003/src/arm/simulator-arm.h File src/arm/simulator-arm.h (right): http://codereview.chromium.org/6170001/diff/55003/src/arm/simulator-arm.h#new... src/arm/simulator-arm.h:289: v8::internal::ExternalReference::Type type); On 2011/01/24 21:45:16, Erik Corry wrote: > The indentation of these two parameters is wrong. The second parameter is too long for 80chars. moved both to next lines and aligned it. http://codereview.chromium.org/6170001/diff/55003/src/arm/stub-cache-arm.cc File src/arm/stub-cache-arm.cc (right): http://codereview.chromium.org/6170001/diff/55003/src/arm/stub-cache-arm.cc#n... src/arm/stub-cache-arm.cc:595: static bool GenerateFastApiDirectCall(MacroAssembler* masm, On 2011/01/24 21:45:16, Erik Corry wrote: > Instead of returning a bool and using a Failure** to return the failure this > function should return a MaybeObject*. This is the pattern used throughout V8 > for functions that may encounter an allocation failure. You can return > Heap::undefined_value() in the success cases where there is nothing to return. Actually i followed the IA32 return structure. Modified as per suggestion. Done.
Great work, thanks. Almost LGTM---you need to rebase your change after http://code.google.com/p/v8/source/detail?r=6448. Thanks again. 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... src/arm/macro-assembler-arm.cc:547: int pending_pushes = stack_space + 4; // 4 pushes in this function. On 2011/01/24 09:43:31, zaheer wrote: > On 2011/01/21 17:56:36, antonm wrote: > > 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? > sorry i miss your comment. > > > > BTW, do we have tests for API function invocations with many (> 3) arguments? > the call api requests space for 4 arguments (Arguments::implicit_args, values, > length, is_construct) and existing tests e.g. (FastApiCallback_SimpleSignature) > already cover this case. > Maybe i miss your comment. > I am sorry, I meant it's somewhat strange to see pending_pushes % 2 == (stack_space + 4) % 2 == stack_space % 2 + 4 % 2 == stack_space % 2. Overall, may I ask you to get rid of pending_pushes altogether and instead have something like: // If stack is unaligned.... Condition condition = stack_space % 2 == 0 ? ne : eq; push(r7, condition) or // If stack is unaligned... if (stack_space % 2 == 0) { push(r7, ne); } ... If you prefer to keep constant 4 to make it more obvious how to change this function later, may you introduced named constant and once again write something like if ((stack_space + kEnterExitFramePushes) % 2) And regarding tests: I meant API function which accept many argument, not the Arguments structure, but it's not relevant here, sorry for noise. 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#ne... src/arm/simulator-arm.cc:1590: SimulatorRuntimeApiCall target = On 2011/01/24 09:43:31, zaheer wrote: > On 2011/01/21 17:56:36, antonm wrote: > > please, consider refactoring the common logic of logging and stack alignment > > checks > The log is unique to each case, the stack alignment check refactoring also has a > problem - moving it down will mean it will print after executing function, > moving it before will impact readability of log. Any ideas. Ok, let's leave it as you have it now. 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 Nice, thanks. On 2011/01/24 09:43:31, zaheer wrote: > On 2011/01/21 17:56:36, antonm wrote: > > 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? > Agree. i simplified one more bit and removed STRONG_ROOT_LIST_ARM. > > #if V8_TARGET_ARCH_ARM && !V8_INTERPRETED_REGEXP > #define STRONG_ROOT_LIST(V) \ > UNCONDITIONAL_STRONG_ROOT_LIST(V) \ > V(Code, re_c_entry_code, RegExpCEntryCode) \ > V(Code, direct_c_entry_code, DirectCEntryCode) > #elif V8_TARGET_ARCH_ARM > #define STRONG_ROOT_LIST(V) \ > UNCONDITIONAL_STRONG_ROOT_LIST(V) \ > V(Code, direct_c_entry_code, DirectCEntryCode) > #else > #define STRONG_ROOT_LIST(V) \ > UNCONDITIONAL_STRONG_ROOT_LIST(V) > #endif > Is this ok? http://codereview.chromium.org/6170001/diff/119001/src/arm/simulator-arm.cc File src/arm/simulator-arm.cc (right): http://codereview.chromium.org/6170001/diff/119001/src/arm/simulator-arm.cc#n... src/arm/simulator-arm.cc:1601: // builtin/runtime call nit: add a dot please. v8 style is to end comments with a dot when they start as the statement indent and leave no end if they are inline: // Foo. bar(); // xyzzy http://codereview.chromium.org/6170001/diff/119001/src/arm/stub-cache-arm.cc File src/arm/stub-cache-arm.cc (right): http://codereview.chromium.org/6170001/diff/119001/src/arm/stub-cache-arm.cc#... src/arm/stub-cache-arm.cc:624: // r2 points to call data as expected by Arguments class (refer layout above). nit: refer <to> layout above? http://codereview.chromium.org/6170001/diff/119001/test/cctest/test-api.cc File test/cctest/test-api.cc (right): http://codereview.chromium.org/6170001/diff/119001/test/cctest/test-api.cc#ne... test/cctest/test-api.cc:7242: v8::Context::Scope context_scope(context); we have handy LocalContext to get rid of those two lines (7241--7242) http://codereview.chromium.org/6170001/diff/119001/test/cctest/test-api.cc#ne... test/cctest/test-api.cc:7246: context->Global()->Set(String::New("nativeobject"), nativeobject_obj); we have handy v8_str for String::New http://codereview.chromium.org/6170001/diff/119001/test/cctest/test-api.cc#ne... test/cctest/test-api.cc:7250: v8::Handle<Value> value = CompileRun( nit: as you don't need return value, you can just omit it. http://codereview.chromium.org/6170001/diff/119001/test/cctest/test-api.cc#ne... test/cctest/test-api.cc:7252: " function inner (){ " nit: no space before ( in function declaration and a space before { here and below, please http://codereview.chromium.org/6170001/diff/119001/test/cctest/test-api.cc#ne... test/cctest/test-api.cc:7260: " outer()(); " why such involved structure: function which creates a closure?
Uploaded patch rebased to bleeding edge. The diffs show up other work so not sure if i need to create a separate CL. >> You need to rebase your change after >> http://code.google.com/p/v8/source/detail?r=6448. Done Could you please take a look. Thanks 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... src/arm/macro-assembler-arm.cc:547: int pending_pushes = stack_space + 4; // 4 pushes in this function. On 2011/01/26 11:36:37, antonm wrote: > On 2011/01/24 09:43:31, zaheer wrote: > > On 2011/01/21 17:56:36, antonm wrote: > > > 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? > > sorry i miss your comment. > > > > > > BTW, do we have tests for API function invocations with many (> 3) > arguments? > > the call api requests space for 4 arguments (Arguments::implicit_args, values, > > length, is_construct) and existing tests e.g. > (FastApiCallback_SimpleSignature) > > already cover this case. > > Maybe i miss your comment. > > > > I am sorry, I meant it's somewhat strange to see pending_pushes % 2 == > (stack_space + 4) % 2 == stack_space % 2 + 4 % 2 == stack_space % 2. > > Overall, may I ask you to get rid of pending_pushes altogether and instead have > something like: > > // If stack is unaligned.... > Condition condition = stack_space % 2 == 0 ? ne : eq; > push(r7, condition) > > or > > // If stack is unaligned... > if (stack_space % 2 == 0) { > push(r7, ne); > } ... > > If you prefer to keep constant 4 to make it more obvious how to change this > function later, may you introduced named constant and once again write something > like > > if ((stack_space + kEnterExitFramePushes) % 2) > > And regarding tests: I meant API function which accept many argument, not the > Arguments structure, but it's not relevant here, sorry for noise. not required with the rebase. http://codereview.chromium.org/6170001/diff/119001/src/arm/simulator-arm.cc File src/arm/simulator-arm.cc (right): http://codereview.chromium.org/6170001/diff/119001/src/arm/simulator-arm.cc#n... src/arm/simulator-arm.cc:1601: // builtin/runtime call On 2011/01/26 11:36:38, antonm wrote: > nit: add a dot please. > > v8 style is to end comments with a dot when they start as the statement indent > and leave no end if they are inline: > > // Foo. > bar(); // xyzzy Done. http://codereview.chromium.org/6170001/diff/119001/src/arm/stub-cache-arm.cc File src/arm/stub-cache-arm.cc (right): http://codereview.chromium.org/6170001/diff/119001/src/arm/stub-cache-arm.cc#... src/arm/stub-cache-arm.cc:624: // r2 points to call data as expected by Arguments class (refer layout above). On 2011/01/26 11:36:38, antonm wrote: > nit: refer <to> layout above? Done. http://codereview.chromium.org/6170001/diff/119001/test/cctest/test-api.cc File test/cctest/test-api.cc (right): http://codereview.chromium.org/6170001/diff/119001/test/cctest/test-api.cc#ne... test/cctest/test-api.cc:7242: v8::Context::Scope context_scope(context); On 2011/01/26 11:36:38, antonm wrote: > we have handy LocalContext to get rid of those two lines (7241--7242) Done. http://codereview.chromium.org/6170001/diff/119001/test/cctest/test-api.cc#ne... test/cctest/test-api.cc:7246: context->Global()->Set(String::New("nativeobject"), nativeobject_obj); On 2011/01/26 11:36:38, antonm wrote: > we have handy v8_str for String::New Done. http://codereview.chromium.org/6170001/diff/119001/test/cctest/test-api.cc#ne... test/cctest/test-api.cc:7250: v8::Handle<Value> value = CompileRun( On 2011/01/26 11:36:38, antonm wrote: > nit: as you don't need return value, you can just omit it. Done. http://codereview.chromium.org/6170001/diff/119001/test/cctest/test-api.cc#ne... test/cctest/test-api.cc:7252: " function inner (){ " On 2011/01/26 11:36:38, antonm wrote: > nit: no space before ( in function declaration and a space before { here and > below, please Done. http://codereview.chromium.org/6170001/diff/119001/test/cctest/test-api.cc#ne... test/cctest/test-api.cc:7260: " outer()(); " On 2011/01/26 11:36:38, antonm wrote: > why such involved structure: function which creates a closure? simplified. on device test, the stub doesnt seem to move in initial few gcs, so i have used 30 iterations (10 gcs) to ensure stub moves. tracking gc for the stub address shows the initial gc forward logs show the same address forward 0x4086dba0 -> 0x4086dba0 forward 0x40875de0 -> 0x40875de0 forward 0x40877cc0 -> 0x40877cc0 forward 0x40875de0 -> 0x40875de0 forward 0x40877cc0 -> 0x408729a0 (address changed)
http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-ar... File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-ar... src/arm/macro-assembler-arm.cc:670: // Reserve place for the return address and align the frame preparing for Please update this comment. http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-ar... src/arm/macro-assembler-arm.cc:1482: ASSERT(allow_stub_calls()); // stub calls are not allowed in some stubs Start comment with uppercase letter end and with period. http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-ar... src/arm/macro-assembler-arm.cc:1520: // return address pushed on stack (could have moved after GC). As far as I can see this relies on DirectCEntryStub itself never moving. It is the same assumption we have for the CEntryStub (and the RegExpCEntryStub I think). Please add a comment on this. For the CEntryStub we have been safe so far as it is generated quite early (with crankshaft this is actually not the case for the variant that saves doubles). How about this will it be generated early, or can a test case where this actually moves be crafted? http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-ar... src/arm/macro-assembler-arm.cc:1521: DirectCEntryStub stub; I thing the calling of this stub should be factored out, maybe to a method on the stub stub.GenerateCall(ref) as this relies on the exact code generated by the stub. http://codereview.chromium.org/6170001/diff/125002/src/assembler.h File src/assembler.h (right): http://codereview.chromium.org/6170001/diff/125002/src/assembler.h#newcode471 src/assembler.h:471: // BUILTIN_CALL - builtin/runtime call. builtin/runtime -> builtin http://codereview.chromium.org/6170001/diff/125002/src/assembler.h#newcode474 src/assembler.h:474: // FP_CALL - builtin/runtime call that returns floating point. Please change "builtin/runtime call that returns floating point." to something like "direct call to a C-function which will never cause a GC". It is not always a function with the signature double f(double, double), e.g. "native_compare_doubles". http://codereview.chromium.org/6170001/diff/125002/src/assembler.h#newcode483 src/assembler.h:483: FP_RETURN_CALL, FP_RETURN_CALL -> FP_CALL as in comment.
http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-ar... File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-ar... src/arm/macro-assembler-arm.cc:1492: int64_t offset = (ref0.address() - ref1.address()); should have noticed that earlier, sorry. I think ARM is 32-bit platform (or at least we only support 32-bit ARM), so you don't need to be that paranoid about offsets: return ref0.address() - ref1.address() should be enough. http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-ar... src/arm/macro-assembler-arm.cc:1520: // return address pushed on stack (could have moved after GC). On 2011/02/02 13:24:38, Søren Gjesse wrote: > As far as I can see this relies on DirectCEntryStub itself never moving. It is > the same assumption we have for the CEntryStub (and the RegExpCEntryStub I > think). Please add a comment on this. > > For the CEntryStub we have been safe so far as it is generated quite early (with > crankshaft this is actually not the case for the variant that saves doubles). > How about this will it be generated early, or can a test case where this > actually moves be crafted? Søren, yes, that's exactly the reason we call goes indirectly via stub. http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-ar... src/arm/macro-assembler-arm.cc:1560: mov(ip, Operand(ExternalReference(Top::k_pending_exception_address))); that should be scheduled_exception, not pending. (scheduled exceptions are thrown by embedder's code, pending by v8 and JS) and if this version passes the tests, that sucks. May I ask you to add a test case which reveals the bug? You should use v8::ThrowException in API callback http://codereview.chromium.org/6170001/diff/125002/test/cctest/test-api.cc File test/cctest/test-api.cc (right): http://codereview.chromium.org/6170001/diff/125002/test/cctest/test-api.cc#ne... test/cctest/test-api.cc:7360: v8::Handle<v8::ObjectTemplate> global = v8::ObjectTemplate::New(); looks like you shouldn't use this global any more.
http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-ar... File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-ar... src/arm/macro-assembler-arm.cc:1520: // return address pushed on stack (could have moved after GC). On 2011/02/02 13:56:28, antonm wrote: > On 2011/02/02 13:24:38, Søren Gjesse wrote: > > As far as I can see this relies on DirectCEntryStub itself never moving. It is > > the same assumption we have for the CEntryStub (and the RegExpCEntryStub I > > think). Please add a comment on this. > > > > For the CEntryStub we have been safe so far as it is generated quite early > (with > > crankshaft this is actually not the case for the variant that saves doubles). > > How about this will it be generated early, or can a test case where this > > actually moves be crafted? > > Søren, yes, that's exactly the reason we call goes indirectly via stub. Sure, and as discussed offline having DirectCEntryStub created in Heap::CreateFixedStubs will in practice ensure that DirectCEntryStub itself will not move even though there is no explicit code in the GC to ensure that.
http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-ar... File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-ar... src/arm/macro-assembler-arm.cc:1520: // return address pushed on stack (could have moved after GC). On 2011/02/02 13:56:28, antonm wrote: > On 2011/02/02 13:24:38, Søren Gjesse wrote: > > As far as I can see this relies on DirectCEntryStub itself never moving. It is > > the same assumption we have for the CEntryStub (and the RegExpCEntryStub I > > think). Please add a comment on this. > > > > For the CEntryStub we have been safe so far as it is generated quite early > (with > > crankshaft this is actually not the case for the variant that saves doubles). > > How about this will it be generated early, or can a test case where this > > actually moves be crafted? > > Søren, yes, that's exactly the reason we call goes indirectly via stub. Sure, and as discussed offline having DirectCEntryStub created in Heap::CreateFixedStubs will in practice ensure that DirectCEntryStub itself will not move even though there is no explicit code in the GC to ensure that.
Thanks Soren, Anton for your comments. Could you please take another look at updated patch. http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-ar... File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-ar... src/arm/macro-assembler-arm.cc:670: // Reserve place for the return address and align the frame preparing for On 2011/02/02 13:24:38, Søren Gjesse wrote: > Please update this comment. Done. http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-ar... src/arm/macro-assembler-arm.cc:1482: ASSERT(allow_stub_calls()); // stub calls are not allowed in some stubs On 2011/02/02 13:24:38, Søren Gjesse wrote: > Start comment with uppercase letter end and with period. Done. http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-ar... src/arm/macro-assembler-arm.cc:1492: int64_t offset = (ref0.address() - ref1.address()); On 2011/02/02 13:56:28, antonm wrote: > should have noticed that earlier, sorry. I think ARM is 32-bit platform (or at > least we only support 32-bit ARM), so you don't need to be that paranoid about > offsets: return ref0.address() - ref1.address() should be enough. sorry copy/paste mistake from x64. Done. http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-ar... src/arm/macro-assembler-arm.cc:1520: // return address pushed on stack (could have moved after GC). On 2011/02/02 14:23:50, Søren Gjesse wrote: > On 2011/02/02 13:56:28, antonm wrote: > > On 2011/02/02 13:24:38, Søren Gjesse wrote: > > > As far as I can see this relies on DirectCEntryStub itself never moving. It > is > > > the same assumption we have for the CEntryStub (and the RegExpCEntryStub I > > > think). Please add a comment on this. > > > > > > For the CEntryStub we have been safe so far as it is generated quite early > > (with > > > crankshaft this is actually not the case for the variant that saves > doubles). > > > How about this will it be generated early, or can a test case where this > > > actually moves be crafted? > > > > Søren, yes, that's exactly the reason we call goes indirectly via stub. > > Sure, and as discussed offline having DirectCEntryStub created in > Heap::CreateFixedStubs will in practice ensure that DirectCEntryStub itself will > not move even though there is no explicit code in the GC to ensure that. Added comment http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-ar... src/arm/macro-assembler-arm.cc:1521: DirectCEntryStub stub; On 2011/02/02 13:24:38, Søren Gjesse wrote: > I thing the calling of this stub should be factored out, maybe to a method on > the stub > > stub.GenerateCall(ref) > > as this relies on the exact code generated by the stub. Done. passed function as a parameter instead of ref. http://codereview.chromium.org/6170001/diff/125002/src/arm/macro-assembler-ar... src/arm/macro-assembler-arm.cc:1560: mov(ip, Operand(ExternalReference(Top::k_pending_exception_address))); On 2011/02/02 13:56:28, antonm wrote: > that should be scheduled_exception, not pending. > > (scheduled exceptions are thrown by embedder's code, pending by v8 and JS) > > and if this version passes the tests, that sucks. May I ask you to add a test > case which reveals the bug? You should use v8::ThrowException in API callback Thanks for catching and the explanation, i wasnt aware of the difference and none of the unit tests/browsing showed up the problem. Added a test. http://codereview.chromium.org/6170001/diff/125002/src/assembler.h File src/assembler.h (right): http://codereview.chromium.org/6170001/diff/125002/src/assembler.h#newcode471 src/assembler.h:471: // BUILTIN_CALL - builtin/runtime call. On 2011/02/02 13:24:38, Søren Gjesse wrote: > builtin/runtime -> builtin Done. http://codereview.chromium.org/6170001/diff/125002/src/assembler.h#newcode474 src/assembler.h:474: // FP_CALL - builtin/runtime call that returns floating point. On 2011/02/02 13:24:38, Søren Gjesse wrote: > Please change "builtin/runtime call that returns floating point." to something > like "direct call to a C-function which will never cause a GC". It is not always > a function with the signature double f(double, double), e.g. > "native_compare_doubles". if iam not mistaken, native_compare_doubles uses the BUILTIN_CALL case and not FP_RETURN case. In that sense isnt the current comment accurate? http://codereview.chromium.org/6170001/diff/125002/src/assembler.h#newcode483 src/assembler.h:483: FP_RETURN_CALL, On 2011/02/02 13:24:38, Søren Gjesse wrote: > FP_RETURN_CALL -> FP_CALL as in comment. modified the comment to FP_RETURN_CALL to reflect the current naming. Let me know if FP_CALL would be more accurate http://codereview.chromium.org/6170001/diff/125002/test/cctest/test-api.cc File test/cctest/test-api.cc (right): http://codereview.chromium.org/6170001/diff/125002/test/cctest/test-api.cc#ne... test/cctest/test-api.cc:7360: v8::Handle<v8::ObjectTemplate> global = v8::ObjectTemplate::New(); On 2011/02/02 13:56:28, antonm wrote: > looks like you shouldn't use this global any more. Done.
LGTM for what I have commented on. http://codereview.chromium.org/6170001/diff/125002/src/assembler.h File src/assembler.h (right): http://codereview.chromium.org/6170001/diff/125002/src/assembler.h#newcode474 src/assembler.h:474: // FP_CALL - builtin/runtime call that returns floating point. On 2011/02/03 07:27:31, zaheer wrote: > On 2011/02/02 13:24:38, Søren Gjesse wrote: > > Please change "builtin/runtime call that returns floating point." to something > > like "direct call to a C-function which will never cause a GC". It is not > always > > a function with the signature double f(double, double), e.g. > > "native_compare_doubles". > if iam not mistaken, native_compare_doubles uses the BUILTIN_CALL case and not > FP_RETURN case. In that sense isnt the current comment accurate? > > You are right it does, and of cause returns an int. However it is rather misleading to use BUILTIN_CALL, as the function is actually expecting two doubles on the stack, but lets leave it like this for now. One small thing though, the function power_double_double in assembler.cc should be marked with FP_RETURN_CALL - it is currently wrongly missing the true argument. It is not currently used on ARM, but probably will.
LGTM and to Erik as well. I'll land addressing myself minor formatting issues. Thanks for a great work, Zaheer. http://codereview.chromium.org/6170001/diff/129005/src/arm/code-stubs-arm.cc File src/arm/code-stubs-arm.cc (right): http://codereview.chromium.org/6170001/diff/129005/src/arm/code-stubs-arm.cc#... src/arm/code-stubs-arm.cc:5735: RelocInfo::CODE_TARGET)); indentation is slightly off. http://codereview.chromium.org/6170001/diff/129005/src/assembler.cc File src/assembler.cc (right): http://codereview.chromium.org/6170001/diff/129005/src/assembler.cc#newcode902 src/assembler.cc:902: ExternalReference:: I'll format it differently. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
