[webkit-reviews] review granted: [Bug 180538] YARR: JIT RegExps with greedy parenthesized sub patterns : [Attachment 328782] Fixed test failure.

[bugzilla-daemon at webkit.org]

JF Bastien <jfbastien at apple.com> has granted Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 180538: YARR: JIT RegExps with greedy parenthesized sub patterns
https://bugs.webkit.org/show_bug.cgi?id=180538

Attachment 328782: Fixed test failure.

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




--- Comment #6 from JF Bastien <jfbastien at apple.com> ---
Comment on attachment 328782
  --> https://bugs.webkit.org/attachment.cgi?id=328782
Fixed test failure.

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

r=me

> Source/JavaScriptCore/testRegExp.cpp:438
> +	   const char* regexpError;

Seems less error-prone to set to nullptr here, instead of in parseRegExpLine.

> Source/JavaScriptCore/yarr/YarrJIT.cpp:187
> +	       return sizeof(ParenContext) + sizeof(Subpatterns) *
parenContextSizes.numSubpatterns() + sizeof(uintptr_t) *
parenContextSizes.frameSlots();

Do we cap these enough so that they can't overflow? We deal with 32-bit values
above, so even if this is size_t (because of sizeof) I can't convince myself we
can't overflow.

> Source/JavaScriptCore/yarr/YarrJIT.cpp:263
> +	       for (unsigned subpattern = firstSubpattern; subpattern <=
lastSubpattern; subpattern++) {

subpattern <= lastSubpattern range seems odd! I guess it's correct since it's
used below as well?

> Source/JavaScriptCore/yarr/YarrJIT.cpp:283
> +	       for (unsigned subpattern = firstSubpattern; subpattern <=
lastSubpattern; subpattern++) {

(here)

> Source/JavaScriptCore/yarr/YarrJIT.cpp:289
> +	   for (unsigned frameLocation = subpatternBaseFrameLocation;
frameLocation < m_parenContextSizes.frameSlots(); frameLocation++) {

Versus closed range here :-)

> Source/JavaScriptCore/yarr/YarrJIT.cpp:2218
> +		   ASSERT_NOT_REACHED();

RELEASE_ ?

> Source/JavaScriptCore/yarr/YarrJIT.cpp:2276
> +		   ASSERT_NOT_REACHED();

RELEASE_ ?

> Source/JavaScriptCore/yarr/YarrJIT.cpp:3421
> +	       move(TrustedImm32(1000000), remainingMatchCount);

matchLimit from Yarr.h instead?

> Source/JavaScriptCore/yarr/YarrJIT.h:43
> +const size_t patternContextBufferSize = 8192; // Space caller allocates to
save nested parenthesis context

I would make this constexpr so it clearly doesn't change, since you use it in a
variable-sized array.