[CSS JIT][ARMv7] The pseudo element early exit trashes r6
Created attachment 255037 [details] Patch
<rdar://problem/21329026> CSS JIT clobbers r6 on armv7
Comment on attachment 255037 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255037&action=review I think it needs to be more explicit with the pushing and popping of r6. I think this whole patch could also be made much simpler with just a push(r6) and pop(r6) at the beginning of generateSelectorChecker if the CPU is ARM_THUMB2. > Source/WebCore/cssjit/SelectorCompiler.cpp:1743 > +inline void SelectorCodeGenerator::generateFunctionEnding(StackAllocator& stackAllocator, bool needsEpilogue) I don't think this is a useful abstraction, and it makes it confusing to not see a popMacroAssemblerRegisters for each call to pushMacroAssemblerRegisters. This is a compiler, and I think we need to see that to remember to keep track of that so we don't have strange bugs like this in the future. > Source/WebCore/cssjit/SelectorCompiler.cpp:1867 > + m_stackAllocator.merge(WTF::move(successStack), WTF::move(failureStack)); What does this do? > Source/WebCore/cssjit/SelectorCompiler.cpp:1898 > + m_stackAllocator.merge(WTF::move(successStack), WTF::move(earlyFailureStack)); Is this necessary?
(In reply to comment #3) > > Source/WebCore/cssjit/SelectorCompiler.cpp:1867 > > + m_stackAllocator.merge(WTF::move(successStack), WTF::move(failureStack)); > > What does this do? > > > Source/WebCore/cssjit/SelectorCompiler.cpp:1898 > > + m_stackAllocator.merge(WTF::move(successStack), WTF::move(earlyFailureStack)); > > Is this necessary? This is part of the security tracking of stack allocator. When you have a runtime branch, StackAllocator let you verify that bot side of the runtime end up with the same stack.
Created attachment 255082 [details] Patch
Comment on attachment 255082 [details] Patch Attachment 255082 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5372972940394496 New failing tests: platform/mac-wk2/tiled-drawing/scrolling/scroll-snap/scroll-snap-mandatory-mainframe-slow-vertical.html
Created attachment 255083 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 255082 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255082&action=review Looks good. The 1 layout test failure is unrelated. > Source/WebCore/cssjit/SelectorCompiler.cpp:1861 > + } else > + failureStack = successStack; > + > + m_stackAllocator.merge(WTF::move(successStack), WTF::move(failureStack)); Couldn't you just do something like else failureStack.clear() instead of this? > Source/WebCore/cssjit/SelectorCompiler.cpp:1896 > + } else > + earlyFailureStack = successStack; > + m_stackAllocator.merge(WTF::move(successStack), WTF::move(earlyFailureStack)); ditto.
(In reply to comment #8) > Comment on attachment 255082 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=255082&action=review > > Looks good. The 1 layout test failure is unrelated. > > > Source/WebCore/cssjit/SelectorCompiler.cpp:1861 > > + } else > > + failureStack = successStack; > > + > > + m_stackAllocator.merge(WTF::move(successStack), WTF::move(failureStack)); > > Couldn't you just do something like else failureStack.clear() instead of > this? > > > Source/WebCore/cssjit/SelectorCompiler.cpp:1896 > > + } else > > + earlyFailureStack = successStack; > > + m_stackAllocator.merge(WTF::move(successStack), WTF::move(earlyFailureStack)); > > ditto. In this particular case, you are right that clear() would work, because the result is a cleared stack. But I prefer this general pattern as it can be used in more places.
Comment on attachment 255082 [details] Patch Clearing flags on attachment: 255082 Committed r185719: <http://trac.webkit.org/changeset/185719>
All reviewed patches have been landed. Closing bug.