Bug 129429 – [Win32][LLINT] Crash when running JSC stress tests.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

I'm currently getting a crash when running the JSC stress tests.
This is caused by accessing uncommitted stack memory.
This happens when we move the stack pointer by a value larger than the Windows stack guard page size (4K).
The system does not get the chance to move the guard page then, because we are not doing any stack reading or writing in between.
The stack guard page is the barrier between committed and uncommitted stack memory on Windows.
When someone tries to read or write to the guard page, the system commits that memory, and moves the guard page.

Created attachment 225378 [details]
Patch

With this patch I'm getting the results:

** The following JSC stress test failures have been introduced:
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-llint
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-baseline
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-dfg-eager-no-cjit-validate-phases
        stress/ftl-arithcos.js.default
        stress/ftl-arithcos.js.no-llint
        stress/ftl-arithcos.js.always-trigger-copy-phase
        stress/ftl-arithcos.js.no-cjit-validate-phases
        stress/ftl-arithcos.js.dfg-eager
        stress/ftl-arithcos.js.dfg-eager-no-cjit-validate
        jsc-layout-tests.yaml/js/script-tests/basic-map.js.layout
        jsc-layout-tests.yaml/js/script-tests/basic-map.js.layout-no-cjit
        jsc-layout-tests.yaml/js/script-tests/function-apply-many-args.js.layout
        jsc-layout-tests.yaml/js/script-tests/function-apply-many-args.js.layout-no-llint
        jsc-layout-tests.yaml/js/script-tests/function-apply-many-args.js.layout-no-cjit
        jsc-layout-tests.yaml/js/script-tests/function-apply-many-args.js.layout-dfg-eager-no-cjit
        jsc-layout-tests.yaml/js/script-tests/reentrant-caching.js.layout-no-llint
        jsc-layout-tests.yaml/js/script-tests/reentrant-caching.js.layout-dfg-eager-no-cjit
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-no-llint
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-no-cjit
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-dfg-eager-no-cjit

Results for JSC stress tests:
    22 failures found.

When running with the C Loop LLINT, I get these results:

** The following JSC stress test failures have been introduced:
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-llint
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-baseline
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-dfg-eager-no-cjit-validate-phases
        stress/ftl-arithcos.js.default
        stress/ftl-arithcos.js.no-llint
        stress/ftl-arithcos.js.always-trigger-copy-phase
        stress/ftl-arithcos.js.no-cjit-validate-phases
        stress/ftl-arithcos.js.dfg-eager
        stress/ftl-arithcos.js.dfg-eager-no-cjit-validate
        jsc-layout-tests.yaml/js/script-tests/basic-map.js.layout
        jsc-layout-tests.yaml/js/script-tests/basic-map.js.layout-no-llint
        jsc-layout-tests.yaml/js/script-tests/basic-map.js.layout-no-cjit
        jsc-layout-tests.yaml/js/script-tests/basic-map.js.layout-dfg-eager-no-cjit
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-no-llint
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-no-cjit
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-dfg-eager-no-cjit

Results for JSC stress tests:
    18 failures found.

[Csaba Osztrogonác]

Comment on attachment 225378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225378&action=review

> Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp:1136
> +#if USE(TEXTURE_MAPPER_GL)
>  PassOwnPtr<TextureMapper> TextureMapper::platformCreateAccelerated()
>  {
>      return TextureMapperGL::create();
>  }
> +#endif

It is an unrelated change.

Created attachment 225383 [details]
Patch

