Bug 234476 - [JSC][32bit] Fix regexp crash on ARMv7
Summary: [JSC][32bit] Fix regexp crash on ARMv7
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mikhail R. Gadelha
URL:
Keywords: InRadar
Depends on: 235470
Blocks:
  Show dependency treegraph
 
Reported: 2021-12-18 06:50 PST by Mikhail R. Gadelha
Modified: 2022-01-24 15:21 PST (History)
15 users (show)

See Also:


Attachments
Patch (8.82 KB, patch)
2021-12-18 06:56 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (7.75 KB, patch)
2021-12-20 20:07 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (10.69 KB, patch)
2021-12-21 09:59 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (21.36 KB, patch)
2022-01-05 12:38 PST, Mikhail R. Gadelha
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (22.14 KB, patch)
2022-01-05 12:58 PST, Mikhail R. Gadelha
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (22.87 KB, patch)
2022-01-05 13:10 PST, Mikhail R. Gadelha
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (23.02 KB, patch)
2022-01-05 13:24 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (23.39 KB, patch)
2022-01-05 15:11 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (22.26 KB, patch)
2022-01-07 14:56 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (22.52 KB, patch)
2022-01-08 13:11 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (22.16 KB, patch)
2022-01-08 13:41 PST, Mikhail R. Gadelha
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (28.71 KB, patch)
2022-01-08 15:41 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (30.80 KB, patch)
2022-01-10 10:21 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (30.85 KB, patch)
2022-01-10 12:21 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (30.81 KB, patch)
2022-01-11 06:06 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (24.55 KB, patch)
2022-01-11 06:57 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (26.02 KB, patch)
2022-01-11 08:05 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (32.01 KB, patch)
2022-01-11 10:53 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (31.93 KB, patch)
2022-01-11 12:14 PST, Mikhail R. Gadelha
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (26.74 KB, patch)
2022-01-11 14:22 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (26.40 KB, patch)
2022-01-11 15:40 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (33.13 KB, patch)
2022-01-12 05:36 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (32.46 KB, patch)
2022-01-12 10:52 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (32.83 KB, patch)
2022-01-12 11:13 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (32.99 KB, patch)
2022-01-13 07:53 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (33.48 KB, patch)
2022-01-13 09:14 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (33.54 KB, patch)
2022-01-13 12:10 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (33.55 KB, patch)
2022-01-13 14:15 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (33.57 KB, patch)
2022-01-14 11:47 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (29.82 KB, patch)
2022-01-14 12:36 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (28.30 KB, patch)
2022-01-17 12:44 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (28.73 KB, patch)
2022-01-18 16:43 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (28.38 KB, patch)
2022-01-19 10:53 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff
Patch (30.14 KB, patch)
2022-01-24 09:24 PST, Mikhail R. Gadelha
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail R. Gadelha 2021-12-18 06:50:19 PST
[JSC][32bit] Fix regexp crash when building on ARMv7 using gcc 10.2.1
Comment 1 Mikhail R. Gadelha 2021-12-18 06:56:44 PST
Created attachment 447519 [details]
Patch
Comment 2 Mikhail R. Gadelha 2021-12-20 20:07:09 PST
Created attachment 447675 [details]
Patch
Comment 3 Mikhail R. Gadelha 2021-12-21 09:59:57 PST
Created attachment 447729 [details]
Patch
Comment 4 Yusuke Suzuki 2021-12-21 10:57:23 PST
Comment on attachment 447729 [details]
Patch

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

> Source/JavaScriptCore/yarr/YarrJIT.cpp:4204
> +            m_jit.loadPtr(MacroAssembler::Address(MacroAssembler::stackPointerRegister, m_arg5OnStackOffset), matchingContext);

Instead, can we use `CCallHelpers::calculatePokeOffset` to compute the right offset without computing it per architecture in YarrJIT?

> Source/JavaScriptCore/yarr/YarrJIT.cpp:4685
> +    unsigned m_arg5OnStackOffset = 0;

