Bug 131182 – Fix JSC Debug Regressions on Windows

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

Windows debug builders on ToT show failing JavaScriptCore tests.  As of r166738, I'm seeing the following test failures:

** The following Mozilla test failures have been introduced:
        ecma/Expressions/11.13.2-4.js
        ecma/Expressions/11.3.1.js
        ecma/Expressions/11.3.2.js
        ecma/Expressions/11.4.4.js
        ecma/Expressions/11.4.5.js
        ecma/Math/15.8.2.10.js
        ecma/Statements/12.10-1.js

Results for Mozilla tests:
    7 regressions found.
    0 tests fixed.

[Mark Lam]

<rdar://problem/16505397>

[Mark Lam]

This 7 JSC test failures started manifesting in r165128: "[Win32][LLINT] Crash when running JSC stress tests."

[Mark Lam]

Testing with ToT r166909, the 7 failures only manifests on a debug build.  They do not manifest on a release build.  The Apple Win 7 Release test bot confirms this.  For example, http://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/44267.

[Mark Lam]

If I disable the JIT using Options::useJIT() = false, the tests no longer fail.  So, this issue is not in the ASM LLINT, but in the baseline JIT.

[Mark Lam]

It looks like, with the JIT, we’re overflowing the floating point stack.  However, the source of the bug and a fix is not available yet.  We’ll temporarily disable the JIT in https://bugs.webkit.org/show_bug.cgi?id=131470 until we have a fix.

I've been debugging this one, and found that the failures are caused by floating point exceptions/errors.
The tests fail because the assertion ASSERT(multiplier >= 1.0) in ExecutionCounter::applyMemoryUsageHeuristics fails.
The value of multiplier int this case is actually infinite.
This should never happen as ExecutableAllocator::memoryPressureMultiplier should always return 1.0 on Windows.
The disassembly shows that the last statement in ExecutableAllocator::memoryPressureMultiplier is
translated into a floating point load instruction:

    return result;
        10183746  fld         qword ptr [result]  

This instruction fails to load the contents of result (1.0) into register st(0), because the st floating point register tags
have not been cleared from a previous floating point error (clearing can be achieved by calling ffree st(x)).
Further debugging shows that the source of the floating point error in this case is the statement

    int64_t asInt64 = static_cast<int64_t>(number);

in the method JSValue::isMachineInt(). This generates an error, and sets the st register tag, because the value of number is infinite.
A possible fix for this is to avoid the floating point error, by checking for infinity before performing this cast.

Created attachment 229374 [details]
Patch

[Darin Adler]

Comment on attachment 229374 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:508
>      if (number != number)
>          return false;
> +    if (!std::isfinite(number))
> +        return false;

Adding this extra check is going to make the code slower. Do we need it on all platforms, or just on Windows? Why isn’t this a problem on Mac? If we only need it on some platforms perhaps it should be conditional.

The line before this is a check for NaN; not sure why it’s using "x != x" rather than std::isnan, maybe performance reasons?

The std::isfinite also checks for NaN. We don’t need both. If we do need the check for infinity we should either use std::isinf or remove the NaN check.

Created attachment 229472 [details]
Patch

(In reply to comment #8)

Thanks for having a look at this :)

