Bug 209435 – Refactor YARR Stack Overflow checks

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

Currently the YARR compiler, ByteCode compiler and JIT generated code handle stack overflow checks differently or not at all.

These should be unified to check for available stack space in a consist manner.

[Michael Saboff]

<rdar://problem/58988252>

[Michael Saboff]

Created attachment 394299 [details]
Draft patch for EWS

[Michael Saboff]

Created attachment 394317 [details]
Updated patch for EWS

[Michael Saboff]

Created attachment 394342 [details]
Speculative fixes for ARMv7 and Windows

[Michael Saboff]

Created attachment 394366 [details]
Speculative fix for MIPS

[Michael Saboff]

Created attachment 394508 [details]
Patch for Landing

[Mark Lam]

Comment on attachment 394508 [details]
Patch for Landing

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

r=me with suggested improvements.

> JSTests/ChangeLog:9
> +	Added a new test and removed a now obsolete test.

Fix indentation.  Can you also explain why the old test is obsolete?

> JSTests/stress/regexp-huge-oom.js:1
> +// Test that throw an OOM exception when compiling / executing a pathologically large RegExp's

Should this test be skipped for memoryLimited devices?  Or will it fail fast?

> Source/JavaScriptCore/runtime/RegExp.cpp:302
> +    position = matchInline<Vector<int>&, Yarr::MatchFrom::CompilerThread>(vm, s, startOffset, ovector);

Previously, we called match() instead of matchInline() because I think we deliberately didn't want to inline it here.  matchInline is ALWAYS_INLINE.  I don't know the reason for the decision to not inline, but unless you have a reason to change this, I suggest you add a not inlined matchFromCompilerThread() that calls matchInline to preserve the previous behavior.  We also don't want to bloat the code size unnecessarily.

> Source/JavaScriptCore/runtime/RegExp.cpp:365
> +    result = matchInline<Yarr::MatchFrom::CompilerThread>(vm, s, startOffset);

Ditto.  Use a not inlined matchFromCompilerThread().

> Source/JavaScriptCore/runtime/RegExpInlines.h:133
> +            Yarr::MatchingContextHolder regExpContext(vm, m_regExpJITCode.get(), matchFrom);

Can we call this MatchContext instead?  Then you can declare this consistently as:
    Yarr::MatchContext matchContext(vm, m_regExpJITCode.get(), matchFrom);

I don't think "Matching" adds anything over "Match", and "Holder" doesn't add anything in addition to "Context".

> Source/JavaScriptCore/runtime/RegExpInlines.h:136
> +                result = m_regExpJITCode->execute(s.characters8(), startOffset, s.length(), offsetVector, &regExpContext).start;

Can we mate the execute() functions take a MatchContext& instead?  We don't allow execute() to ever be called without a MatchContext, right?

> Source/JavaScriptCore/yarr/YarrJIT.cpp:93
> +    static const RegisterID matchingContext = ARM64Registers::x4;

Let's call this matchContext.

> Source/JavaScriptCore/yarr/YarrJIT.cpp:3893
> +#if CPU(X86_64) && OS(WINDOWS)
> +            RegisterID matchingContext = regT1;
> +            loadPtr(Address(X86Registers::ebp, 7 * sizeof(void*)), matchingContext);
> +#elif CPU(ARM_THUMB2) || CPU(MIPS)
> +            RegisterID matchingContext = regT1;
> +            loadPtr(Address(stackPointerRegister, 4 * sizeof(void*)), matchingContext);
> +#endif

After staring at this code for a while, it finally dawned on me that you're loading the 5th argument passed in execute() to the YarrCodeBlock JIT generated function here, and the 5th argument in this case happens to be at the stack locations.  Please add a comment here to document this detail.  Currently, it's difficult to tell from just reading this code where these constants came from and why it would work.

> Source/JavaScriptCore/yarr/YarrJIT.cpp:3898
> +            move(TrustedImmPtr((void*)static_cast<size_t>(-2)), returnRegister);
> +            move(TrustedImm32(0), returnRegister2);

You're returning a MatchResult here.  Is -2 the start or the end?  I presume it's the start (CPU(BIG_ENDIAN) folks may have trouble ... I suggest pinging Caio as an FYI).

According to MatchResult, return WTF::notFound (i.e. -1) for start means the match failed.  Why not return WTF::notFound here?
If there's a need for a different error code (and I suspect this is the case), let's make -2 an official error code in MatchResult and give it a name instead of using a literal here.

Wait a minute ... I see that you already have a name for this: JSRegExpJITCodeFailure.  We check for that in RegExp::matchInline().  Let's use JSRegExpJITCodeFailure here instead.

