[webkit-reviews] review granted: [Bug 235348] [JSC] Fix YarrJIT backtrackCharacterClassNonGreedy breakpoint : [Attachment 449465] Patch

[bugzilla-daemon at webkit.org]

Michael Saboff <msaboff at apple.com> has granted Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 235348: [JSC] Fix YarrJIT backtrackCharacterClassNonGreedy breakpoint
https://bugs.webkit.org/show_bug.cgi?id=235348

Attachment 449465: Patch

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




--- Comment #7 from Michael Saboff <msaboff at apple.com> ---
Comment on attachment 449465
  --> https://bugs.webkit.org/attachment.cgi?id=449465
Patch

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

r=me

>>>>> Source/JavaScriptCore/yarr/YarrJIT.cpp:2062
>>>>> + 	   m_jit.sub32(MacroAssembler::TrustedImm32(1), m_regs.index);
>>>> 
>>>> Do we need anything different in the m_decodeSurrogatePairs case? In that
case it might be incremented my more than one?
>>> 
>>> nonGreedyFailuresDecrementIndex is added only when m_decodeSurrogatePairs
is true. So, this path will be taken only when m_decodeSurrogatePairs is true
case. (nonGreedyFailuresDecrementIndex is added in L2052, which is executed
when m_decodeSurrogatePairs is true).
>> 
>> OK, maybe I can’t review then, because I can’t spot the add32 that this
subtract is reversing, so I suppose you need a review from a subject matter
expert. Sorry, I was hoping I could save a little time by being your reviewer!
> 
> np!!!!! Thank you for your review :D

This is safe and appropriate because we only get in this case when the
following is true (from reading L2050-2063 and L516-520):
 1) We are in the m_decodeSurrogatePairs case.
 2) We are trying to match a non-BMP character.
 3) We are at the end of the string input with the first of the two characters,
i.e. we have incremented 1.
 4) There we can't advance the index for the second character we can't read.
Therefore we need to decrement index by 1 and fall through to the failed
non-greedy match.