put_by_val_direct need to check the property is index or not for using putDirect / putDirectIndex
Created attachment 244575 [details] Patch
Comment on attachment 244575 [details] Patch WIP
Created attachment 244577 [details] Patch
Comment on attachment 244577 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244577&action=review Added comments > Source/JavaScriptCore/dfg/DFGOperations.cpp:101 > + ASSERT_WITH_MESSAGE(index != PropertyName::NotAnIndex, "Since JSValue::isUInt32 returns true only when the boxed value is int32_t and positive, it doesn't return true for uint32_t max value that is PropertyName::NotAnIndex."); isUInt32 return true when the boxed value is int32_t and positive. So `index` is not PropertyName::NotAnIndex, that is UINT32_MAX. So, numbers larger than 0x7fffffff (INT32_MAX) are dropped into the isDouble path. > Source/JavaScriptCore/dfg/DFGOperations.cpp:109 > + if (propertyAsDouble == propertyAsUInt32 && propertyAsUInt32 != PropertyName::NotAnIndex) { When the double number is 0xffffffff that is UINT32_MAX, it is non-index, so we cannot use putDirectIndex. We fallback it to the toString and putDirect path. > Source/JavaScriptCore/dfg/DFGOperations.cpp:132 > + unsigned index = propertyName.asIndex(); If the string "0" (like "index" string) comes, we need to use putDirectIndex for such a indentifier. Therefore, we check it here.
Oops, I'll update the test.
Created attachment 244579 [details] Patch
Comment on attachment 244579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244579&action=review r- because I see at least one unfixed case in the code, and I think I see one missing test case. Please see my comment below for an alternative proposal for how to fix this bug. > Source/JavaScriptCore/dfg/DFGOperations.cpp:110 > - if (propertyAsDouble == propertyAsUInt32) { > + if (propertyAsDouble == propertyAsUInt32 && propertyAsUInt32 != PropertyName::NotAnIndex) { > putByVal<strict, direct>(exec, baseValue, propertyAsUInt32, value); The function signatures for putByVal, putByIndex, and so on accept unsigned as their input, and do not seem to forbid PropertyName::NotAnIndex (UINT_MAX) in any explicit way. However, I do see that the PropertyName class implicitly forbids UINT_MAX by using UINT_MAX as an in-band signal that a string could not convert to a number (because it was null or empty, or contained non-digit characters). There are lots of places in the code that call putByIndex, or that do numeric conversion by hand, and I'm afraid that there are too many to audit to make sure that none of them allow a UINT_MAX to slip through. For example, searching for 10 seconds found me NetscapePluginInstanceProxy::setProperty, which seems to allow UINT_MAX to slip through. So, I think the approach of using UINT_MAX as an in-band signal is a fragile design, and it's insufficient to patch it at these call sites in the DFG. I propose an alternative solution to these bugs: (1) Change PropertyName::asIndex() to return std::pair<uint32_t, bool>, with the bool indicating whether the integer is valid or not. (2) Change callers of PropertyName::asIndex() to check the bool instead of comparing against PropertyName::NotAnIndex. (3) Allow callers to pass UINT_MAX to putByIndex. What do you think? > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:846 > + // Don't put to an object if toString throws an exception. Does your test cover the case where toString throws an exception? I looked for this case and I didn't see it. Maybe I missed it?
Comment on attachment 244579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244579&action=review Thank you for your review! >> Source/JavaScriptCore/dfg/DFGOperations.cpp:110 >> putByVal<strict, direct>(exec, baseValue, propertyAsUInt32, value); > > The function signatures for putByVal, putByIndex, and so on accept unsigned as their input, and do not seem to forbid PropertyName::NotAnIndex (UINT_MAX) in any explicit way. > > However, I do see that the PropertyName class implicitly forbids UINT_MAX by using UINT_MAX as an in-band signal that a string could not convert to a number (because it was null or empty, or contained non-digit characters). > > There are lots of places in the code that call putByIndex, or that do numeric conversion by hand, and I'm afraid that there are too many to audit to make sure that none of them allow a UINT_MAX to slip through. For example, searching for 10 seconds found me NetscapePluginInstanceProxy::setProperty, which seems to allow UINT_MAX to slip through. > > So, I think the approach of using UINT_MAX as an in-band signal is a fragile design, and it's insufficient to patch it at these call sites in the DFG. > > I propose an alternative solution to these bugs: > > (1) Change PropertyName::asIndex() to return std::pair<uint32_t, bool>, with the bool indicating whether the integer is valid or not. > > (2) Change callers of PropertyName::asIndex() to check the bool instead of comparing against PropertyName::NotAnIndex. > > (3) Allow callers to pass UINT_MAX to putByIndex. > > What do you think? Your proposal sounds very nice. Specially handing UINT_MAX value as an exception is not good design. We should force it by type and compiler errors. However I have one issue about it. Currently, PropertyName classifies itself into 2, an index property and not-an-index property. The reason why we do may be that ECMAScript spec describes that the array index is 0 - 2^23 -1 range. (in sec 15.4) And some behavior of abstract operations in the spec becomes different. For example, Array.[[DefineOwnProperty]] recognizes the property is array index or not. And according to this information, the process of Array.[[DefineOwnProperty]] becomes different. (Expanding "length" property or not etc.) Based on this spec, JSC provides 2 functions for [[DefineOwnProperty]]. JSObject::defineOwnNonIndexProperty and JSObject::defineOwnIndexedProperty. And these implmenetations are quite different. Accepting UINT_MAX in all index operations requires all subsequent changes of Indexed/NonIndex operations, and it makes the implementation complicated since the term "index" becomes different from the spec. To solve this problem efficiently, I slightly modified your nice proposal, 1. Change PropertyName::asIndex() returns WTF::Optional<uint32_t> value. In this time, when the value is UINT_MAX, we makes it invalid optional value. 2. Change callers of PropertyName::asIndex() to check the bool instead of comparing against PropertyName::NotAnIndex. At this time, since we makes the UINT_MAX as invalid, it will be filtered here. 3. Allow callers to pass UINT_MAX to putByIndex (and other JSC_EXPORT_PRIVATE annotated APIs). It allows the user outside JSC to pass UINT_MAX as index. In the case of (3), the current implementation of putByIndex already accepts UINT_MAX as index while putDirectIndex / putDirect don't accept this. http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/runtime/JSObject.cpp#L415 I think this is good design since put*Direct* should not be called from the outside of the JSC. If the API is annotated with JS_EXPORT_PRIVATE, we should accept UINT_MAX as index like the current putByIndex do. What do you think of? >> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:846 >> + // Don't put to an object if toString throws an exception. > > Does your test cover the case where toString throws an exception? I looked for this case and I didn't see it. Maybe I missed it? Thank you! I'll add it to the test cases.
Comment on attachment 244579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244579&action=review >>> Source/JavaScriptCore/dfg/DFGOperations.cpp:110 >>> putByVal<strict, direct>(exec, baseValue, propertyAsUInt32, value); >> >> The function signatures for putByVal, putByIndex, and so on accept unsigned as their input, and do not seem to forbid PropertyName::NotAnIndex (UINT_MAX) in any explicit way. >> >> However, I do see that the PropertyName class implicitly forbids UINT_MAX by using UINT_MAX as an in-band signal that a string could not convert to a number (because it was null or empty, or contained non-digit characters). >> >> There are lots of places in the code that call putByIndex, or that do numeric conversion by hand, and I'm afraid that there are too many to audit to make sure that none of them allow a UINT_MAX to slip through. For example, searching for 10 seconds found me NetscapePluginInstanceProxy::setProperty, which seems to allow UINT_MAX to slip through. >> >> So, I think the approach of using UINT_MAX as an in-band signal is a fragile design, and it's insufficient to patch it at these call sites in the DFG. >> >> I propose an alternative solution to these bugs: >> >> (1) Change PropertyName::asIndex() to return std::pair<uint32_t, bool>, with the bool indicating whether the integer is valid or not. >> >> (2) Change callers of PropertyName::asIndex() to check the bool instead of comparing against PropertyName::NotAnIndex. >> >> (3) Allow callers to pass UINT_MAX to putByIndex. >> >> What do you think? > > Your proposal sounds very nice. Specially handing UINT_MAX value as an exception is not good design. We should force it by type and compiler errors. > > However I have one issue about it. > Currently, PropertyName classifies itself into 2, an index property and not-an-index property. > The reason why we do may be that ECMAScript spec describes that the array index is 0 - 2^23 -1 range. (in sec 15.4) > And some behavior of abstract operations in the spec becomes different. > For example, Array.[[DefineOwnProperty]] recognizes the property is array index or not. > And according to this information, the process of Array.[[DefineOwnProperty]] becomes different. (Expanding "length" property or not etc.) > > Based on this spec, JSC provides 2 functions for [[DefineOwnProperty]]. JSObject::defineOwnNonIndexProperty and JSObject::defineOwnIndexedProperty. > And these implmenetations are quite different. > Accepting UINT_MAX in all index operations requires all subsequent changes of Indexed/NonIndex operations, and it makes the implementation complicated since the term "index" becomes different from the spec. > > To solve this problem efficiently, I slightly modified your nice proposal, > > 1. Change PropertyName::asIndex() returns WTF::Optional<uint32_t> value. In this time, when the value is UINT_MAX, we makes it invalid optional value. > 2. Change callers of PropertyName::asIndex() to check the bool instead of comparing against PropertyName::NotAnIndex. At this time, since we makes the UINT_MAX as invalid, it will be filtered here. > 3. Allow callers to pass UINT_MAX to putByIndex (and other JSC_EXPORT_PRIVATE annotated APIs). It allows the user outside JSC to pass UINT_MAX as index. > > In the case of (3), the current implementation of putByIndex already accepts UINT_MAX as index while putDirectIndex / putDirect don't accept this. > http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/runtime/JSObject.cpp#L415 > I think this is good design since put*Direct* should not be called from the outside of the JSC. > If the API is annotated with JS_EXPORT_PRIVATE, we should accept UINT_MAX as index like the current putByIndex do. > What do you think of? And another proposal (and maybe it's orthogonal to the previous our proposal) is introducing ArrayIndex class instead of using unsigned as array index. And provides a factory function like, Optional<ArrayIndex> ArrayIndex::from(unsigned maybeIndex) { if (maybeIndex == UINT32_MAX) return Nullopt; return ArrayIndex(maybeIndex); // private constructor } It forces the provided value is ArrayIndex by type checker. What do you think of?
Created attachment 244762 [details] Patch
Comment on attachment 244762 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244762&action=review Updated the patch. I changed Identifier::asIndex to return Optional<uint32_t>. However, I don't apply my proposal that changing unsigned index parameter to ArrayIndex type. So this patch doesn't handle the case that the caller calls very low-level API (putDirect, defineOwnIndexedProperty etc.) with unsigned arguments. If it's prefereable, I'll change it so :) > Source/JavaScriptCore/runtime/PropertyName.h:70 > + if (value == UINT_MAX) When the value is UINT_MAX, we make it Nullopt since it's not an index. > LayoutTests/js/dfg-put-by-val-direct-with-edge-numbers-expected.txt:56 > +PASS lookupWithKey2(toStringThrowsError) threw exception 'Error: ng' on all iterations including after DFG tier-up. Introduce the test that a key throws an error. > LayoutTests/resources/js-test-pre.js:278 > +function dfgShouldThrow(theFunction, _a, _e) Introduce new test helper dfgShouldThrow.
Comment on attachment 244762 [details] Patch Attachment 244762 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5356741017468928 New failing tests: fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html js/dom/dom-as-prototype-assignment-exception.html js/dom/dom-attributes-on-mismatch-type.html js/dom/shadow-navigator-geolocation-in-strict-mode-does-not-throw.html
Created attachment 244763 [details] Archive of layout-test-results from ews102 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 244762 [details] Patch Attachment 244762 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6635351707746304 New failing tests: fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html js/dom/dom-as-prototype-assignment-exception.html js/dom/dom-attributes-on-mismatch-type.html js/dom/shadow-navigator-geolocation-in-strict-mode-does-not-throw.html
Created attachment 244764 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 244762 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244762&action=review This approach looks good to me. Any insight into the failing tests on the EWS bots? I have to say r- because of the test failures. > Source/JavaScriptCore/dfg/DFGOperations.cpp:133 > + Optional<uint32_t> optionalIndex = propertyName.asIndex(); > + if (!optionalIndex) I would just call this "index". Also, in simple cases like this, I like to combine lines: if (Optional<uint32_t> index = propertyName.asIndex()) // putDirectIndex else // putDirect > Source/JavaScriptCore/jit/JITOperations.cpp:509 > + // Don't put to an object if toString throws an exception. > + Identifier property = subscript.toString(callFrame)->toIdentifier(callFrame); > + if (!callFrame->vm().exception()) { This is a good place to use early return in the case of exception -- that way, the resulting code uses less indentation. > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:848 > + // Don't put to an object if toString throws an exception. > + Identifier property = subscript.toString(exec)->toIdentifier(exec); > + if (!exec->vm().exception()) { Early return. > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:851 > + Optional<uint32_t> optionalIndex = propertyName.asIndex(); > + if (!optionalIndex) { Combine. > Source/JavaScriptCore/runtime/Arguments.cpp:165 > + Optional<uint32_t> optionalIndex = propertyName.asIndex(); > + if (optionalIndex) { Combine. > Source/JavaScriptCore/runtime/JSArray.cpp:162 > + Optional<uint32_t> optionalIndex = propertyName.asIndex(); > + if (optionalIndex) { Combine. > Source/JavaScriptCore/runtime/JSCJSValue.cpp:123 > + Optional<uint32_t> optionalIndex = propertyName.asIndex(); > + if (optionalIndex) { Combine. > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:328 > + Optional<uint32_t> optionalIndex = propertyName.asIndex(); > + if (optionalIndex) { Combine. > Source/JavaScriptCore/runtime/JSObject.cpp:343 > + Optional<uint32_t> optionalIndex = propertyName.asIndex(); > + if (optionalIndex) { Combine. > Source/JavaScriptCore/runtime/JSObject.cpp:1261 > + Optional<uint32_t> optionalIndex = propertyName.asIndex(); > + if (optionalIndex) Combine. > Source/JavaScriptCore/runtime/JSObject.cpp:2663 > + Optional<uint32_t> optionalIndex = propertyName.asIndex(); > + if (optionalIndex) { Combine. > Source/JavaScriptCore/runtime/JSObject.h:1248 > + Optional<uint32_t> optionalIndex = propertyName.asIndex(); > + if (optionalIndex) Combine. > Source/JavaScriptCore/runtime/JSObject.h:1278 > + Optional<uint32_t> optionalIndex = propertyName.asIndex(); > + if (optionalIndex) Combine. > Source/JavaScriptCore/runtime/LiteralParser.cpp:653 > + Optional<uint32_t> optionalIndex = ident.asIndex(); > + if (optionalIndex) Combine. > Source/JavaScriptCore/runtime/Structure.cpp:1014 > + Optional<uint32_t> optionalIndex = propertyName.asIndex(); > + if (optionalIndex) Combine. > Source/WebCore/bindings/js/JSHTMLAllCollectionCustom.cpp:69 > + Optional<uint32_t> optionalIndex = toUInt32FromStringImpl(string.impl()); > + if (optionalIndex) Combine. > Source/WebCore/bindings/js/JSHTMLAllCollectionCustom.cpp:79 > + Optional<uint32_t> optionalIndex = toUInt32FromStringImpl(exec->argument(1).toWTFString(exec).impl()); > + if (optionalIndex) { Combine. > Source/WebCore/bindings/scripts/test/JS/JSFloat64Array.cpp:219 > + Optional<uint32_t> optionalIndex = propertyName.asIndex(); > + if (optionalIndex) { Combine. > Source/WebCore/bridge/runtime_array.cpp:123 > + Optional<uint32_t> optionalIndex = propertyName.asIndex(); > + if (optionalIndex) { Combine.
Comment on attachment 244762 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244762&action=review Thank you for your review :) I'll upload the revised patch. In my main development environment (Linux WebKitGTK port), there's no failures. So later, I'll check it on my OSX machine. >> Source/JavaScriptCore/dfg/DFGOperations.cpp:133 >> + if (!optionalIndex) > > I would just call this "index". > > Also, in simple cases like this, I like to combine lines: > > if (Optional<uint32_t> index = propertyName.asIndex()) > // putDirectIndex > else > // putDirect That looks nice! I'll change to that. >> Source/JavaScriptCore/jit/JITOperations.cpp:509 >> + if (!callFrame->vm().exception()) { > > This is a good place to use early return in the case of exception -- that way, the resulting code uses less indentation. Nice. I've fixed. >> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:848 >> + if (!exec->vm().exception()) { > > Early return. Done >> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:851 >> + if (!optionalIndex) { > > Combine. Done >> Source/JavaScriptCore/runtime/Arguments.cpp:165 >> + if (optionalIndex) { > > Combine. Done >> Source/JavaScriptCore/runtime/JSArray.cpp:162 >> + if (optionalIndex) { > > Combine. Done >> Source/JavaScriptCore/runtime/JSCJSValue.cpp:123 >> + if (optionalIndex) { > > Combine. Done >> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:328 >> + if (optionalIndex) { > > Combine. Done >> Source/JavaScriptCore/runtime/JSObject.cpp:343 >> + if (optionalIndex) { > > Combine. Done >> Source/JavaScriptCore/runtime/JSObject.cpp:1261 >> + if (optionalIndex) > > Combine. Done >> Source/JavaScriptCore/runtime/JSObject.cpp:2663 >> + if (optionalIndex) { > > Combine. Done >> Source/JavaScriptCore/runtime/JSObject.h:1248 >> + if (optionalIndex) > > Combine. Done >> Source/JavaScriptCore/runtime/JSObject.h:1278 >> + if (optionalIndex) > > Combine. Done >> Source/JavaScriptCore/runtime/LiteralParser.cpp:653 >> + if (optionalIndex) > > Combine. Done >> Source/JavaScriptCore/runtime/Structure.cpp:1014 >> + if (optionalIndex) > > Combine. Done >> Source/WebCore/bindings/js/JSHTMLAllCollectionCustom.cpp:69 >> + if (optionalIndex) > > Combine. Done >> Source/WebCore/bindings/js/JSHTMLAllCollectionCustom.cpp:79 >> + if (optionalIndex) { > > Combine. Done >> Source/WebCore/bindings/scripts/test/JS/JSFloat64Array.cpp:219 >> + if (optionalIndex) { > > Combine. Done >> Source/WebCore/bridge/runtime_array.cpp:123 >> + if (optionalIndex) { > > Combine. Done
Created attachment 244954 [details] Patch
Created attachment 244957 [details] Patch
Ah, seeing the test failures, I've found that the expectation file includes line number of CONSOLE MESSAGE. Maybe, adding dfgShouldThrow into js-test-pre.js causes this.
Comment on attachment 244957 [details] Patch Attachment 244957 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6196256799981568 New failing tests: fast/xmlhttprequest/xmlhttprequest-responsetype-sync-request.html js/dom/dom-as-prototype-assignment-exception.html js/dom/dom-attributes-on-mismatch-type.html js/dom/shadow-navigator-geolocation-in-strict-mode-does-not-throw.html
Created attachment 244962 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 244971 [details] Patch
Comment on attachment 244971 [details] Patch r=me
Comment on attachment 244971 [details] Patch Clearing flags on attachment: 244971 Committed r178751: <http://trac.webkit.org/changeset/178751>
All reviewed patches have been landed. Closing bug.
Looks like this may have caused test failures on the Apple Mavericks 32-bit JSC test bot: https://build.webkit.org/builders/Apple%20Mavericks%2032-bit%20JSC%20%28BuildAndTest%29/builds/6876 https://build.webkit.org/builders/Apple%20Mavericks%2032-bit%20JSC%20%28BuildAndTest%29/builds/6876/steps/webkit-32bit-jsc-test/logs/stdio > ** The following JSC stress test failures have been introduced: > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-dfg-eager-no-cjit > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-eager-no-cjit > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-no-cjit > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-no-cjit > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-no-llint Errors looking like: > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: PASS lookupWithKey("-4.2") is 42 on all iterations including after DFG tier-up. > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: Exception: ReferenceError: Can't find variable: dfgShouldThrow > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: global code@dfg-put-by-val-direct-with-edge-numbers.js:86:15 > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: PASS successfullyParsed is true Should we roll out this change, or somehow skip the test?
(In reply to comment #27) > Looks like this may have caused test failures on the Apple Mavericks 32-bit > JSC test bot: > https://build.webkit.org/builders/Apple%20Mavericks%2032- > bit%20JSC%20%28BuildAndTest%29/builds/6876 > https://build.webkit.org/builders/Apple%20Mavericks%2032- > bit%20JSC%20%28BuildAndTest%29/builds/6876/steps/webkit-32bit-jsc-test/logs/ > stdio > > > ** The following JSC stress test failures have been introduced: > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-dfg-eager-no-cjit > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-eager-no-cjit > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-no-cjit > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-no-cjit > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-no-llint > > Errors looking like: > > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: PASS lookupWithKey("-4.2") is 42 on all iterations including after DFG tier-up. > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: Exception: ReferenceError: Can't find variable: dfgShouldThrow > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: global code@dfg-put-by-val-direct-with-edge-numbers.js:86:15 > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: PASS successfullyParsed is true > > Should we roll out this change, or somehow skip the test? Thank you for informing us!! Hmm, "Can't find variable: dfgShouldThrow" is very curious behavior because, (1): dfgShouldThrow is actually defined in js-test-pre.js. (2): dfgShouldBe works before raising this error. (3): dfgShouldBe uses dfgCompiled, this is defined after dfgShouldThrow in js-test-pre.js. Geoffrey, what do you think of?
Comment on attachment 244971 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244971&action=review > LayoutTests/resources/js-test-pre.js:623 > + debug("WARN: dfgShouldBe() expects a function and two strings"); Nit: This looks like a copy/paste error, this should be "dfgShouldThrow()" in the error message.
> Geoffrey, what do you think of? Strange. I don't know what's going on here. Can you reproduce this failure by running in 32-bit mode?
(In reply to comment #30) > > Geoffrey, what do you think of? > > Strange. I don't know what's going on here. > > Can you reproduce this failure by running in 32-bit mode? I agree, strange. However, I will start a rollout, since the bots are continuing to fail on these tests.
Re-opened since this is blocked by bug 140694
(In reply to comment #29) > Comment on attachment 244971 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=244971&action=review > > > LayoutTests/resources/js-test-pre.js:623 > > + debug("WARN: dfgShouldBe() expects a function and two strings"); > > Nit: This looks like a copy/paste error, this should be "dfgShouldThrow()" > in the error message. Thanks! I'll fix it at the same time.
(In reply to comment #30) > > Geoffrey, what do you think of? > > Strange. I don't know what's going on here. > > Can you reproduce this failure by running in 32-bit mode? OK. I'll build it with OSX in 32-bit mode and attempt to reproduce it :)
(In reply to comment #28) > (In reply to comment #27) > > Looks like this may have caused test failures on the Apple Mavericks 32-bit > > JSC test bot: > > https://build.webkit.org/builders/Apple%20Mavericks%2032- > > bit%20JSC%20%28BuildAndTest%29/builds/6876 > > https://build.webkit.org/builders/Apple%20Mavericks%2032- > > bit%20JSC%20%28BuildAndTest%29/builds/6876/steps/webkit-32bit-jsc-test/logs/ > > stdio > > > > > ** The following JSC stress test failures have been introduced: > > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout > > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-dfg-eager-no-cjit > > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl > > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-eager-no-cjit > > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-no-cjit > > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-no-cjit > > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-no-llint > > > > Errors looking like: > > > > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: PASS lookupWithKey("-4.2") is 42 on all iterations including after DFG tier-up. > > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: Exception: ReferenceError: Can't find variable: dfgShouldThrow > > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: global code@dfg-put-by-val-direct-with-edge-numbers.js:86:15 > > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout: PASS successfullyParsed is true > > > > Should we roll out this change, or somehow skip the test? > > Thank you for informing us!! > > Hmm, "Can't find variable: dfgShouldThrow" is very curious behavior because, > > (1): dfgShouldThrow is actually defined in js-test-pre.js. > (2): dfgShouldBe works before raising this error. > (3): dfgShouldBe uses dfgCompiled, this is defined after dfgShouldThrow in > js-test-pre.js. > > Geoffrey, what do you think of? Wait!! There are multiple js-test-pre.js. I suspect you might need to update at least one other to get your change. Maybe the http one? ./LayoutTests/http/tests/resources/js-test-pre.js ./LayoutTests/http/tests/webgl/1.0.2/resources/webgl_test_files/resources/js-test-pre.js ./LayoutTests/js/mozilla/resources/js-test-pre.js ./LayoutTests/resources/js-test-pre.js
> There are multiple js-test-pre.js. I suspect you might need to update at > least one other to get your change. Maybe the http one? Hmm, there are a bunch of differences between them, and the other dfg functions are not even available in the http one. So this is probably not it =(.
I've built WebKit with $ Tools/Scripts/build-webkit --debug --32-bit on OSX Yosemite x86_64 environment. And run LayoutTests with $ Tools/Scripts/run-webkit-tests --debug --32-bit -f 'js/dfg-put-by-val-direct-with-edge-numbers.html' But the test has passed in my environment. Is it a correct way to build & test 32-bit mode WebKit?
I think to reproduce what the bot did, you need to use run-esc-stress-tests or run-javascriptcore-tests.
(In reply to comment #38) > I think to reproduce what the bot did, you need to use run-esc-stress-tests > or run-javascriptcore-tests. Thank you. I've reproduced that.
(In reply to comment #38) > I think to reproduce what the bot did, you need to use run-esc-stress-tests > or run-javascriptcore-tests. When moving dfgShouldThrow function into dfg-put-by-val-direct-with-edge-numbers.js itself, I've found that stress tests pass. Geoffrey, what do you think about this solution? Yes, possibly, it's not fundamental solution for this problem. Now I guess that there may be some issues in lookupping variables from GlobalObject's segmeted vector and it may cause this problem.
Created attachment 245047 [details] Patch
Comment on attachment 245047 [details] Patch r=me
> When moving dfgShouldThrow function into > dfg-put-by-val-direct-with-edge-numbers.js itself, I've found that stress > tests pass. > Geoffrey, what do you think about this solution? I think this is an OK work-around in the short term. r=me > Yes, possibly, it's not fundamental solution for this problem. > Now I guess that there may be some issues in lookupping variables from > GlobalObject's segmeted vector and it may cause this problem. I think this is worth investigating in a follow-up bug. This behavior is very strange!
Comment on attachment 245047 [details] Patch Clearing flags on attachment: 245047 Committed r178894: <http://trac.webkit.org/changeset/178894>
This broke JSC tests, I'm going to roll out. https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20%28Tests%29/builds/2207/steps/jscore-test/logs/stdio jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-eager-no-cjit: Timed out after 339.000000 seconds! jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-eager-no-cjit: 1 0x1046ce7d3 WTF::threadEntryPoint(void*) jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-eager-no-cjit: 2 0x1046cecbf WTF::wtfThreadEntryPoint(void*) jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-eager-no-cjit: 3 0x7fff8fb492fc _pthread_body jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-eager-no-cjit: 4 0x7fff8fb49279 _pthread_body jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-eager-no-cjit: 5 0x7fff8fb474b1 thread_start jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-eager-no-cjit: test_script_14898: line 2: 24711 Segmentation fault: 11 "$@" ../../../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --enableFunctionDotArguments\=true --testTheFTL\=true --useFTLJIT\=true --enableConcurrentJIT\=false --thresholdForJITAfterWarmUp\=100 --thresholdForJITAfterWarmUp\=10 --thresholdForJITSoon\=10 --thresholdForOptimizeAfterWarmUp\=20 --thresholdForOptimizeAfterLongWarmUp\=20 --thresholdForOptimizeSoon\=20 --thresholdForFTLOptimizeAfterWarmUp\=20 --thresholdForFTLOptimizeSoon\=20 resources/standalone-pre.js dfg-put-by-val-direct-with-edge-numbers.js resources/standalone-post.js jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge-numbers.js.layout-ftl-eager-no-cjit: ERROR: Unexpected exit code: 139
Bindings tests are also broken, although those probably just need an update. https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20(Tests)/builds/2207/steps/bindings-generation-tests/logs/stdio
Re-opened since this is blocked by bug 140775
(In reply to comment #46) > This broke JSC tests, I'm going to roll out. > > https://build.webkit.org/builders/ > Apple%20Yosemite%20Release%20WK2%20%28Tests%29/builds/2207/steps/jscore-test/ > logs/stdio > > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge- > numbers.js.layout-ftl-eager-no-cjit: Timed out after 339.000000 seconds! > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge- > numbers.js.layout-ftl-eager-no-cjit: 1 0x1046ce7d3 > WTF::threadEntryPoint(void*) > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge- > numbers.js.layout-ftl-eager-no-cjit: 2 0x1046cecbf > WTF::wtfThreadEntryPoint(void*) > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge- > numbers.js.layout-ftl-eager-no-cjit: 3 0x7fff8fb492fc _pthread_body > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge- > numbers.js.layout-ftl-eager-no-cjit: 4 0x7fff8fb49279 _pthread_body > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge- > numbers.js.layout-ftl-eager-no-cjit: 5 0x7fff8fb474b1 thread_start > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge- > numbers.js.layout-ftl-eager-no-cjit: test_script_14898: line 2: 24711 > Segmentation fault: 11 "$@" > ../../../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false > --enableFunctionDotArguments\=true --testTheFTL\=true --useFTLJIT\=true > --enableConcurrentJIT\=false --thresholdForJITAfterWarmUp\=100 > --thresholdForJITAfterWarmUp\=10 --thresholdForJITSoon\=10 > --thresholdForOptimizeAfterWarmUp\=20 > --thresholdForOptimizeAfterLongWarmUp\=20 --thresholdForOptimizeSoon\=20 > --thresholdForFTLOptimizeAfterWarmUp\=20 --thresholdForFTLOptimizeSoon\=20 > resources/standalone-pre.js dfg-put-by-val-direct-with-edge-numbers.js > resources/standalone-post.js > jsc-layout-tests.yaml/js/script-tests/dfg-put-by-val-direct-with-edge- > numbers.js.layout-ftl-eager-no-cjit: ERROR: Unexpected exit code: 139 Thank you! I'll investigate it... And I'll update the patch to fix for binding tests.
In http://trac.webkit.org/changeset/182406, refactoring part (changing uint32_t => Option<uint32_t>) was landed. So I'll clean up the patch to make it smaller. And investigate the issue!
Created attachment 250218 [details] Patch
Created attachment 250220 [details] Patch
Comment on attachment 250220 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250220&action=review Attempt to re-land this patch! > Source/JavaScriptCore/ChangeLog:10 > + This patch checks toString-ed Identifier is index or not to choose putDirect / putDirectIndex. Now, Optional<uint32_t> patch is splitted and landed. So the patch for this issue becomes cleaner. > Source/JavaScriptCore/ChangeLog:23 > + (toStringThrowsError.toString): Move LayoutTests provided in the previous patch to stress tests. > Source/JavaScriptCore/tests/stress/dfg-put-by-val-direct-with-edge-numbers.js:9 > +noInline(lookupWithKey); I think this is missing in the previous patch (and causes timeout). > Source/JavaScriptCore/tests/stress/dfg-put-by-val-direct-with-edge-numbers.js:83 > +noInline(lookupWithKey2); Ditto.
Comment on attachment 250220 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250220&action=review > Source/JavaScriptCore/dfg/DFGOperations.cpp:105 > - putByVal<strict, direct>(exec, baseValue, property.asUInt32(), value); > + uint32_t index = property.asUInt32(); > + ASSERT_WITH_MESSAGE(isIndex(index), "Since JSValue::isUInt32 returns true only when the boxed value is int32_t and positive, it doesn't return true for uint32_t max value (0xFFFFFFFF) that is not array index."); > + putByVal<strict, direct>(exec, baseValue, index, value); The message here might be a good comment to have explaining this assert, but it doesn’t seem helpful as an assertion message. I would write this assertion: // Despite its name, JSValue::isUInt32 will return true only for positive boxed int32_t; all those values are valid array indices. ASSERT(isIndex(property.asUInt32())); And I would leave the actual putByVal line alone; I don’t think we should put the thing into a local variable just to share code between the real code and the assertion. > Source/JavaScriptCore/jit/JITOperations.cpp:505 > + uint32_t index = subscript.asUInt32(); > + ASSERT_WITH_MESSAGE(isIndex(index), "Since JSValue::isUInt32 returns true only when the boxed value is int32_t and positive, it doesn't return true for uint32_t max value (0xFFFFFFFF) that is not array index."); > + baseObject->putDirectIndex(callFrame, index, value, 0, isStrictMode ? PutDirectIndexShouldThrow : PutDirectIndexShouldNotThrow); Same comment here as above. Code should just be: baseObject->putDirectIndex(callFrame, subscript.asUInt32(), value, 0, isStrictMode ? PutDirectIndexShouldThrow : PutDirectIndexShouldNotThrow); > Source/JavaScriptCore/jit/JITOperations.cpp:511 > + double subscriptAsDouble = subscript.asDouble(); > + uint32_t subscriptAsUInt32 = static_cast<uint32_t>(subscriptAsDouble); I’m surprised this is really the code we want. Does this handle values such as 2^32 properly? Do we have test coverage for this? > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:802 > + ASSERT_WITH_MESSAGE(isIndex(index), "Since JSValue::isUInt32 returns true only when the boxed value is int32_t and positive, it doesn't return true for uint32_t max value (0xFFFFFFFF) that is not array index."); Same thought about this assertion as above.
No, 0xFFFFFFFF is not an index! That is a JavaScript language feature, not something specific about our ending!
(In reply to comment #55) > No, 0xFFFFFFFF is not an index! That is a JavaScript language feature, not > something specific about our ending! Oops, please ignore that comment.
Comment on attachment 250220 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250220&action=review Thank you for your review. >> Source/JavaScriptCore/dfg/DFGOperations.cpp:105 >> + putByVal<strict, direct>(exec, baseValue, index, value); > > The message here might be a good comment to have explaining this assert, but it doesn’t seem helpful as an assertion message. I would write this assertion: > > // Despite its name, JSValue::isUInt32 will return true only for positive boxed int32_t; all those values are valid array indices. > ASSERT(isIndex(property.asUInt32())); > > And I would leave the actual putByVal line alone; I don’t think we should put the thing into a local variable just to share code between the real code and the assertion. Looks very nice! I'll change it. >> Source/JavaScriptCore/jit/JITOperations.cpp:505 >> + baseObject->putDirectIndex(callFrame, index, value, 0, isStrictMode ? PutDirectIndexShouldThrow : PutDirectIndexShouldNotThrow); > > Same comment here as above. Code should just be: > > baseObject->putDirectIndex(callFrame, subscript.asUInt32(), value, 0, isStrictMode ? PutDirectIndexShouldThrow : PutDirectIndexShouldNotThrow); Ditto >> Source/JavaScriptCore/jit/JITOperations.cpp:511 >> + uint32_t subscriptAsUInt32 = static_cast<uint32_t>(subscriptAsDouble); > > I’m surprised this is really the code we want. Does this handle values such as 2^32 properly? Do we have test coverage for this? Yes, it correctly handles values (int32_t max, uint32_t max) range. 0x7fffffff, 0x80000000 and others are covered by the tests. >> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:802 >> + ASSERT_WITH_MESSAGE(isIndex(index), "Since JSValue::isUInt32 returns true only when the boxed value is int32_t and positive, it doesn't return true for uint32_t max value (0xFFFFFFFF) that is not array index."); > > Same thought about this assertion as above. Ditto. > Source/JavaScriptCore/tests/stress/dfg-put-by-val-direct-with-edge-numbers.js:68 > + '"-4.2"', Oops. During migration from layout-tests to stress tests, their stringified representation ('"' + X + '"', '+0.0', etc) should be striped. So I'll land it after stripping it.
Committed r182452: <http://trac.webkit.org/changeset/182452>