Bug 234476 – [JSC][32bit] Fix regexp crash on ARMv7

• Component: New Bugs
• Status: RESOLVED FIXED

[Mikhail R. Gadelha]

[JSC][32bit] Fix regexp crash when building on ARMv7 using gcc 10.2.1

[Mikhail R. Gadelha]

Created attachment 447519 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 447675 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 447729 [details]
Patch

[Yusuke Suzuki]

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

[Radar WebKit Bug Importer]

<rdar://problem/86904879>

[Mikhail R. Gadelha]

Created attachment 448420 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 448422 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 448423 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 448425 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 448434 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 448635 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 448679 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 448681 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 448685 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 448767 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 448785 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 448841 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 448845 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 448848 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 448856 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 448867 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 448878 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 448884 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 448937 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 448964 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 448971 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 449060 [details]
Patch

[Geza Lore]

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.

[Mikhail R. Gadelha]

Created attachment 449068 [details]
Patch

[Darin Adler]

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.

[Mikhail R. Gadelha]

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

[Mikhail R. Gadelha]

Created attachment 449091 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 449108 [details]
Patch

[Darin Adler]

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.

[Mikhail R. Gadelha]

Created attachment 449195 [details]
Patch

[Yusuke Suzuki]

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.

[Mikhail R. Gadelha]

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.

[Mikhail R. Gadelha]

Created attachment 449208 [details]
Patch

[Yusuke Suzuki]

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?

[Geza Lore]

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.

[Mikhail R. Gadelha]

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.

[Yusuke Suzuki]

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

[Mikhail R. Gadelha]

(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

[Yusuke Suzuki]

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

[Yusuke Suzuki]

Comment on attachment 449208 [details]
Patch

So, since this is regressing ARM64 code, putting r-.

[Mikhail R. Gadelha]

Created attachment 449345 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 449450 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 449495 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 449495 [details]
Patch

r=me

[EWS]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 235470

[Yusuke Suzuki]

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())));

[Mikhail R. Gadelha]

Created attachment 449825 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 449825 [details]
Patch

r=me, nice.

[EWS]

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