Currently they call it twice, see the following test case: ``` var o = {}; var counter = 0; o.valueOf = () => {counter ++; return 42}; o++; if (counter != 1) throw "valueOf was executed " + counter + " times during a post-increment instead of once!"; ``` As far as I can tell this is not spec-compliant: 12.4.4 Postfix Increment Operator requires a single call to ToNumeric. Then there is a single chain of ToNumeric -> ToNumber -> ToPrimitive -> OrdinaryToPrimitive -> valueOf with no reason for the call to be duplicated anywhere. The problem appears to be from: ``` static RegisterID* emitPostIncOrDec(BytecodeGenerator& generator, RegisterID* dst, RegisterID* srcDst, Operator oper) { if (dst == srcDst) return generator.emitToNumber(generator.finalDestination(dst), srcDst); RefPtr<RegisterID> tmp = generator.emitToNumber(generator.tempDestination(dst), srcDst); emitIncOrDec(generator, srcDst, oper); return generator.move(dst, tmp.get()); } ``` which uses an emitToNumber, but then does an emitIncOrDec on the original value, which itself can lead to a slow path that does ToNumber anew.
Things are a bit more subtle than I expected: wrapping the testcase above in a function and calling that function suppresses the bug! It comes back if "o++" is replaced by "var x = o++" so I suspect it is caused by the code: ``` RegisterID* PostfixNode::emitResolve(BytecodeGenerator& generator, RegisterID* dst) { if (dst == generator.ignoredResult()) return PrefixNode::emitResolve(generator, dst); ... RefPtr<RegisterID> oldValue = emitPostIncOrDec(generator, generator.finalDestination(dst), value.get(), m_operator); ... } ``` also in NodesCodegen.cpp. It visibly replaces "o++;" by "++o;" when the result is not stored anywhere. The mystery is why it only occurs when wrapped in a function, and not at the top-level.
Created attachment 380545 [details] Patch
Comment on attachment 380545 [details] Patch r=me
Comment on attachment 380545 [details] Patch Attachment 380545 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/13113521 Number of test failures exceeded the failure limit.
Created attachment 380608 [details] Archive of layout-test-results from ews213 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews213 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Seems like this patch has broken jsc EWS... :(
*** Bug 202710 has been marked as a duplicate of this bug. ***
Created attachment 380943 [details] Patch
Comment on attachment 380943 [details] Patch Attachment 380943 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/13131267 New failing tests: mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla
Created attachment 381002 [details] Patch Same patch as before, to check that the previous test failure was a flake.
Comment on attachment 381002 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=381002&action=review r=me > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1791 > + RefPtr<RegisterID> tmp = generator.emitToNumber(generator.newTemporary(), srcDst); > + RefPtr<RegisterID> result = generator.tempDestination(srcDst); > + generator.move(result.get(), tmp.get()); > + emitIncOrDec(generator, result.get(), oper); > + generator.move(srcDst, result.get()); why not something like this? ``` RefPtr<RegisterID> result = generator.emitToNumber(generator.tempDestination(dst), srcDst); emitIncOrDec(generator, result.get(), oper); generator.move(srcDst, result.get()); return generator.move(dst, result.get()); ```
Comment on attachment 381002 [details] Patch The jsc-mips bot appears stuck, but since it does not show an actual test failure and all other bots are green, I'm trying to land this.
Comment on attachment 381002 [details] Patch Clearing flags on attachment: 381002 Committed r251371: <https://trac.webkit.org/changeset/251371>
All reviewed patches have been landed. Closing bug.
<rdar://problem/56471606>