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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jan 11 02:24:05 PST 2022

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

Attachment 448500: minor style fix


--- 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
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 =
> -    static constexpr ARMRegisters::FPDoubleRegisterID fpTempRegister =
> +    static inline constexpr ARMRegisters::FPDoubleRegisterID fpTempRegister
= ARMRegisters::d7;

When constexpr is specified, static member variable is implicitly inline. So
this is not necessary.

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

More information about the webkit-reviews mailing list