Bug 146078 - [CSS JIT][ARMv7] The pseudo element early exit trashes r6
Summary: [CSS JIT][ARMv7] The pseudo element early exit trashes r6
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-06-17 14:43 PDT by Benjamin Poulain
Modified: 2015-06-18 13:30 PDT (History)
3 users (show)

See Also:


Attachments
Patch (10.83 KB, patch)
2015-06-17 14:49 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (10.13 KB, patch)
2015-06-17 21:03 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (586.28 KB, application/zip)
2015-06-17 21:40 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2015-06-17 14:43:26 PDT
[CSS JIT][ARMv7] The pseudo element early exit trashes r6
Comment 1 Benjamin Poulain 2015-06-17 14:49:25 PDT
Created attachment 255037 [details]
Patch
Comment 2 Benjamin Poulain 2015-06-17 14:55:37 PDT
<rdar://problem/21329026> CSS JIT clobbers r6 on armv7
Comment 3 Alex Christensen 2015-06-17 16:35:08 PDT
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?
Comment 4 Benjamin Poulain 2015-06-17 16:38:51 PDT
(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.
Comment 5 Benjamin Poulain 2015-06-17 21:03:26 PDT
Created attachment 255082 [details]
Patch
Comment 6 Build Bot 2015-06-17 21:40:28 PDT
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
Comment 7 Build Bot 2015-06-17 21:40:30 PDT
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 8 Alex Christensen 2015-06-18 10:18:57 PDT
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.
Comment 9 Benjamin Poulain 2015-06-18 13:29:29 PDT
(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 10 Benjamin Poulain 2015-06-18 13:30:15 PDT
Comment on attachment 255082 [details]
Patch

Clearing flags on attachment: 255082

Committed r185719: <http://trac.webkit.org/changeset/185719>
Comment 11 Benjamin Poulain 2015-06-18 13:30:19 PDT
All reviewed patches have been landed.  Closing bug.