[JSC] Fix YarrJIT backtrackCharacterClassNonGreedy breakpoint
Created attachment 449465 [details] Patch
rdar://84108897
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?
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).
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!
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
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.
Comment on attachment 449465 [details] Patch Thanks!
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].