Bug 235348 – [JSC] Fix YarrJIT backtrackCharacterClassNonGreedy breakpoint

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Fix YarrJIT backtrackCharacterClassNonGreedy breakpoint

[Yusuke Suzuki]

Created attachment 449465 [details]
Patch

[Yusuke Suzuki]

rdar://84108897

[Darin Adler]

Comment on attachment 449465 [details]
Patch

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

> 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?

[Yusuke Suzuki]

Comment on attachment 449465 [details]
Patch

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

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

[Darin Adler]

Comment on attachment 449465 [details]
Patch

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

>>> 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!

[Yusuke Suzuki]

Comment on attachment 449465 [details]
Patch

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

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

[Michael Saboff]

Comment on attachment 449465 [details]
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.

[Yusuke Suzuki]

Comment on attachment 449465 [details]
Patch

Thanks!

[EWS]

Committed r288224 (246184@main): <https://commits.webkit.org/246184@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449465 [details].