[webkit-reviews] review denied: [Bug 129429] [Win32][LLINT] Crash when running JSC stress tests. : [Attachment 225383] Patch

[bugzilla-daemon at webkit.org]

Michael Saboff <msaboff at apple.com> has denied peavo at outlook.com's request for
review:
Bug 129429: [Win32][LLINT] Crash when running JSC stress tests.
https://bugs.webkit.org/show_bug.cgi?id=129429

Attachment 225383: Patch
https://bugs.webkit.org/attachment.cgi?id=225383&action=review

------- Additional Comments from Michael Saboff <msaboff at apple.com>
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