[webkit-reviews] review denied: [Bug 234476] [JSC][32bit] Fix regexp crash on ARMv7 : [Attachment 449195] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has denied Mikhail R. Gadelha
<mikhail at igalia.com>'s request for review:
Bug 234476: [JSC][32bit] Fix regexp crash on ARMv7
https://bugs.webkit.org/show_bug.cgi?id=234476

Attachment 449195: Patch

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




--- Comment #36 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 449195
  --> https://bugs.webkit.org/attachment.cgi?id=449195
Patch

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

I think the updated code is breaking JITCage. Can you please do not change
ARM64 / ARM64E code?

> Source/JavaScriptCore/yarr/YarrJIT.cpp:-3996
> -	   if (!Options::useJITCage())
> -	       m_jit.tagReturnAddress();

This part is broken. You should not tag when JITCage is not enabled. But
updated code is not following.

> Source/JavaScriptCore/yarr/YarrJIT.cpp:4048
> +	       m_jit.untagReturnAddress();

Why is it added?

> Source/JavaScriptCore/yarr/YarrJIT.cpp:4183
> +	       constexpr unsigned ArgNum = 4;

We use `argNum` (instead of `ArgNum`) for constants.

> Source/JavaScriptCore/yarr/YarrJITRegisters.h:-75
> -    static constexpr GPRReg supplementaryPlanesBase = ARM64Registers::x12;
> -    static constexpr GPRReg leadingSurrogateTag = ARM64Registers::x13;
> -    static constexpr GPRReg trailingSurrogateTag = ARM64Registers::x14;

I think using register is better for code size. Please do not change this.