For default field initializer, use `{ 0 }` instead of ` = 0`.
Comment 5 Radar WebKit Bug Importer 2021-12-25 06:51:16 PST
<rdar://problem/86904879>
Comment 6 Mikhail R. Gadelha 2022-01-05 12:38:09 PST
Created attachment 448420 [details]
Patch
Comment 7 Mikhail R. Gadelha 2022-01-05 12:58:27 PST
Created attachment 448422 [details]
Patch
Comment 8 Mikhail R. Gadelha 2022-01-05 13:10:39 PST
Created attachment 448423 [details]
Patch
Comment 9 Mikhail R. Gadelha 2022-01-05 13:24:15 PST
Created attachment 448425 [details]
Patch
Comment 10 Mikhail R. Gadelha 2022-01-05 15:11:29 PST
Created attachment 448434 [details]
Patch
Comment 11 Mikhail R. Gadelha 2022-01-07 14:56:22 PST
Created attachment 448635 [details]
Patch
Comment 12 Mikhail R. Gadelha 2022-01-08 13:11:23 PST
Created attachment 448679 [details]
Patch
Comment 13 Mikhail R. Gadelha 2022-01-08 13:41:35 PST
Created attachment 448681 [details]
Patch
Comment 14 Mikhail R. Gadelha 2022-01-08 15:41:37 PST
Created attachment 448685 [details]
Patch
Comment 15 Mikhail R. Gadelha 2022-01-10 10:21:49 PST
Created attachment 448767 [details]
Patch
Comment 16 Mikhail R. Gadelha 2022-01-10 12:21:56 PST
Created attachment 448785 [details]
Patch
Comment 17 Mikhail R. Gadelha 2022-01-11 06:06:46 PST
Created attachment 448841 [details]
Patch
Comment 18 Mikhail R. Gadelha 2022-01-11 06:57:29 PST
Created attachment 448845 [details]
Patch
Comment 19 Mikhail R. Gadelha 2022-01-11 08:05:40 PST
Created attachment 448848 [details]
Patch
Comment 20 Mikhail R. Gadelha 2022-01-11 10:53:57 PST
Created attachment 448856 [details]
Patch
Comment 21 Mikhail R. Gadelha 2022-01-11 12:14:09 PST
Created attachment 448867 [details]
Patch
Comment 22 Mikhail R. Gadelha 2022-01-11 14:22:02 PST
Created attachment 448878 [details]
Patch
Comment 23 Mikhail R. Gadelha 2022-01-11 15:40:21 PST
Created attachment 448884 [details]
Patch
Comment 24 Mikhail R. Gadelha 2022-01-12 05:36:31 PST
Created attachment 448937 [details]
Patch
Comment 25 Mikhail R. Gadelha 2022-01-12 10:52:53 PST
Created attachment 448964 [details]
Patch
Comment 26 Mikhail R. Gadelha 2022-01-12 11:13:06 PST
Created attachment 448971 [details]
Patch
Comment 27 Mikhail R. Gadelha 2022-01-13 07:53:54 PST
Created attachment 449060 [details]
Patch
Comment 28 Geza Lore 2022-01-13 08:00:44 PST
Comment on attachment 449060 [details]
Patch

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

> Source/JavaScriptCore/jit/CCallHelpers.h:1001
> +    constexpr GPRReg loadArgumentFromStack(GPRReg scratchGPR)

This function is not constexpr (it calls `loadPtr` which is not constexpr), nor does it need to be constexpr. Same applies to 'getArgumentGPR'

> Source/JavaScriptCore/jit/CCallHelpers.h:1037
> +    constexpr GPRReg getArgumentGPR(GPRReg scratchGPR = GPRReg::InvalidGPRReg)

Like loadArgumentFromStack, this function is not constexpr, nor does it need to be.
Comment 29 Mikhail R. Gadelha 2022-01-13 09:14:20 PST
Created attachment 449068 [details]
Patch
Comment 30 Darin Adler 2022-01-13 10:28:58 PST
Comment on attachment 449068 [details]
Patch

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

Some coding style comments: not intended to be a complete review, although the patch generally looks good

> Source/JavaScriptCore/jit/CCallHelpers.h:996
> +private:
> +

Normally we’d omit that blank line.

> Source/JavaScriptCore/jit/CCallHelpers.h:1001
> +    GPRReg loadArgumentFromStack(unsigned ArgNum, GPRReg scratchGPR)

When ArgNum is just a function argument rather than a template argument, I suggest we use the traditional lowercased "argumentNumber", or "argNum" if the local tradition in this code is to use abbreviations rather than words (WebKit style generally is to prefer words).

But also, why are we using a function argument rather than template argument here? Is there a good reason to mix styles like this?

> Source/JavaScriptCore/jit/CCallHelpers.h:1018
> +        constexpr int returnAdressOffset = 2;

"address" is misspelled here

> Source/JavaScriptCore/jit/CCallHelpers.h:1023
> +        const int returnTypeOffset = sizeof(RESULT_TYPE) >= 8;
> +        const int offset = ArgNum - NUMBER_OF_ARGUMENT_REGISTERS + POKE_ARGUMENT_OFFSET + returnAdressOffset + returnTypeOffset;
> +#else
> +        const int offset = ArgNum - NUMBER_OF_ARGUMENT_REGISTERS + POKE_ARGUMENT_OFFSET + returnAdressOffset;