> (From update of attachment 229374 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229374&action=review
> 
> > Source/JavaScriptCore/runtime/JSCJSValueInlines.h:508
> >      if (number != number)
> >          return false;
> > +    if (!std::isfinite(number))
> > +        return false;
> 
> Adding this extra check is going to make the code slower. Do we need it on all platforms, or just on Windows? Why isn’t this a problem on Mac? If we only need it on some platforms perhaps it should be conditional.
> 
> The line before this is a check for NaN; not sure why it’s using "x != x" rather than std::isnan, maybe performance reasons?
> 
> The std::isfinite also checks for NaN. We don’t need both. If we do need the check for infinity we should either use std::isinf or remove the NaN check.

It only seems to be needed on Windows, other platforms does not have this problem, AFAIK.
Maybe gcc and MSVC behaves differently when it comes to the != operator and infinite numbers?
Or maybe this function is not called with an infinite parameter on Mac?

I made the test conditional as you suggested, to avoid performance penalty on other platforms.

[Brent Fulgham]

(In reply to comment #8)
> (From update of attachment 229374 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229374&action=review
> 
> > Source/JavaScriptCore/runtime/JSCJSValueInlines.h:508
> >      if (number != number)
> >          return false;
> > +    if (!std::isfinite(number))
> > +        return false;
> 
> Adding this extra check is going to make the code slower. Do we need it on all platforms, or just on Windows? Why isn’t this a problem on Mac? If we only need it on some platforms perhaps it should be conditional.
> 
> The line before this is a check for NaN; not sure why it’s using "x != x" rather than std::isnan, maybe performance reasons?
> 
> The std::isfinite also checks for NaN. We don’t need both. If we do need the check for infinity we should either use std::isinf or remove the NaN check.

darin: I just did a quick search, and we have lots of code like the following:

C:\Projects\WebKit\OpenSource\Source\WebCore\css\CSSCalculationValue.cpp(189):        if (std::isnan(value) || std::isinf(value))

Maybe this was not consistent in earlier C++ standards? Maybe we can clean up our code now?

[Brent Fulgham]

(In reply to comment #11)
> (In reply to comment #8)
> > (From update of attachment 229374 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=229374&action=review
> > 
> > > Source/JavaScriptCore/runtime/JSCJSValueInlines.h:508
> > >      if (number != number)
> > >          return false;
> > > +    if (!std::isfinite(number))
> > > +        return false;
> > 
> > Adding this extra check is going to make the code slower. Do we need it on all platforms, or just on Windows? Why isn’t this a problem on Mac? If we only need it on some platforms perhaps it should be conditional.
> > 
> > The line before this is a check for NaN; not sure why it’s using "x != x" rather than std::isnan, maybe performance reasons?
> > 
> > The std::isfinite also checks for NaN. We don’t need both. If we do need the check for infinity we should either use std::isinf or remove the NaN check.
> 
> darin: I just did a quick search, and we have lots of code like the following:
> 
> C:\Projects\WebKit\OpenSource\Source\WebCore\css\CSSCalculationValue.cpp(189):        if (std::isnan(value) || std::isinf(value))
> 
> Maybe this was not consistent in earlier C++ standards? Maybe we can clean up our code now?

Hmm. On Windows, it looks like 'isinf' does not check for NaN:

template<class _Ty> inline __nothrow bool isinf(_Ty _X)
{
    return (fpclassify(_X) == FP_INFINITE);
}

template<class _Ty> inline __nothrow bool isnan(_Ty _X)
{
    return (fpclassify(_X) == FP_NAN);
}

[Brent Fulgham]

Comment on attachment 229472 [details]
Patch

This looks good as-is. r=me.

(In reply to comment #13)
> (From update of attachment 229472 [details])
> This looks good as-is. r=me.

Thanks for reviewing, guys :)

[WebKit Commit Bot]

Comment on attachment 229472 [details]
Patch

Clearing flags on attachment: 229472

Committed r167382: <http://trac.webkit.org/changeset/167382>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

Note: I can confirm that with this patch in place, 32-bit Windows Debug runs properly:

Results for Mozilla tests:
    0 regression found.
    0 tests fixed.
    OK.

[Brent Fulgham]

It would be wonderful to understand why the behavior for this cast is so different on Mac and Windows. I did a bit of googling, but couldn't find much information, aside from some hand-waving about the IEEE standard leaving this behavior undefined.

[Darin Adler]

(In reply to comment #11)
> > The line before this is a check for NaN; not sure why it’s using "x != x" rather than std::isnan, maybe performance reasons?
> > 
> > The std::isfinite also checks for NaN. We don’t need both. If we do need the check for infinity we should either use std::isinf or remove the NaN check.
> 
> darin: I just did a quick search, and we have lots of code like the following:
> 
> C:\Projects\WebKit\OpenSource\Source\WebCore\css\CSSCalculationValue.cpp(189):        if (std::isnan(value) || std::isinf(value))
> 
> Maybe this was not consistent in earlier C++ standards? Maybe we can clean up our code now?

Just to be clear: isnan, isinf, and isfinite are three different functions.

And "std::isnan(x) || std::isinf(x)" is another way of writing "!std::isfinite(x)".