131182 2014-04-03 14:03:25 -0700 Fix JSC Debug Regressions on Windows 2014-04-17 10:02:56 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 mark.lam mark.lam bfulgham commit-queue darin peavo webkit-bug-importer oldest_to_newest 997298 0 mark.lam 2014-04-03 14:03:25 -0700 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. 997299 1 mark.lam 2014-04-03 14:03:51 -0700 <rdar://problem/16505397> 998417 2 mark.lam 2014-04-07 21:34:27 -0700 This 7 JSC test failures started manifesting in r165128: "[Win32][LLINT] Crash when running JSC stress tests." 998450 3 mark.lam 2014-04-07 23:16:19 -0700 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. 998794 4 mark.lam 2014-04-08 14:41:18 -0700 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. 999397 5 mark.lam 2014-04-09 19:26:39 -0700 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. 1000874 6 peavo 2014-04-15 10:00:05 -0700 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. 1000875 7 229374 peavo 2014-04-15 10:01:07 -0700 Created attachment 229374 Patch 1001180 8 229374 darin 2014-04-16 10:11:28 -0700 Comment on attachment 229374 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. 1001288 9 229472 peavo 2014-04-16 13:03:58 -0700 Created attachment 229472 Patch 1001291 10 peavo 2014-04-16 13:09:26 -0700 (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. 1001299 11 bfulgham 2014-04-16 13:20:51 -0700 (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? 1001301 12 bfulgham 2014-04-16 13:22:25 -0700 (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); } 1001302 13 229472 bfulgham 2014-04-16 13:22:55 -0700 Comment on attachment 229472 Patch This looks good as-is. r=me. 1001304 14 peavo 2014-04-16 13:26:05 -0700 (In reply to comment #13) > (From update of attachment 229472 [details]) > This looks good as-is. r=me. Thanks for reviewing, guys :) 1001318 15 229472 commit-queue 2014-04-16 13:55:13 -0700 Comment on attachment 229472 Patch Clearing flags on attachment: 229472 Committed r167382: <http://trac.webkit.org/changeset/167382> 1001319 16 commit-queue 2014-04-16 13:55:17 -0700 All reviewed patches have been landed. Closing bug. 1001576 17 bfulgham 2014-04-17 09:27:16 -0700 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. 1001580 18 bfulgham 2014-04-17 09:36:26 -0700 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. 1001596 19 darin 2014-04-17 10:02:56 -0700 (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)". 229374 2014-04-15 10:01:07 -0700 2014-04-16 13:03:50 -0700 Patch bug-131182-20140415190039.patch text/plain 2348 peavo SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTY3MzA2KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIwIEBA CisyMDE0LTA0LTE1ICBwZWF2b0BvdXRsb29rLmNvbSAgPHBlYXZvQG91dGxvb2suY29tPgorCisg ICAgICAgIEZpeCBKU0MgRGVidWcgUmVncmVzc2lvbnMgb24gV2luZG93cworICAgICAgICBodHRw czovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTMxMTgyCisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhlIGNhc3Qgc3RhdGljX2Nhc3Q8 aW50NjRfdD4obnVtYmVyKSBpbiBKU1ZhbHVlOjppc01hY2hpbmVJbnQoKSBjYW4gZ2VuZXJhdGUg YSBmbG9hdGluZyBwb2ludCBlcnJvciwKKyAgICAgICAgYW5kIHNldCB0aGUgc3QgZmxvYXRpbmcg cG9pbnQgcmVnaXN0ZXIgdGFncywgaWYgdGhlIHZhbHVlIG9mIHRoZSBudW1iZXIgcGFyYW1ldGVy IGlzIGluZmluaXRlLgorICAgICAgICBJZiB0aGUgc3QgZmxvYXRpbmcgcG9pbnQgcmVnaXN0ZXIg dGFncyBhcmUgbm90IGNsZWFyZWQsIHRoaXMgY2FuIGNhdXNlIHN0cmFuZ2UgZmxvYXRpbmcgcG9p bnQgYmVoYXZpb3IgbGF0ZXIgb24uCisgICAgICAgIFRoaXMgY2FuIGJlIGF2b2lkZWQgYnkgY2hl Y2tpbmcgZm9yIGluZmluaXR5IGZpcnN0LgorCisgICAgICAgICogcnVudGltZS9KU0NKU1ZhbHVl SW5saW5lcy5oOgorICAgICAgICAoSlNDOjpKU1ZhbHVlOjppc01hY2hpbmVJbnQpOiBBdm9pZCBm bG9hdGluZyBwb2ludCBlcnJvciBieSBjaGVja2luZyBmb3IgaW5maW5pdHkgZmlyc3QuCisgICAg ICAgICogcnVudGltZS9PcHRpb25zLmNwcDoKKyAgICAgICAgKEpTQzo6cmVjb21wdXRlRGVwZW5k ZW50T3B0aW9ucyk6IFJlLWVuYWJsZSBqaXQgZm9yIFdpbmRvd3MuCisKIDIwMTQtMDQtMTQgIENv bW1pdCBRdWV1ZSAgPGNvbW1pdC1xdWV1ZUB3ZWJraXQub3JnPgogCiAgICAgICAgIFVucmV2aWV3 ZWQsIHJvbGxpbmcgb3V0IHIxNjcyNzIuCkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVu dGltZS9KU0NKU1ZhbHVlSW5saW5lcy5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0 Q29yZS9ydW50aW1lL0pTQ0pTVmFsdWVJbmxpbmVzLmgJKHJldmlzaW9uIDE2NzMwNikKKysrIFNv dXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTQ0pTVmFsdWVJbmxpbmVzLmgJKHdvcmtpbmcg Y29weSkKQEAgLTUwNCw2ICs1MDQsOCBAQCBpbmxpbmUgYm9vbCBKU1ZhbHVlOjppc01hY2hpbmVJ bnQoKSBjb25zCiAgICAgZG91YmxlIG51bWJlciA9IGFzRG91YmxlKCk7CiAgICAgaWYgKG51bWJl ciAhPSBudW1iZXIpCiAgICAgICAgIHJldHVybiBmYWxzZTsKKyAgICBpZiAoIXN0ZDo6aXNmaW5p dGUobnVtYmVyKSkKKyAgICAgICAgcmV0dXJuIGZhbHNlOwogICAgIGludDY0X3QgYXNJbnQ2NCA9 IHN0YXRpY19jYXN0PGludDY0X3Q+KG51bWJlcik7CiAgICAgaWYgKGFzSW50NjQgIT0gbnVtYmVy KQogICAgICAgICByZXR1cm4gZmFsc2U7CkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVu dGltZS9PcHRpb25zLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVu dGltZS9PcHRpb25zLmNwcAkocmV2aXNpb24gMTY3MzA2KQorKysgU291cmNlL0phdmFTY3JpcHRD b3JlL3J1bnRpbWUvT3B0aW9ucy5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTIyNiwxMSArMjI2LDYg QEAgc3RhdGljIHZvaWQgcmVjb21wdXRlRGVwZW5kZW50T3B0aW9ucygpCiAjaWYgIUVOQUJMRShG VExfSklUKQogICAgIE9wdGlvbnM6OnVzZUZUTEpJVCgpID0gZmFsc2U7CiAjZW5kaWYKLSNpZiBP UyhXSU5ET1dTKQotICAgIC8vIFRlbXBvcmFyaWx5IGRpc2FibGUgdGhlIEpJVCBvbiBXaW5kb3dz IHVudGlsIHdlIGhhdmUgYSBmaXggZm9yCi0gICAgLy8gaHR0cHM6Ly93ZWJraXQub3JnL2IvMTMx MTgyLgotICAgIE9wdGlvbnM6OnVzZUpJVCgpID0gZmFsc2U7Ci0jZW5kaWYKIAogICAgIGlmIChP cHRpb25zOjpzaG93RGlzYXNzZW1ibHkoKQogICAgICAgICB8fCBPcHRpb25zOjpzaG93REZHRGlz YXNzZW1ibHkoKQo= 229472 2014-04-16 13:03:58 -0700 2014-04-16 13:55:13 -0700 Patch bug-131182-20140416220327.patch text/plain 2522 peavo SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTY3Mzc4KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIwIEBA CisyMDE0LTA0LTE2ICBwZWF2b0BvdXRsb29rLmNvbSAgPHBlYXZvQG91dGxvb2suY29tPgorCisg ICAgICAgIEZpeCBKU0MgRGVidWcgUmVncmVzc2lvbnMgb24gV2luZG93cworICAgICAgICBodHRw czovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTMxMTgyCisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhlIGNhc3Qgc3RhdGljX2Nhc3Q8 aW50NjRfdD4obnVtYmVyKSBpbiBKU1ZhbHVlOjppc01hY2hpbmVJbnQoKSBjYW4gZ2VuZXJhdGUg YSBmbG9hdGluZyBwb2ludCBlcnJvciwKKyAgICAgICAgYW5kIHNldCB0aGUgc3QgZmxvYXRpbmcg cG9pbnQgcmVnaXN0ZXIgdGFncywgaWYgdGhlIHZhbHVlIG9mIHRoZSBudW1iZXIgcGFyYW1ldGVy IGlzIGluZmluaXRlLgorICAgICAgICBJZiB0aGUgc3QgZmxvYXRpbmcgcG9pbnQgcmVnaXN0ZXIg dGFncyBhcmUgbm90IGNsZWFyZWQsIHRoaXMgY2FuIGNhdXNlIHN0cmFuZ2UgZmxvYXRpbmcgcG9p bnQgYmVoYXZpb3IgbGF0ZXIgb24uCisgICAgICAgIFRoaXMgY2FuIGJlIGF2b2lkZWQgYnkgY2hl Y2tpbmcgZm9yIGluZmluaXR5IGZpcnN0LgorCisgICAgICAgICogcnVudGltZS9KU0NKU1ZhbHVl SW5saW5lcy5oOgorICAgICAgICAoSlNDOjpKU1ZhbHVlOjppc01hY2hpbmVJbnQpOiBBdm9pZCBm bG9hdGluZyBwb2ludCBlcnJvciBieSBjaGVja2luZyBmb3IgaW5maW5pdHkgZmlyc3QuCisgICAg ICAgICogcnVudGltZS9PcHRpb25zLmNwcDoKKyAgICAgICAgKEpTQzo6cmVjb21wdXRlRGVwZW5k ZW50T3B0aW9ucyk6IFJlLWVuYWJsZSBqaXQgZm9yIFdpbmRvd3MuCisKIDIwMTQtMDQtMTYgIE1h cmsgSGFobmVuYmVyZyAgPG1oYWhuZW5iZXJnQGFwcGxlLmNvbT4KIAogICAgICAgICBbV2ViS2l0 XSBDbGVhbnVwIHRoZSBidWlsZCBmcm9tIHVuaW5pdGlhbGl6ZWQgdmFyaWFibGUgaW4gSmF2YVNj cmlwdENvcmUKSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTQ0pTVmFsdWVJ bmxpbmVzLmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlND SlNWYWx1ZUlubGluZXMuaAkocmV2aXNpb24gMTY3MzQ2KQorKysgU291cmNlL0phdmFTY3JpcHRD b3JlL3J1bnRpbWUvSlNDSlNWYWx1ZUlubGluZXMuaAkod29ya2luZyBjb3B5KQpAQCAtNTA0LDYg KzUwNCwxMSBAQCBpbmxpbmUgYm9vbCBKU1ZhbHVlOjppc01hY2hpbmVJbnQoKSBjb25zCiAgICAg ZG91YmxlIG51bWJlciA9IGFzRG91YmxlKCk7CiAgICAgaWYgKG51bWJlciAhPSBudW1iZXIpCiAg ICAgICAgIHJldHVybiBmYWxzZTsKKyNpZiBPUyhXSU5ET1dTKQorICAgIC8vIE5lZWQgdG8gY2hl Y2sgZm9yIGluZmluaXR5IG9uIFdpbmRvd3MgdG8gYXZvaWQgZmxvYXRpbmcgcG9pbnQgZXJyb3Ig b24gZm9sbG93aW5nIGNhc3QsIHNlZSBidWcgMTMxMTgyLgorICAgIGlmIChzdGQ6OmlzaW5mKG51 bWJlcikpCisgICAgICAgIHJldHVybiBmYWxzZTsKKyNlbmRpZgogICAgIGludDY0X3QgYXNJbnQ2 NCA9IHN0YXRpY19jYXN0PGludDY0X3Q+KG51bWJlcik7CiAgICAgaWYgKGFzSW50NjQgIT0gbnVt YmVyKQogICAgICAgICByZXR1cm4gZmFsc2U7CkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUv cnVudGltZS9PcHRpb25zLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUv cnVudGltZS9PcHRpb25zLmNwcAkocmV2aXNpb24gMTY3MzQ2KQorKysgU291cmNlL0phdmFTY3Jp cHRDb3JlL3J1bnRpbWUvT3B0aW9ucy5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTIyNiwxMSArMjI2 LDYgQEAgc3RhdGljIHZvaWQgcmVjb21wdXRlRGVwZW5kZW50T3B0aW9ucygpCiAjaWYgIUVOQUJM RShGVExfSklUKQogICAgIE9wdGlvbnM6OnVzZUZUTEpJVCgpID0gZmFsc2U7CiAjZW5kaWYKLSNp ZiBPUyhXSU5ET1dTKQotICAgIC8vIFRlbXBvcmFyaWx5IGRpc2FibGUgdGhlIEpJVCBvbiBXaW5k b3dzIHVudGlsIHdlIGhhdmUgYSBmaXggZm9yCi0gICAgLy8gaHR0cHM6Ly93ZWJraXQub3JnL2Iv MTMxMTgyLgotICAgIE9wdGlvbnM6OnVzZUpJVCgpID0gZmFsc2U7Ci0jZW5kaWYKIAogICAgIGlm IChPcHRpb25zOjpzaG93RGlzYXNzZW1ibHkoKQogICAgICAgICB8fCBPcHRpb25zOjpzaG93REZH RGlzYXNzZW1ibHkoKQo=