Bug 146078 – [CSS JIT][ARMv7] The pseudo element early exit trashes r6

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

[CSS JIT][ARMv7] The pseudo element early exit trashes r6

[Benjamin Poulain]

Created attachment 255037 [details]
Patch

[Benjamin Poulain]

<rdar://problem/21329026> CSS JIT clobbers r6 on armv7

[Alex Christensen]

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?

[Benjamin Poulain]

(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.

[Benjamin Poulain]

Created attachment 255082 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Alex Christensen]

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.

[Benjamin Poulain]

(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.

[Benjamin Poulain]

Comment on attachment 255082 [details]
Patch

Clearing flags on attachment: 255082

Committed r185719: <http://trac.webkit.org/changeset/185719>

[Benjamin Poulain]

All reviewed patches have been landed.  Closing bug.