[Webkit-unassigned] [Bug 129429] [Win32][LLINT] Crash when running JSC stress tests.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=129429


Michael Saboff <msaboff at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #225383|review?                     |review-
               Flag|                            |




--- Comment #8 from Michael Saboff <msaboff at apple.com>  2014-02-27 10:38:33 PST ---
(From update of attachment 225383)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.