[webkit-reviews] review granted: [Bug 209435] Refactor YARR Stack Overflow checks : [Attachment 394508] Patch for Landing

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Michael Saboff <msaboff at apple.com>'s
request for review:
Bug 209435: Refactor YARR Stack Overflow checks
https://bugs.webkit.org/show_bug.cgi?id=209435

Attachment 394508: Patch for Landing

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




--- Comment #7 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 394508
  --> https://bugs.webkit.org/attachment.cgi?id=394508
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.