constexpr please

But also, maybe we should define a returnTypeOffset of 0 for non-Windows-X86 rather than repeating the entire offset expression twice.

> Source/JavaScriptCore/jit/CCallHelpers.h:1032
> +public:
> +

Here, we’d omit the blank line.

> Source/JavaScriptCore/jit/CCallHelpers.h:1039
> +        ASSERT(ArgNum <= 255, "C++ standard forbis functions with more than 256 arguments");

"forbids" Is misspelled here

> Source/JavaScriptCore/runtime/RegExpInlines.h:149
> +                Yarr::MatchingContextHolder regExpContext(vm, /* usesPatternContextBuffer: */ false, this, matchFrom);

These comments are needed because we are not following the usual WebKit convention of using an enumeration for an argument like this where we often pass a constant value. That style is typically pretty easy to follow, although it does make the non-constant-value call sites a little wordy.

> Source/JavaScriptCore/yarr/YarrJIT.h:276
>  public:
> +

No blank line in a case like this.

> Source/JavaScriptCore/yarr/YarrJIT.h:280
> +    using YarrJITCode8 = SlowPathReturnType (*)(const LChar* input, UCPURegister start, UCPURegister length, int* output, MatchingContextHolder* matchingContext) YARR_CALL;
> +    using YarrJITCode16 = SlowPathReturnType (*)(const UChar* input, UCPURegister start, UCPURegister length, int* output, MatchingContextHolder* matchingContext) YARR_CALL;
> +    using YarrJITCodeMatchOnly8 = SlowPathReturnType (*)(const LChar* input, UCPURegister start, UCPURegister length, void*, MatchingContextHolder* matchingContext) YARR_CALL;
> +    using YarrJITCodeMatchOnly16 = SlowPathReturnType (*)(const UChar* input, UCPURegister start, UCPURegister length, void*, MatchingContextHolder* matchingContext) YARR_CALL;

I suggests omitting the matchingContext argument name here since WebKit coding style tells us to do that when the type makes them unambiguous.

> Source/JavaScriptCore/yarr/YarrJITRegisters.h:38
>  public:
> +

Again, no blank line here in our coding style. Also, should just delete "public" since this is a struct.
Comment 31 Mikhail R. Gadelha 2022-01-13 12:08:42 PST
Comment on attachment 449068 [details]
Patch

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

>> Source/JavaScriptCore/jit/CCallHelpers.h:1001
>> +    GPRReg loadArgumentFromStack(unsigned ArgNum, GPRReg scratchGPR)
> 
> When ArgNum is just a function argument rather than a template argument, I suggest we use the traditional lowercased "argumentNumber", or "argNum" if the local tradition in this code is to use abbreviations rather than words (WebKit style generally is to prefer words).
> 
> But also, why are we using a function argument rather than template argument here? Is there a good reason to mix styles like this?

In the previous patch, ArgNum was a template argument but I revert as suggested in the previous review.

>> Source/JavaScriptCore/jit/CCallHelpers.h:1023
>> +        const int offset = ArgNum - NUMBER_OF_ARGUMENT_REGISTERS + POKE_ARGUMENT_OFFSET + returnAdressOffset;
> 
> constexpr please
> 
> But also, maybe we should define a returnTypeOffset of 0 for non-Windows-X86 rather than repeating the entire offset expression twice.

I'll make ArgNum a template argument otherwise the compiler won't allow this to be a constexpr (it complains that ArgNum is not a constant).
Comment 32 Mikhail R. Gadelha 2022-01-13 12:10:31 PST
Created attachment 449091 [details]
Patch
Comment 33 Mikhail R. Gadelha 2022-01-13 14:15:02 PST
Created attachment 449108 [details]
Patch
Comment 34 Darin Adler 2022-01-14 09:55:10 PST
Comment on attachment 449108 [details]
Patch

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

> Source/JavaScriptCore/jit/CCallHelpers.h:1003
> +        ASSERT(ArgNum >= NUMBER_OF_ARGUMENT_REGISTERS, "Should return GPRInfo::argumentGPRX");

This can/should be a static_assert.

> Source/JavaScriptCore/jit/CCallHelpers.h:1037
> +        ASSERT(ArgNum <= 255, "C++ standard forbids functions with more than 256 arguments");

This can/should be a static_assert.

