Sorry, I forgot change the description. I discovered that this need to be a signed comparison because we calculate what the SP will be after pushing the arguments, and that calculated value could end up wrapping around 0 becoming negative. So my change ended up with only adding test for the flag check_stack. Erik Corry has CL http://codereview.chromium.org/2926 <http://codereview.chromium.org/2926>open which changes the logic and also takes preemption (either thread switch or debug break) into account.
/Søren On Fri, Sep 26, 2008 at 3:02 PM, Ivan Posva <[EMAIL PROTECTED]> wrote: > > Soren, > > Contrary to your commit comment you did not change the greater to > above_equal. Any reason for your change of heart? > > Thanks, > -Ivan > > > On Fri, Sep 26, 2008 at 12:29, <[EMAIL PROTECTED]> wrote: > > > > Author: [EMAIL PROTECTED] > > Date: Fri Sep 26 03:27:39 2008 > > New Revision: 384 > > > > Modified: > > branches/bleeding_edge/src/builtins-ia32.cc > > > > Log: > > Stack checks in generated code for function apply is now controlled > > by the check-stack flag. Changed the condition code from greater to > > above_equal as the SP should be unsigned (this matches the stack > > check in function entry). > > Review URL: http://codereview.chromium.org/4296 > > > > Modified: branches/bleeding_edge/src/builtins-ia32.cc > > > ============================================================================== > > --- branches/bleeding_edge/src/builtins-ia32.cc (original) > > +++ branches/bleeding_edge/src/builtins-ia32.cc Fri Sep 26 03:27:39 2008 > > @@ -517,21 +517,23 @@ > > > > // Eagerly check for stack-overflow before pushing all the arguments > > // to the stack. > > - Label okay; > > - __ lea(ecx, Operand(esp, -3 * kPointerSize)); // receiver, limit, > index > > - __ mov(edx, Operand(eax)); > > - __ shl(edx, kPointerSizeLog2 - kSmiTagSize); > > - __ sub(ecx, Operand(edx)); > > - ExternalReference stack_guard_limit_address = > > - ExternalReference::address_of_stack_guard_limit(); > > - __ cmp(ecx, Operand::StaticVariable(stack_guard_limit_address)); > > - __ j(greater, &okay, taken); > > + if (FLAG_check_stack) { > > + Label okay; > > + __ lea(ecx, Operand(esp, -3 * kPointerSize)); // receiver, limit, > > index > > + __ mov(edx, Operand(eax)); > > + __ shl(edx, kPointerSizeLog2 - kSmiTagSize); > > + __ sub(ecx, Operand(edx)); > > + ExternalReference stack_guard_limit_address = > > + ExternalReference::address_of_stack_guard_limit(); > > + __ cmp(ecx, Operand::StaticVariable(stack_guard_limit_address)); > > + __ j(greater, &okay, taken); > > > > - // Too bad: Out of stack space. > > - __ push(Operand(ebp, 4 * kPointerSize)); // push this > > - __ push(eax); > > - __ InvokeBuiltin(Builtins::APPLY_OVERFLOW, CALL_FUNCTION); > > - __ bind(&okay); > > + // Too bad: Out of stack space. > > + __ push(Operand(ebp, 4 * kPointerSize)); // push this > > + __ push(eax); > > + __ InvokeBuiltin(Builtins::APPLY_OVERFLOW, CALL_FUNCTION); > > + __ bind(&okay); > > + } > > > > // Push current index and limit. > > const int kLimitOffset = > > > > > > > > > > > --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