> Source/JavaScriptCore/yarr/YarrJIT.h:58
> +class MatchingContextHolder {

Let's call this MatchContext.

> Source/JavaScriptCore/yarr/YarrJIT.h:109
> +    MatchResult execute(const LChar* input, unsigned start, unsigned length, int* output, MatchingContextHolder* matchingContext)

Let's have these execute functions take a MatchContext& because it is a mandatory argument and can never be null.

> Source/JavaScriptCore/yarr/YarrJIT.h:112
> +        return MatchResult(untagCFunctionPtr<YarrJITCode8, Yarr8BitPtrTag>(m_ref8.code().executableAddress())(input, start, length, output, matchingContext));

Here's we can pass &matchContext instead to the JITted code.

[Michael Saboff]

Comment on attachment 394508 [details]
Patch for Landing

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

>> JSTests/ChangeLog:9
>> +	Added a new test and removed a now obsolete test.
> 
> Fix indentation.  Can you also explain why the old test is obsolete?

Will do.

>> JSTests/stress/regexp-huge-oom.js:1
>> +// Test that throw an OOM exception when compiling / executing a pathologically large RegExp's
> 
> Should this test be skipped for memoryLimited devices?  Or will it fail fast?

I'll skip for memory limited devices.

>> Source/JavaScriptCore/runtime/RegExp.cpp:302
>> +    position = matchInline<Vector<int>&, Yarr::MatchFrom::CompilerThread>(vm, s, startOffset, ovector);
> 
> Previously, we called match() instead of matchInline() because I think we deliberately didn't want to inline it here.  matchInline is ALWAYS_INLINE.  I don't know the reason for the decision to not inline, but unless you have a reason to change this, I suggest you add a not inlined matchFromCompilerThread() that calls matchInline to preserve the previous behavior.  We also don't want to bloat the code size unnecessarily.

I call matchInline directly since it is a templated method.  Plain match() is used in many other places, all from the VM thread.  We won't save any code bloat by creating a new matchFromCompilerThread() method that does the inlining of matchInline and is only used here, in fact it will create a little more code bloat.

>> Source/JavaScriptCore/runtime/RegExp.cpp:365
>> +    result = matchInline<Yarr::MatchFrom::CompilerThread>(vm, s, startOffset);
> 
> Ditto.  Use a not inlined matchFromCompilerThread().

Ditto with my response.  This would be require a  different matchFromCompilerThread()since this variant doesn't pass a Vector reference.

>> Source/JavaScriptCore/runtime/RegExpInlines.h:133
>> +            Yarr::MatchingContextHolder regExpContext(vm, m_regExpJITCode.get(), matchFrom);
> 
> Can we call this MatchContext instead?  Then you can declare this consistently as:
>     Yarr::MatchContext matchContext(vm, m_regExpJITCode.get(), matchFrom);
> 
> I don't think "Matching" adds anything over "Match", and "Holder" doesn't add anything in addition to "Context".

I chose the verb form "Matching" over the noun form "Match" since this is used for the process of matching.  I also don't want to confuse with the unrelated MatchResult type.  The suffix "Holder" was added because this is an RAII object that keeps the MatchingContext alive while in scope.

>> Source/JavaScriptCore/yarr/YarrJIT.cpp:93
>> +    static const RegisterID matchingContext = ARM64Registers::x4;
> 
> Let's call this matchContext.

See above.

>> Source/JavaScriptCore/yarr/YarrJIT.cpp:3898
>> +            move(TrustedImm32(0), returnRegister2);
> 
> You're returning a MatchResult here.  Is -2 the start or the end?  I presume it's the start (CPU(BIG_ENDIAN) folks may have trouble ... I suggest pinging Caio as an FYI).
> 
> According to MatchResult, return WTF::notFound (i.e. -1) for start means the match failed.  Why not return WTF::notFound here?
> If there's a need for a different error code (and I suspect this is the case), let's make -2 an official error code in MatchResult and give it a name instead of using a literal here.
> 
> Wait a minute ... I see that you already have a name for this: JSRegExpJITCodeFailure.  We check for that in RegExp::matchInline().  Let's use JSRegExpJITCodeFailure here instead.

Done.

>> Source/JavaScriptCore/yarr/YarrJIT.h:58
>> +class MatchingContextHolder {
> 
> Let's call this MatchContext.

See above.

>> Source/JavaScriptCore/yarr/YarrJIT.h:109
>> +    MatchResult execute(const LChar* input, unsigned start, unsigned length, int* output, MatchingContextHolder* matchingContext)
> 
> Let's have these execute functions take a MatchContext& because it is a mandatory argument and can never be null.

Done.

[Michael Saboff]

Committed r259092: <https://trac.webkit.org/changeset/259092>