> Source/JavaScriptCore/jit/CCallHelpers.h:1038
> +        ASSERT(FunctionTraits<OperationType>::cCallArity() > ArgNum, "Function does not have enough arguments");

This can/should be a static_assert.
Comment 35 Mikhail R. Gadelha 2022-01-14 11:47:42 PST
Created attachment 449195 [details]
Patch
Comment 36 Yusuke Suzuki 2022-01-14 11:59:30 PST
Comment on attachment 449195 [details]
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.
Comment 37 Mikhail R. Gadelha 2022-01-14 12:14:03 PST
Comment on attachment 449195 [details]
Patch

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

>> Source/JavaScriptCore/yarr/YarrJIT.cpp:-3996
>> -            m_jit.tagReturnAddress();
> 
> This part is broken. You should not tag when JITCage is not enabled. But updated code is not following.

I see, the problem is that emitFunctionPrologue() always calls tagReturnAddress(). I'll special-case ARM64 then and revert to the original code.

>> Source/JavaScriptCore/yarr/YarrJIT.cpp:4048
>> +            m_jit.untagReturnAddress();
> 
> Why is it added?

Suggested by a colleague, I'll remove it.
Comment 38 Mikhail R. Gadelha 2022-01-14 12:36:35 PST
Created attachment 449208 [details]
Patch
Comment 39 Yusuke Suzuki 2022-01-14 12:38:35 PST
Comment on attachment 449208 [details]
Patch

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

> Source/JavaScriptCore/yarr/YarrJIT.cpp:3963
> +#if !CPU(ARM64)
> +        m_jit.emitFunctionPrologue();
> +#endif

Why is this change necessary? It looks like rather it is introducing complication.

> Source/JavaScriptCore/yarr/YarrJIT.cpp:4062
> +    m_jit.emitFunctionEpilogue();

indentation is not correct. And why do we need this modification?
Comment 40 Geza Lore 2022-01-14 12:41:31 PST
Yusuke: 'emitFunctionPrologue' always tags the return address. We did not use to use this, but getting to the stacked arguments consistently without the frame pointer setup is a pain. So we now call 'emitFunctionPrologue', always. To do the tail call when the JITCage is used, we simply untag prior to the tail call. Should have the same effect.
Comment 41 Mikhail R. Gadelha 2022-01-14 12:46:27 PST
Comment on attachment 449208 [details]
Patch

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

>> Source/JavaScriptCore/yarr/YarrJIT.cpp:3963
>> +#endif
> 
> Why is this change necessary? It looks like rather it is introducing complication.

