Bug 172972

Actually, the real problem here is because Comparisons into ARMv6 MacroAssembler are being emitted incorrectly. Patch coming soon.

Created attachment 312851 [details] Patch

Comment on attachment 312851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312851&action=review > Source/JavaScriptCore/ChangeLog:11 > + semantics of cmn is oposite of cmp[1]. One case that it's breaking is typo: /oposite/opposite/ > Source/JavaScriptCore/ChangeLog:21 > + when $r0 > 0. In that case, the correct opcode is "cmp". Whith this typo: /Whith/With/ > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:1609 > - if (tmp != ARMAssembler::InvalidImmediate) > - m_assembler.cmn(left, tmp); > - else > + if (tmp != ARMAssembler::InvalidImmediate) { > + if (isUInt12(right.m_value)) > + m_assembler.cmp(left, tmp); > + else > + m_assembler.cmn(left, tmp); > + } else This is wrong; you'll turn a "cmn r0, #1" into a "cmp r0, #1", which is plainly wrong. tmp is an encoding of -right.m_value. Hence, the correct comparison operation to use is always cmn, with one exception: 0. That is because -0 is still 0, and hence, the sign polarity is not inverted as required for cmn to work properly. You can fix the issue by first checking for 0 before doing the cmn, or you can solve this in a more generic way like so: ARMWord tmp = m_assembler.getOp2(right.m_value); if (tmp != ARMAssembler::InvalidImmediate) { m_assembler.cmp(left, tmp); return; } ASSERT(right.m_value != 0 && static_cast<unsigned>(right.m_value) != 0x80000000); tmp = m_assembler.getOp2(-right.m_value); if (tmp != ARMAssembler::InvalidImmediate) { m_assembler.cmn(left, tmp); return; } m_assembler.cmp(left, m_assembler.getImm(right.m_value, ARMRegisters::S0)); The m_assembler.getOp2(right.m_value) case is guaranteed to take care of 0 and 0x80000000 (and a lot of other possible constants as well). Hence, it is now safe to try cmn on -right.m_value. > Source/JavaScriptCore/assembler/MacroAssemblerARM.h:1622 > - if (tmp != ARMAssembler::InvalidImmediate) > - m_assembler.cmn(ARMRegisters::S1, tmp); > - else > + if (tmp != ARMAssembler::InvalidImmediate) { > + if (isUInt12(right.m_value)) > + m_assembler.cmp(ARMRegisters::S1, tmp); > + else > + m_assembler.cmn(ARMRegisters::S1, tmp); > + } else Ditto. This is wrong.

Comment on attachment 312851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312851&action=review >> Source/JavaScriptCore/assembler/MacroAssemblerARM.h:1609 >> + } else > > This is wrong; you'll turn a "cmn r0, #1" into a "cmp r0, #1", which is plainly wrong. > > tmp is an encoding of -right.m_value. Hence, the correct comparison operation to use is always cmn, with one exception: 0. That is because -0 is still 0, and hence, the sign polarity is not inverted as required for cmn to work properly. You can fix the issue by first checking for 0 before doing the cmn, or you can solve this in a more generic way like so: > > ARMWord tmp = m_assembler.getOp2(right.m_value); > if (tmp != ARMAssembler::InvalidImmediate) { > m_assembler.cmp(left, tmp); > return; > } > ASSERT(right.m_value != 0 && static_cast<unsigned>(right.m_value) != 0x80000000); > tmp = m_assembler.getOp2(-right.m_value); > if (tmp != ARMAssembler::InvalidImmediate) { > m_assembler.cmn(left, tmp); > return; > } > m_assembler.cmp(left, m_assembler.getImm(right.m_value, ARMRegisters::S0)); > > The m_assembler.getOp2(right.m_value) case is guaranteed to take care of 0 and 0x80000000 (and a lot of other possible constants as well). Hence, it is now safe to try cmn on -right.m_value. Looks like m_assembler.getImm() already tries to encode m_assembler.getOp2(right.m_value). Hence, the fix is actually to add the !right.m_value check only like so: ARMWord tmp = (!right.m_value || static_cast<unsigned>(right.m_value) == 0x80000000) ? ARMAssembler::InvalidImmediate : m_assembler.getOp2(-right.m_value); if (tmp != ARMAssembler::InvalidImmediate) m_assembler.cmn(left, tmp); else m_assembler.cmp(left, m_assembler.getImm(right.m_value, ARMRegisters::S0));

Created attachment 312946 [details] Patch

(In reply to Mark Lam from comment #4) > Comment on attachment 312851 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=312851&action=review > > >> Source/JavaScriptCore/assembler/MacroAssemblerARM.h:1609 > >> + } else > > > > This is wrong; you'll turn a "cmn r0, #1" into a "cmp r0, #1", which is plainly wrong. > > > > tmp is an encoding of -right.m_value. Hence, the correct comparison operation to use is always cmn, with one exception: 0. That is because -0 is still 0, and hence, the sign polarity is not inverted as required for cmn to work properly. You can fix the issue by first checking for 0 before doing the cmn, or you can solve this in a more generic way like so: Oh, I think that I understood it wrongly. According my debug code here, all positive values fallback into cmp branch, since "m_assembler.getOp2" results in ARMAssembler::InvalidImmediate for them. > Looks like m_assembler.getImm() already tries to encode > m_assembler.getOp2(right.m_value). Hence, the fix is actually to add the > !right.m_value check only like so: > > ARMWord tmp = (!right.m_value || static_cast<unsigned>(right.m_value) == > 0x80000000) ? ARMAssembler::InvalidImmediate : > m_assembler.getOp2(-right.m_value); > if (tmp != ARMAssembler::InvalidImmediate) > m_assembler.cmn(left, tmp); > else > m_assembler.cmp(left, m_assembler.getImm(right.m_value, > ARMRegisters::S0)); Yes. I agree that it's the right fix. I think that just #0 is broken here. Running javascript-core locally to check failures. Thank you for the quick review.

Created attachment 312984 [details] Patch

Comment on attachment 312984 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312984&action=review r=me. Please fix the typo in the ChangeLog. > Source/JavaScriptCore/ChangeLog:11 > + semantics of cmn is oposite of cmp[1]. One case that it's breaking is please fix typo: /oposite/opposite/

Created attachment 313282 [details] Patch for landing ChangeLog updated.

Comment on attachment 313282 [details] Patch for landing Attachment 313282 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3957395 New failing tests: inspector/canvas/create-canvas-contexts.html

Created attachment 313290 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Created attachment 313296 [details] Patch for landing Rebased with master

Comment on attachment 313296 [details] Patch for landing Clearing flags on attachment: 313296 Committed r218519: <http://trac.webkit.org/changeset/218519>

All reviewed patches have been landed. Closing bug.

*** Bug 154853 has been marked as a duplicate of this bug. ***

*** Bug 154856 has been marked as a duplicate of this bug. ***

And it already fixed failing rest parameters and spread tests too.