(In reply to comment #4)
> (From update of attachment 225378 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225378&action=review
> 
> It is an unrelated change.

Thanks for looking into this, updated patch :)

[Geoffrey Garen]

Is this what C programs do on Windows as well?

[Michael Saboff]

Comment on attachment 225383 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225383&action=review

This still needs some work.

> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:78
> +// See http ://msdn.microsoft.com/en-us/library/ms686774%28VS.85%29.aspx

Remove the space after "http".

I didn't see anything on the MSDN web page that each intermediate page needs to be touched when the allocation grows by multiple pages.  Did you verify this through testing?

> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:82
> +    const int winStackGuardPageSize = 4096;

We should use a OS provided value for the page size instead of hard coding a value here.

> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:83
> +    const TrustedImm32 delta(winStackGuardPageSize - 16);

Why subtract the 16?  It seems that the first time through the loop, we'll do the sub #0, [sp - 4080 (4k-16)].  The second time it is 8k - 32 and so on.  It seems that we might not touch all pages depending on where in the current page SP points.

> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:86
> +    move(TrustedImm32(-offset), temp);
> +    sub32(base, temp);

You should be using subPtr, since we want this to eventually work for 64 bit.

After these two instructions, temp = -offset - base.

If you want temp to be base - offset::
    move(base, temp);
    subPtr(TrustedImm32(offset), temp)
Or if you want temp to be the amount of bytes you're moving down, then the sub is not needed.

> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:94
> +    sub32(TrustedImm32(0), Address(stackPointerRegister));

This ends up being a read followed by a write.  Are you doing this to avoid using another register?

> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:95
> +    Jump jmp = branch32(LessThan, temp, delta);
> +    sub32(delta, temp);
> +    sub32(delta, stackPointerRegister);
> +    // Dummy operation to make sure the system commits memory, and moves the guard page.
> +    sub32(TrustedImm32(0), Address(stackPointerRegister));
> +    jump(moveGuardPageLoop);

Use subPtr.

I'm not sure this is doing what you want.  Assuming that you want to enter the loop with temp pointing at the extent of the new allocation, the sub32(delta, temp) will reduce temp beyond the allocation.

I don't think you need to update the stack pointer.  You should be able to use temp for the dummy operation as well.

> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:97
> +    Label doneMovingGuardPage(this);
> +    jmp.linkTo(doneMovingGuardPage, this);

Replace this with a "jmp.link(this)".  Also, change "jmp" to "doneMovingGuardPage".

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:393
> +    const winStackGuardPageSize = 4096

Same comment as in AssemblyHelpers about using 4096 .

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:394
> +    const delta = winStackGuardPageSize - 16

Same comment as in AssemblyHelpers about subtracting the 16 from the page size.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:397
> +    subi reg, scratch
> +    addi sp, scratch

After this, I think scratch = frameSize - reg + sp.  Given that reg is either cfr or the location of a callee's cfr, you end up with scratch = frameSize - cfr + sp.  cfr-sp is the currentFrame's local register space.
It appears that we want scratch to have the delta at the top of the loop and then want to advance one page at a time doing a dummy op.  Therefore the subi/addi at the top of the loop is not really needed.

You're doing pointer arithmetic, so you need to use addp / subp

[Mark Lam]

Comment on attachment 225383 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225383&action=review

>> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:78
>> +#if PLATFORM(WIN)
>> +// On Windows we need to check if the stack pointer moves over the stack guard page of size 4K.
>> +// If so, we need to read or write some data on every following guard page
>> +// to make sure the system commits the previously uncommitted stack memory.
>> +// Otherwise we might crash later if we access uncommitted stack memory.
>> +// See http ://msdn.microsoft.com/en-us/library/ms686774%28VS.85%29.aspx
> 
> Remove the space after "http".
> 
> I didn't see anything on the MSDN web page that each intermediate page needs to be touched when the allocation grows by multiple pages.  Did you verify this through testing?

Like Michael, I also don’t see anything from the docs at the url you provided that says that application code needs to touch the memory first.  Also, what is the difference between touching it here in moveStackPointerAndGuardPage vs touching it later?  

The only time when I can see this making a difference is if system memory is low to begin with and you’re eagerly touching it to get the physical stack pages committed before some other consumer gets it first.  If that is the case, I’m not convinced that this is a good solution.  It would involve competing in a race condition (i.e. doesn’t guarantee that we’ll succeed in claiming those pages), and it simply shifts the problem of dealing with lack of memory to some other part of the code.

Can you please provide some details (and data to back it up) on what the actual problem is that you’re trying to solve here?  If I missed the relevant details in a previous comment, please highlight it for me.  Thanks.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:390
> +# On Windows we need to check if the stack pointer moves over the stack guard page of size 4K.
> +# If so, we need to read or write some data on every following guard page
> +# to make sure the system commits the previously uncommitted stack memory.
> +# Otherwise we might crash later if we access uncommitted stack memory.
> +# See http://msdn.microsoft.com/en-us/library/ms686774%28VS.85%29.aspx

Perhaps I’m missing it, but I don’t see anything from the docs at the url you provided that says that application code needs to touch the memory first.  Also, what is the difference between touching it here in moveStackPointerAndGuardPage vs touching it later?  

The only time when I can see this making a difference is if system memory is low to begin with and you’re eagerly touching it to get the physical stack pages committed before some other consumer gets it first.  If that is the case, I’m not convinced that this is a good solution.  It would involve competing in a race condition (i.e. doesn’t guarantee that we’ll succeed in claiming those pages), and it simply shifts the problem of dealing with lack of memory to some other part of the code.

Can you please provide some details (and data to back it up) on what the actual problem is that you’re trying to solve here?  If I missed the relevant details in a previous comment, please highlight it for me.  Thanks.

[Mark Lam]

(In reply to comment #9)

Sorry for the repeated comments.

[Geoffrey Garen]

Comment on attachment 225383 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225383&action=review

> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:93
> +    // Dummy operation to make sure the system commits memory, and moves the guard page.

I think this is the key misunderstanding in this patch.

What the Microsoft doc says is, "... the reserved size minus one page (which is used as a guard page to prevent stack overflow)". So, the guard page starts out at the end of the reserved space, and is not moved by incremental virtual memory commits.

Thanks for your comments :)

I have probably been doing some unnecessary work here.
As suggested, I created a test program in C with a simple function which had a
parameter of size > 4K.
The assembly then shows a call to _chkstk, which probably does the job needed.

See

http://msdn.microsoft.com/en-us/library/ms648426(VS.85).aspx 

or e.g.

http://www.codeguru.com/cpp/v-s/debug/article.php/c19241/Adventures-with-chkstk.htm

Maybe we should be using this function instead?

I also believe the guard page is put between committed and uncommitted memory,
see e.g. the comment in WebKit\Source\WTF\wtf\StackBounds.cpp in the Windows implementation.

[Michael Saboff]

(In reply to comment #12)
> Thanks for your comments :)
> 
> I have probably been doing some unnecessary work here.
> As suggested, I created a test program in C with a simple function which had a
> parameter of size > 4K.
> The assembly then shows a call to _chkstk, which probably does the job needed.
> 
> See
> 
> http://msdn.microsoft.com/en-us/library/ms648426(VS.85).aspx 
> 
> or e.g.
> 
> http://www.codeguru.com/cpp/v-s/debug/article.php/c19241/Adventures-with-chkstk.htm
> 
> Maybe we should be using this function instead?
> 
> I also believe the guard page is put between committed and uncommitted memory,
> see e.g. the comment in WebKit\Source\WTF\wtf\StackBounds.cpp in the Windows implementation.

There is other support on the Web for the notion that Windows moves the guard page - yuck!

Old Microsoft post: http://support.microsoft.com/kb/100775
Someone's reverse engineering: http://j00ru.vexillium.org/?p=1594

Here's the _chkstk implementation:

--- f:\dd\vctools\crt\crtw32\startup\i386\chkstk.asm ---------------------------
008A8980  push        ecx  
008A8981  lea         ecx,[esp+4]  
008A8985  sub         ecx,eax  
008A8987  sbb         eax,eax  
008A8989  not         eax  
008A898B  and         ecx,eax  
008A898D  mov         eax,esp  
008A898F  and         eax,0FFFFF000h  
008A8994  cmp         ecx,eax  
008A8996  jb          cs10+0Eh (08A89A2h)  
008A8998  mov         eax,ecx  
008A899A  pop         ecx  
008A899B  xchg        eax,esp  
008A899C  mov         eax,dword ptr [eax]  
008A899E  mov         dword ptr [esp],eax  
008A89A1  ret  
008A89A2  sub         eax,1000h  
008A89A7  test        dword ptr [eax],eax  
008A89A9  jmp         cs10 (08A8994h)

It seems like the test instruction is used to touch the guard page here.

[Geoffrey Garen]

Can we just call _chkstk on the stack check slow path on Windows?

(In reply to comment #16)
> Can we just call _chkstk on the stack check slow path on Windows?

I'm not sure, but isn't the stack check slow path called only if we have exceeded the stack bounds?
I don't think that is the case here, since the stack bounds include both the committed and uncommitted stack memory.

[Geoffrey Garen]

OK -- on Windows, stack bounds should not include uncommitted memory.

(In reply to comment #18)
> OK -- on Windows, stack bounds should not include uncommitted memory.

Thanks, I will try out calling _chkstk :)

Just included the _chkstk asm source for completeness sake :)

;***
;chkstk.asm - C stack checking routine
;
;       Copyright (c) Microsoft Corporation. All rights reserved.
;
;Purpose:
;       Provides support for automatic stack checking in C procedures
;       when stack checking is enabled.
;
;*******************************************************************************

.xlist
        include cruntime.inc
.list

; size of a page of memory

_PAGESIZE_      equ     1000h


        CODESEG

page
;***
;_chkstk - check stack upon procedure entry
;
;Purpose:
;       Provide stack checking on procedure entry. Method is to simply probe
;       each page of memory required for the stack in descending order. This
;       causes the necessary pages of memory to be allocated via the guard
;       page scheme, if possible. In the event of failure, the OS raises the
;       _XCPT_UNABLE_TO_GROW_STACK exception.
;
;       NOTE:  Currently, the (EAX < _PAGESIZE_) code path falls through
;       to the "lastpage" label of the (EAX >= _PAGESIZE_) code path.  This
;       is small; a minor speed optimization would be to special case
;       this up top.  This would avoid the painful save/restore of
;       ecx and would shorten the code path by 4-6 instructions.
;
;Entry:
;       EAX = size of local frame
;
;Exit:
;       ESP = new stackframe, if successful
;
;Uses:
;       EAX
;
;Exceptions:
;       _XCPT_GUARD_PAGE_VIOLATION - May be raised on a page probe. NEVER TRAP
;                                    THIS!!!! It is used by the OS to grow the
;                                    stack on demand.
;       _XCPT_UNABLE_TO_GROW_STACK - The stack cannot be grown. More precisely,
;                                    the attempt by the OS memory manager to
;                                    allocate another guard page in response
;                                    to a _XCPT_GUARD_PAGE_VIOLATION has
;                                    failed.
;
;*******************************************************************************

public  _alloca_probe

_chkstk proc

_alloca_probe    =  _chkstk

        push    ecx

; Calculate new TOS.

        lea     ecx, [esp] + 8 - 4      ; TOS before entering function + size for ret value
        sub     ecx, eax                ; new TOS

; Handle allocation size that results in wraparound.
; Wraparound will result in StackOverflow exception.

        sbb     eax, eax                ; 0 if CF==0, ~0 if CF==1
        not     eax                     ; ~0 if TOS did not wrapped around, 0 otherwise
        and     ecx, eax                ; set to 0 if wraparound

        mov     eax, esp                ; current TOS
        and     eax, not ( _PAGESIZE_ - 1) ; Round down to current page boundary

cs10:
        cmp     ecx, eax                ; Is new TOS
        jb      short cs20              ; in probed page?
        mov     eax, ecx                ; yes.
        pop     ecx
        xchg    esp, eax                ; update esp
        mov     eax, dword ptr [eax]    ; get return address
        mov     dword ptr [esp], eax    ; and put it at new TOS
        ret

; Find next lower page and probe
cs20:
        sub     eax, _PAGESIZE_         ; decrease by PAGESIZE
        test    dword ptr [eax],eax     ; probe page.
        jmp     short cs10

_chkstk endp

        end

Created attachment 225659 [details]
Patch

Created attachment 225664 [details]
Patch

(In reply to comment #22)
> Created an attachment (id=225664) [details]
> Patch

Updated the patch after review.

I've also tried using the _chkstk() function, which fixes the crash as well,
but it introduced some new stress test failures.
I think this is caused by _chkstk() saving and restoring a register it uses (see source in a previous comment).
I'm more comfortable with this routine not changing the contents of the stack.

[Geoffrey Garen]

Ah, I didn't realize at first that this patch was emitting moveStackPointerAndGuardPage at the head of every function. Let's not do that: it's slow and brittle.

Instead, I think we should pre-commit stack space on Windows. The right place to do that is probably inside VM::updateStackLimit.

Pre-committing will be faster, but it will also remove lots of tricky Windows-specific code, which is a worthy goal to pursue.

(In reply to comment #24)
> Ah, I didn't realize at first that this patch was emitting moveStackPointerAndGuardPage at the head of every function. Let's not do that: it's slow and brittle.
> 
> Instead, I think we should pre-commit stack space on Windows. The right place to do that is probably inside VM::updateStackLimit.
> 
> Pre-committing will be faster, but it will also remove lots of tricky Windows-specific code, which is a worthy goal to pursue.

Thanks, sounds good, will look into that :)

Created attachment 225791 [details]
Patch

[Geoffrey Garen]

Comment on attachment 225791 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225791&action=review

Looking good, but I think this needs one more pass.

> Source/JavaScriptCore/runtime/VM.cpp:763
> +// when needed, see http://support.microsoft.com/kb/100775

"." at the end of the sentence, please.

> Source/JavaScriptCore/runtime/VM.cpp:778
> +#if PLATFORM(WIN) && ENABLE(LLINT)

I don't think we want to check ENABLE(LLINT) here. For example, if you disabled LLInt and enabled JIT, you would still need preCommitStackMemory. So, let's just check PLATFORM(WIN).

> Source/JavaScriptCore/runtime/VM.cpp:803
> +#if PLATFORM(WIN) && ENABLE(LLINT)
> +    if (lastStackLimit != m_stackLimit)
> +        preCommitStackMemory(m_stackLimit);
> +#endif

Looks good.

> Source/WTF/wtf/WTFThreadData.h:99
>          // We need to always get a fresh StackBounds from the OS due to how fibers work.
>          // See https://bugs.webkit.org/show_bug.cgi?id=102411
> -#if OS(WINDOWS)
> +#if OS(WINDOWS) && !ENABLE(LLINT)
>          m_stackBounds = StackBounds::currentThreadStackBounds();
>  #endif
>          return m_stackBounds;

I don't think we want to check ENABLE(LLINT) here. A Windows client can use fibers regardless of LLInt.

[Mark Lam]

(In reply to comment #27)
> > Source/JavaScriptCore/runtime/VM.cpp:778
> > +#if PLATFORM(WIN) && ENABLE(LLINT)
> 
> I don't think we want to check ENABLE(LLINT) here. For example, if you disabled LLInt and enabled JIT, you would still need preCommitStackMemory. So, let's just check PLATFORM(WIN).

FYI, we already always require ENABLE(LLINT).  It's just a matter of ENABLE(JIT) or ENABLE(LLINT_C_LOOP) which is mutually exclusive.

Maybe the time has come for us to remove all references to ENABLE(LLINT) and ENABLE(LLINT_C_LOOP) (inferred by !ENABLE(JIT)) everywhere (but not for this patch).

Created attachment 225804 [details]
Patch

(In reply to comment #27)
> (From update of attachment 225791 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225791&action=review
> 

Thanks for reviewing :) Updated patch.

> > Source/WTF/wtf/WTFThreadData.h:99
> >          // We need to always get a fresh StackBounds from the OS due to how fibers work.
> >          // See https://bugs.webkit.org/show_bug.cgi?id=102411
> > -#if OS(WINDOWS)
> > +#if OS(WINDOWS) && !ENABLE(LLINT)
> >          m_stackBounds = StackBounds::currentThreadStackBounds();
> >  #endif
> >          return m_stackBounds;
> 
> I don't think we want to check ENABLE(LLINT) here. A Windows client can use fibers regardless of LLInt.

Can we avoid updating the stack bounds?
I reverted this in the latest patch, but I see now that it introduces more stress test errors.
Probably because it gives different results after stack memory is precommitted.

(In reply to comment #28)
> (In reply to comment #27)
> > > Source/JavaScriptCore/runtime/VM.cpp:778
> > > +#if PLATFORM(WIN) && ENABLE(LLINT)
> > 
> > I don't think we want to check ENABLE(LLINT) here. For example, if you disabled LLInt and enabled JIT, you would still need preCommitStackMemory. So, let's just check PLATFORM(WIN).
> 
> FYI, we already always require ENABLE(LLINT).  It's just a matter of ENABLE(JIT) or ENABLE(LLINT_C_LOOP) which is mutually exclusive.
> 
> Maybe the time has come for us to remove all references to ENABLE(LLINT) and ENABLE(LLINT_C_LOOP) (inferred by !ENABLE(JIT)) everywhere (but not for this patch).

I see, thanks for clarifying :)

[Geoffrey Garen]

> Can we avoid updating the stack bounds?

I don't think so :(. On Windows, a thread can switch to another fiber at any time.

> I reverted this in the latest patch, but I see now that it introduces more stress test errors.
> Probably because it gives different results after stack memory is precommitted.

What kind of errors?

(In reply to comment #32)
> > Can we avoid updating the stack bounds?
> 
> I don't think so :(. On Windows, a thread can switch to another fiber at any time.
> 
> > I reverted this in the latest patch, but I see now that it introduces more stress test errors.
> > Probably because it gives different results after stack memory is precommitted.
> 
> What kind of errors?

Seems like stack overflow errors.
I have debugged when we enter the StackBounds::initialize() after precommitting,
and the result is different compared to the initial call, because we have precommitted memory.
There is now committed memory before and after the guard page, which produces different bounds result.

(In reply to comment #32)
> > Can we avoid updating the stack bounds?
> 
> I don't think so :(. On Windows, a thread can switch to another fiber at any time.
> 
> > I reverted this in the latest patch, but I see now that it introduces more stress test errors.
> > Probably because it gives different results after stack memory is precommitted.
> 
> What kind of errors?

It seems we also have to move the guard page, not only commit the memory.
I will add code for that.

Created attachment 225818 [details]
Patch

(In reply to comment #35)
> Created an attachment (id=225818) [details]
> Patch

Added new patch which also moves the stack guard page when precommitting.
This fixes the new stack overflow errors in the stress tests I've been having.

[Geoffrey Garen]

Comment on attachment 225818 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=225818&action=review

> Source/JavaScriptCore/runtime/VM.cpp:790
> +    MEMORY_BASIC_INFORMATION uncommittedMemory;
> +    if (VirtualQuery(stackLimit, &uncommittedMemory, sizeof(uncommittedMemory)) > 0)
> +        if (uncommittedMemory.State == MEM_RESERVE) {
> +            int size = uncommittedMemory.RegionSize;
> +            // Commit memory
> +            OSAllocator::commit(stackLimit, size, true, false);
> +
> +            // Move guard page
> +            MEMORY_BASIC_INFORMATION guardPage;
> +            char* guardPagePointer = reinterpret_cast<char*>(stackLimit) + size;
> +            if (VirtualQuery(guardPagePointer, &guardPage, sizeof(guardPage)) > 0) {
> +                ASSERT(guardPage.Protect & PAGE_GUARD);
> +                DWORD oldProtect;
> +                // Remove guard flag from old guard page
> +                BOOL retval = VirtualProtect(guardPagePointer, guardPage.RegionSize, PAGE_READWRITE, &oldProtect);
> +                ASSERT(retval);
> +                char* newGuardPagePointer = reinterpret_cast<char*>(stackLimit) - guardPage.RegionSize;
> +                // Commit new guard page
> +                OSAllocator::commit(newGuardPagePointer, guardPage.RegionSize, true, false);
> +                // Set guard flag on new guard page
> +                retval = VirtualProtect(newGuardPagePointer, guardPage.RegionSize, PAGE_READWRITE | PAGE_GUARD, &oldProtect);
> +                ASSERT(retval);
> +            }
> +        }

Are we just duplicating _chckstck here? Should we just call _chckstck instead?

(In reply to comment #37)
> (From update of attachment 225818 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225818&action=review
> 
> > Source/JavaScriptCore/runtime/VM.cpp:790
> > +    MEMORY_BASIC_INFORMATION uncommittedMemory;
> > +    if (VirtualQuery(stackLimit, &uncommittedMemory, sizeof(uncommittedMemory)) > 0)
> > +        if (uncommittedMemory.State == MEM_RESERVE) {
> > +            int size = uncommittedMemory.RegionSize;
> > +            // Commit memory
> > +            OSAllocator::commit(stackLimit, size, true, false);
> > +
> > +            // Move guard page
> > +            MEMORY_BASIC_INFORMATION guardPage;
> > +            char* guardPagePointer = reinterpret_cast<char*>(stackLimit) + size;
> > +            if (VirtualQuery(guardPagePointer, &guardPage, sizeof(guardPage)) > 0) {
> > +                ASSERT(guardPage.Protect & PAGE_GUARD);
> > +                DWORD oldProtect;
> > +                // Remove guard flag from old guard page
> > +                BOOL retval = VirtualProtect(guardPagePointer, guardPage.RegionSize, PAGE_READWRITE, &oldProtect);
> > +                ASSERT(retval);
> > +                char* newGuardPagePointer = reinterpret_cast<char*>(stackLimit) - guardPage.RegionSize;
> > +                // Commit new guard page
> > +                OSAllocator::commit(newGuardPagePointer, guardPage.RegionSize, true, false);
> > +                // Set guard flag on new guard page
> > +                retval = VirtualProtect(newGuardPagePointer, guardPage.RegionSize, PAGE_READWRITE | PAGE_GUARD, &oldProtect);
> > +                ASSERT(retval);
> > +            }
> > +        }
> 
> Are we just duplicating _chckstck here? Should we just call _chckstck instead?

I think calling _chkstk() from C/C++ is a bit tricky, since it requires the stack allocation size in eax, and we can't use inline assembly because of 64-bit.
Also, _chkstk() moves the stack guard page, one page at a time, while this does it just once.

Created attachment 225899 [details]
Patch

(In reply to comment #39)
> Created an attachment (id=225899) [details]
> Patch

Updated patch with a platform independent way to precommit stack memory.

[Geoffrey Garen]

Comment on attachment 225899 [details]
Patch

r=me

(In reply to comment #41)
> (From update of attachment 225899 [details])
> r=me

Thanks :) Will have a look at 64-bit next.

[WebKit Commit Bot]

Comment on attachment 225899 [details]
Patch

Clearing flags on attachment: 225899

Committed r165128: <http://trac.webkit.org/changeset/165128>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Hahnenberg]

(In reply to comment #43)
> (From update of attachment 225899 [details])
> Clearing flags on attachment: 225899
> 
> Committed r165128: <http://trac.webkit.org/changeset/165128>

This patch disabled write barriers in Repatch.cpp for platforms with !ENABLE(DFG_JIT). That's clearly wrong.

[Mark Hahnenberg]

(In reply to comment #45)
> (In reply to comment #43)
> > (From update of attachment 225899 [details] [details])
> > Clearing flags on attachment: 225899
> > 
> > Committed r165128: <http://trac.webkit.org/changeset/165128>
> 
> This patch disabled write barriers in Repatch.cpp for platforms with !ENABLE(DFG_JIT). That's clearly wrong.

I take that back. It didn't disable them. It made them always take slow path. Which sort of defeats the purpose of inline caching, but it won't cause crashes. I filed bug 129760 for this.

[Brent Fulgham]

Please note that http://trac.webkit.org/changeset/165228 is also needed for this change.