We need it so the frame pointer is properly set up in a way that is independent of the arch. The function loadArgumentFromStack needs the frame pointer to be set or it will fail to load the argument.
Comment 42 Yusuke Suzuki 2022-01-14 12:49:17 PST
(In reply to Mikhail R. Gadelha from comment #41)
> Comment on attachment 449208 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449208&action=review
> 
> >> Source/JavaScriptCore/yarr/YarrJIT.cpp:3963
> >> +#endif
> > 
> > Why is this change necessary? It looks like rather it is introducing complication.
> 
> We need it so the frame pointer is properly set up in a way that is
> independent of the arch. The function loadArgumentFromStack needs the frame
> pointer to be set or it will fail to load the argument.

Why cant’ we use stack pointer here? If we count # of pushes in prologue (as suggested in https://webkit.slack.com/archives/CTV4FGWF4/p1640206561238000?thread_ts=1640112033.226400&cid=CTV4FGWF4), we can use stack pointer to get the argument.
Comment 43 Mikhail R. Gadelha 2022-01-14 13:03:19 PST
(In reply to Yusuke Suzuki from comment #42)
> (In reply to Mikhail R. Gadelha from comment #41)
> > Comment on attachment 449208 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=449208&action=review
> > 
> > >> Source/JavaScriptCore/yarr/YarrJIT.cpp:3963
> > >> +#endif
> > > 
> > > Why is this change necessary? It looks like rather it is introducing complication.
> > 
> > We need it so the frame pointer is properly set up in a way that is
> > independent of the arch. The function loadArgumentFromStack needs the frame
> > pointer to be set or it will fail to load the argument.
> 
> Why cant’ we use stack pointer here? If we count # of pushes in prologue (as
> suggested in
> https://webkit.slack.com/archives/CTV4FGWF4/
> p1640206561238000?thread_ts=1640112033.226400&cid=CTV4FGWF4), we can use
> stack pointer to get the argument.

We can use it but (In reply to Yusuke Suzuki from comment #42)
> (In reply to Mikhail R. Gadelha from comment #41)
> > Comment on attachment 449208 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=449208&action=review
> > 
> > >> Source/JavaScriptCore/yarr/YarrJIT.cpp:3963
> > >> +#endif
> > > 
> > > Why is this change necessary? It looks like rather it is introducing complication.
> > 
> > We need it so the frame pointer is properly set up in a way that is
> > independent of the arch. The function loadArgumentFromStack needs the frame
> > pointer to be set or it will fail to load the argument.
> 
> Why cant’ we use stack pointer here? If we count # of pushes in prologue (as
> suggested in
> https://webkit.slack.com/archives/CTV4FGWF4/
> p1640206561238000?thread_ts=1640112033.226400&cid=CTV4FGWF4), we can use
> stack pointer to get the argument.

It just (In reply to Yusuke Suzuki from comment #42)
> (In reply to Mikhail R. Gadelha from comment #41)
> > Comment on attachment 449208 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=449208&action=review
> > 
> > >> Source/JavaScriptCore/yarr/YarrJIT.cpp:3963
> > >> +#endif
> > > 
> > > Why is this change necessary? It looks like rather it is introducing complication.
> > 
> > We need it so the frame pointer is properly set up in a way that is
> > independent of the arch. The function loadArgumentFromStack needs the frame
> > pointer to be set or it will fail to load the argument.
> 
> Why cant’ we use stack pointer here? If we count # of pushes in prologue (as
> suggested in
> https://webkit.slack.com/archives/CTV4FGWF4/
> p1640206561238000?thread_ts=1640112033.226400&cid=CTV4FGWF4), we can use
> stack pointer to get the argument.

We can but the current approach seems more robust because we have an arch-independent way of getting what we want: we simply need to set up frame pointer in the function prologue.

The patch that counts the pushes looks worse IMHO (ignore the code not in generateEnter()): https://bugs.webkit.org/attachment.cgi?id=448434&action=review
Comment 44 Yusuke Suzuki 2022-01-14 13:11:20 PST
(In reply to Mikhail R. Gadelha from comment #43)
> 
> We can but the current approach seems more robust because we have an
> arch-independent way of getting what we want: we simply need to set up frame
> pointer in the function prologue.
> 
> The patch that counts the pushes looks worse IMHO (ignore the code not in
> generateEnter()):
> https://bugs.webkit.org/attachment.cgi?id=448434&action=review

But this is adding unnecessary tagging and untagging, which can affect on,

1. machine code generation size
2. performance

since RegExp code is tiny.
So, even it is more robust, I don't think this is OK since it hurts performance.
Comment 45 Yusuke Suzuki 2022-01-15 13:26:23 PST
Comment on attachment 449208 [details]
Patch

So, since this is regressing ARM64 code, putting r-.
Comment 46 Mikhail R. Gadelha 2022-01-17 12:44:52 PST
Created attachment 449345 [details]
Patch
Comment 47 Mikhail R. Gadelha 2022-01-18 16:43:11 PST
Created attachment 449450 [details]
Patch
Comment 48 Mikhail R. Gadelha 2022-01-19 10:53:04 PST
Created attachment 449495 [details]
Patch
Comment 49 Yusuke Suzuki 2022-01-21 20:15:28 PST
Comment on attachment 449495 [details]
Patch

r=me
Comment 50 EWS 2022-01-21 20:59:45 PST
Committed r288400 (246291@main): <https://commits.webkit.org/246291@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449495 [details].
Comment 51 WebKit Commit Bot 2022-01-21 22:01:40 PST
Re-opened since this is blocked by bug 235470
Comment 52 Yusuke Suzuki 2022-01-21 22:04:51 PST
Reverted since it broke ARM64E build.

According to Peng, the error is the following.

/Volumes/Data/webkit/OpenSource/Source/JavaScriptCore/yarr/YarrJIT.h:335:32: error: no matching function for call to 'vmEntryToYarrJIT'
      return MatchResult(vmEntryToYarrJIT(input, start, length, output, &matchingContext, retagCodePtr<Yarr8BitPtrTag, YarrEntryPtrTag>(m_ref8.code().executableAddress())));
Comment 53 Mikhail R. Gadelha 2022-01-24 09:24:01 PST
Created attachment 449825 [details]
Patch
Comment 54 Yusuke Suzuki 2022-01-24 14:49:10 PST
Comment on attachment 449825 [details]
Patch

r=me, nice.
Comment 55 EWS 2022-01-24 15:21:33 PST
Committed r288476 (246359@main): <https://commits.webkit.org/246359@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449825 [details].