[webkit-reviews] review denied: [Bug 133156] make css jit work on arm64 : [Attachment 231836] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 133156: make css jit work on arm64
https://bugs.webkit.org/show_bug.cgi?id=133156

Attachment 231836: Patch
https://bugs.webkit.org/attachment.cgi?id=231836&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=231836&action=review


> Source/WebCore/cssjit/SelectorCompiler.cpp:874
> +#if CPU(ARM64)
> +    m_assembler.pushPair(JSC::ARM64Registers::lr, JSC::ARM64Registers::fp);
> +#endif
> +}
> +
> +void SelectorCodeGenerator::generateEpilogue()
> +{
> +#if CPU(ARM64)
> +    m_assembler.popPair(JSC::ARM64Registers::lr, JSC::ARM64Registers::fp);
> +#endif

I would prefer this to be done through the stack allocator.

Here, if we have a mismatch of pushPair/popPair, we could run into security
problems.

> Source/WebCore/cssjit/SelectorCompiler.cpp:935
> +	       generateEpilogue();
>	       m_assembler.ret();
>  
>	       // Failure.
>	       if (!failureCases.empty()) {
>		   failureCases.link(&m_assembler);
>		   m_assembler.move(Assembler::TrustedImm32(0),
returnRegister);
> +		   generateEpilogue();
>		   m_assembler.ret();

The duplicated epilogue may be slower than an unconditional jump. You may want
to generalize reservedCalleeSavedRegisters.