[webkit-reviews] review denied: [Bug 233822] [JSC] Port EXTRA_CTI_THUNKS to all platforms : [Attachment 448500] minor style fix

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has denied Geza Lore <glore at igalia.com>'s
request for review:
Bug 233822: [JSC] Port EXTRA_CTI_THUNKS to all platforms
https://bugs.webkit.org/show_bug.cgi?id=233822

Attachment 448500: minor style fix

https://bugs.webkit.org/attachment.cgi?id=448500&action=review




--- Comment #15 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 448500
  --> https://bugs.webkit.org/attachment.cgi?id=448500
minor style fix

View in context: https://bugs.webkit.org/attachment.cgi?id=448500&action=review

Nice. But found several bugs.
In ARM64E, we need to tag return address register in the prologue. And we
should not tag it when doing a tail call. It seems that this patch broke these
things.
Please check this is met by comparing old code and new code.

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:44
> +    static inline constexpr RegisterID dataTempRegister = ARMRegisters::ip;
> +    static inline constexpr RegisterID addressTempRegister =
ARMRegisters::r6;
>  
> -    static constexpr ARMRegisters::FPDoubleRegisterID fpTempRegister =
ARMRegisters::d7;
> +    static inline constexpr ARMRegisters::FPDoubleRegisterID fpTempRegister
= ARMRegisters::d7;

When constexpr is specified, static member variable is implicitly inline. So
this is not necessary.
http://eel.is/c++draft/dcl.constexpr#1.sentence-3

> Source/JavaScriptCore/jit/CCallHelpers.cpp:98
> +static_assert(!((maxFrameExtentForSlowPathCall + 2*sizeof(CPURegister)) %
16), "Stack must be aligned after CTI thunk entry");

We should add space between 2 and * and sizeof.

> Source/JavaScriptCore/jit/CCallHelpers.cpp:100
> +void CCallHelpers::emitCTIThunkPrologue()

This is not correct. It is removing tagging from the original code.
Please ensure that these code is exactly the same to the original sequence of
generated code.

> Source/JavaScriptCore/jit/JITOpcodes.cpp:-1286
> -    jit.tagReturnAddress();

This is missing.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:-2625
> -    jit.tagReturnAddress();

This is removed.