[JSC][32bit] Fix regexp crash when building on ARMv7 using gcc 10.2.1
Created attachment 447519 [details] Patch
Created attachment 447675 [details] Patch
Created attachment 447729 [details] Patch
Comment on attachment 447729 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=447729&action=review > Source/JavaScriptCore/yarr/YarrJIT.cpp:4204 > + m_jit.loadPtr(MacroAssembler::Address(MacroAssembler::stackPointerRegister, m_arg5OnStackOffset), matchingContext); Instead, can we use `CCallHelpers::calculatePokeOffset` to compute the right offset without computing it per architecture in YarrJIT? > Source/JavaScriptCore/yarr/YarrJIT.cpp:4685 > + unsigned m_arg5OnStackOffset = 0; For default field initializer, use `{ 0 }` instead of ` = 0`.
<rdar://problem/86904879>
Created attachment 448420 [details] Patch
Created attachment 448422 [details] Patch
Created attachment 448423 [details] Patch
Created attachment 448425 [details] Patch
Created attachment 448434 [details] Patch
Created attachment 448635 [details] Patch
Created attachment 448679 [details] Patch
Created attachment 448681 [details] Patch
Created attachment 448685 [details] Patch
Created attachment 448767 [details] Patch
Created attachment 448785 [details] Patch
Created attachment 448841 [details] Patch
Created attachment 448845 [details] Patch
Created attachment 448848 [details] Patch
Created attachment 448856 [details] Patch
Created attachment 448867 [details] Patch
Created attachment 448878 [details] Patch
Created attachment 448884 [details] Patch
Created attachment 448937 [details] Patch
Created attachment 448964 [details] Patch
Created attachment 448971 [details] Patch
Created attachment 449060 [details] Patch
Comment on attachment 449060 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449060&action=review > Source/JavaScriptCore/jit/CCallHelpers.h:1001 > + constexpr GPRReg loadArgumentFromStack(GPRReg scratchGPR) This function is not constexpr (it calls `loadPtr` which is not constexpr), nor does it need to be constexpr. Same applies to 'getArgumentGPR' > Source/JavaScriptCore/jit/CCallHelpers.h:1037 > + constexpr GPRReg getArgumentGPR(GPRReg scratchGPR = GPRReg::InvalidGPRReg) Like loadArgumentFromStack, this function is not constexpr, nor does it need to be.
Created attachment 449068 [details] Patch
Comment on attachment 449068 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449068&action=review Some coding style comments: not intended to be a complete review, although the patch generally looks good > Source/JavaScriptCore/jit/CCallHelpers.h:996 > +private: > + Normally we’d omit that blank line. > Source/JavaScriptCore/jit/CCallHelpers.h:1001 > + GPRReg loadArgumentFromStack(unsigned ArgNum, GPRReg scratchGPR) When ArgNum is just a function argument rather than a template argument, I suggest we use the traditional lowercased "argumentNumber", or "argNum" if the local tradition in this code is to use abbreviations rather than words (WebKit style generally is to prefer words). But also, why are we using a function argument rather than template argument here? Is there a good reason to mix styles like this? > Source/JavaScriptCore/jit/CCallHelpers.h:1018 > + constexpr int returnAdressOffset = 2; "address" is misspelled here > Source/JavaScriptCore/jit/CCallHelpers.h:1023 > + const int returnTypeOffset = sizeof(RESULT_TYPE) >= 8; > + const int offset = ArgNum - NUMBER_OF_ARGUMENT_REGISTERS + POKE_ARGUMENT_OFFSET + returnAdressOffset + returnTypeOffset; > +#else > + const int offset = ArgNum - NUMBER_OF_ARGUMENT_REGISTERS + POKE_ARGUMENT_OFFSET + returnAdressOffset; constexpr please But also, maybe we should define a returnTypeOffset of 0 for non-Windows-X86 rather than repeating the entire offset expression twice. > Source/JavaScriptCore/jit/CCallHelpers.h:1032 > +public: > + Here, we’d omit the blank line. > Source/JavaScriptCore/jit/CCallHelpers.h:1039 > + ASSERT(ArgNum <= 255, "C++ standard forbis functions with more than 256 arguments"); "forbids" Is misspelled here > Source/JavaScriptCore/runtime/RegExpInlines.h:149 > + Yarr::MatchingContextHolder regExpContext(vm, /* usesPatternContextBuffer: */ false, this, matchFrom); These comments are needed because we are not following the usual WebKit convention of using an enumeration for an argument like this where we often pass a constant value. That style is typically pretty easy to follow, although it does make the non-constant-value call sites a little wordy. > Source/JavaScriptCore/yarr/YarrJIT.h:276 > public: > + No blank line in a case like this. > Source/JavaScriptCore/yarr/YarrJIT.h:280 > + using YarrJITCode8 = SlowPathReturnType (*)(const LChar* input, UCPURegister start, UCPURegister length, int* output, MatchingContextHolder* matchingContext) YARR_CALL; > + using YarrJITCode16 = SlowPathReturnType (*)(const UChar* input, UCPURegister start, UCPURegister length, int* output, MatchingContextHolder* matchingContext) YARR_CALL; > + using YarrJITCodeMatchOnly8 = SlowPathReturnType (*)(const LChar* input, UCPURegister start, UCPURegister length, void*, MatchingContextHolder* matchingContext) YARR_CALL; > + using YarrJITCodeMatchOnly16 = SlowPathReturnType (*)(const UChar* input, UCPURegister start, UCPURegister length, void*, MatchingContextHolder* matchingContext) YARR_CALL; I suggests omitting the matchingContext argument name here since WebKit coding style tells us to do that when the type makes them unambiguous. > Source/JavaScriptCore/yarr/YarrJITRegisters.h:38 > public: > + Again, no blank line here in our coding style. Also, should just delete "public" since this is a struct.
Comment on attachment 449068 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449068&action=review >> Source/JavaScriptCore/jit/CCallHelpers.h:1001 >> + GPRReg loadArgumentFromStack(unsigned ArgNum, GPRReg scratchGPR) > > When ArgNum is just a function argument rather than a template argument, I suggest we use the traditional lowercased "argumentNumber", or "argNum" if the local tradition in this code is to use abbreviations rather than words (WebKit style generally is to prefer words). > > But also, why are we using a function argument rather than template argument here? Is there a good reason to mix styles like this? In the previous patch, ArgNum was a template argument but I revert as suggested in the previous review. >> Source/JavaScriptCore/jit/CCallHelpers.h:1023 >> + const int offset = ArgNum - NUMBER_OF_ARGUMENT_REGISTERS + POKE_ARGUMENT_OFFSET + returnAdressOffset; > > constexpr please > > But also, maybe we should define a returnTypeOffset of 0 for non-Windows-X86 rather than repeating the entire offset expression twice. I'll make ArgNum a template argument otherwise the compiler won't allow this to be a constexpr (it complains that ArgNum is not a constant).
Created attachment 449091 [details] Patch
Created attachment 449108 [details] Patch
Comment on attachment 449108 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449108&action=review > Source/JavaScriptCore/jit/CCallHelpers.h:1003 > + ASSERT(ArgNum >= NUMBER_OF_ARGUMENT_REGISTERS, "Should return GPRInfo::argumentGPRX"); This can/should be a static_assert. > Source/JavaScriptCore/jit/CCallHelpers.h:1037 > + ASSERT(ArgNum <= 255, "C++ standard forbids functions with more than 256 arguments"); This can/should be a static_assert. > Source/JavaScriptCore/jit/CCallHelpers.h:1038 > + ASSERT(FunctionTraits<OperationType>::cCallArity() > ArgNum, "Function does not have enough arguments"); This can/should be a static_assert.
Created attachment 449195 [details] Patch
Comment on attachment 449195 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449195&action=review I think the updated code is breaking JITCage. Can you please do not change ARM64 / ARM64E code? > Source/JavaScriptCore/yarr/YarrJIT.cpp:-3996 > - if (!Options::useJITCage()) > - m_jit.tagReturnAddress(); This part is broken. You should not tag when JITCage is not enabled. But updated code is not following. > Source/JavaScriptCore/yarr/YarrJIT.cpp:4048 > + m_jit.untagReturnAddress(); Why is it added? > Source/JavaScriptCore/yarr/YarrJIT.cpp:4183 > + constexpr unsigned ArgNum = 4; We use `argNum` (instead of `ArgNum`) for constants. > Source/JavaScriptCore/yarr/YarrJITRegisters.h:-75 > - static constexpr GPRReg supplementaryPlanesBase = ARM64Registers::x12; > - static constexpr GPRReg leadingSurrogateTag = ARM64Registers::x13; > - static constexpr GPRReg trailingSurrogateTag = ARM64Registers::x14; I think using register is better for code size. Please do not change this.
Comment on attachment 449195 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449195&action=review >> Source/JavaScriptCore/yarr/YarrJIT.cpp:-3996 >> - m_jit.tagReturnAddress(); > > This part is broken. You should not tag when JITCage is not enabled. But updated code is not following. I see, the problem is that emitFunctionPrologue() always calls tagReturnAddress(). I'll special-case ARM64 then and revert to the original code. >> Source/JavaScriptCore/yarr/YarrJIT.cpp:4048 >> + m_jit.untagReturnAddress(); > > Why is it added? Suggested by a colleague, I'll remove it.
Created attachment 449208 [details] Patch
Comment on attachment 449208 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449208&action=review > Source/JavaScriptCore/yarr/YarrJIT.cpp:3963 > +#if !CPU(ARM64) > + m_jit.emitFunctionPrologue(); > +#endif Why is this change necessary? It looks like rather it is introducing complication. > Source/JavaScriptCore/yarr/YarrJIT.cpp:4062 > + m_jit.emitFunctionEpilogue(); indentation is not correct. And why do we need this modification?
Yusuke: 'emitFunctionPrologue' always tags the return address. We did not use to use this, but getting to the stacked arguments consistently without the frame pointer setup is a pain. So we now call 'emitFunctionPrologue', always. To do the tail call when the JITCage is used, we simply untag prior to the tail call. Should have the same effect.
Comment on attachment 449208 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449208&action=review >> Source/JavaScriptCore/yarr/YarrJIT.cpp:3963 >> +#endif > > Why is this change necessary? It looks like rather it is introducing complication. We need it so the frame pointer is properly set up in a way that is independent of the arch. The function loadArgumentFromStack needs the frame pointer to be set or it will fail to load the argument.
(In reply to Mikhail R. Gadelha from comment #41) > Comment on attachment 449208 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=449208&action=review > > >> Source/JavaScriptCore/yarr/YarrJIT.cpp:3963 > >> +#endif > > > > Why is this change necessary? It looks like rather it is introducing complication. > > We need it so the frame pointer is properly set up in a way that is > independent of the arch. The function loadArgumentFromStack needs the frame > pointer to be set or it will fail to load the argument. Why cant’ we use stack pointer here? If we count # of pushes in prologue (as suggested in https://webkit.slack.com/archives/CTV4FGWF4/p1640206561238000?thread_ts=1640112033.226400&cid=CTV4FGWF4), we can use stack pointer to get the argument.
(In reply to Yusuke Suzuki from comment #42) > (In reply to Mikhail R. Gadelha from comment #41) > > Comment on attachment 449208 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=449208&action=review > > > > >> Source/JavaScriptCore/yarr/YarrJIT.cpp:3963 > > >> +#endif > > > > > > Why is this change necessary? It looks like rather it is introducing complication. > > > > We need it so the frame pointer is properly set up in a way that is > > independent of the arch. The function loadArgumentFromStack needs the frame > > pointer to be set or it will fail to load the argument. > > Why cant’ we use stack pointer here? If we count # of pushes in prologue (as > suggested in > https://webkit.slack.com/archives/CTV4FGWF4/ > p1640206561238000?thread_ts=1640112033.226400&cid=CTV4FGWF4), we can use > stack pointer to get the argument. We can use it but (In reply to Yusuke Suzuki from comment #42) > (In reply to Mikhail R. Gadelha from comment #41) > > Comment on attachment 449208 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=449208&action=review > > > > >> Source/JavaScriptCore/yarr/YarrJIT.cpp:3963 > > >> +#endif > > > > > > Why is this change necessary? It looks like rather it is introducing complication. > > > > We need it so the frame pointer is properly set up in a way that is > > independent of the arch. The function loadArgumentFromStack needs the frame > > pointer to be set or it will fail to load the argument. > > Why cant’ we use stack pointer here? If we count # of pushes in prologue (as > suggested in > https://webkit.slack.com/archives/CTV4FGWF4/ > p1640206561238000?thread_ts=1640112033.226400&cid=CTV4FGWF4), we can use > stack pointer to get the argument. It just (In reply to Yusuke Suzuki from comment #42) > (In reply to Mikhail R. Gadelha from comment #41) > > Comment on attachment 449208 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=449208&action=review > > > > >> Source/JavaScriptCore/yarr/YarrJIT.cpp:3963 > > >> +#endif > > > > > > Why is this change necessary? It looks like rather it is introducing complication. > > > > We need it so the frame pointer is properly set up in a way that is > > independent of the arch. The function loadArgumentFromStack needs the frame > > pointer to be set or it will fail to load the argument. > > Why cant’ we use stack pointer here? If we count # of pushes in prologue (as > suggested in > https://webkit.slack.com/archives/CTV4FGWF4/ > p1640206561238000?thread_ts=1640112033.226400&cid=CTV4FGWF4), we can use > stack pointer to get the argument. We can but the current approach seems more robust because we have an arch-independent way of getting what we want: we simply need to set up frame pointer in the function prologue. The patch that counts the pushes looks worse IMHO (ignore the code not in generateEnter()): https://bugs.webkit.org/attachment.cgi?id=448434&action=review
(In reply to Mikhail R. Gadelha from comment #43) > > We can but the current approach seems more robust because we have an > arch-independent way of getting what we want: we simply need to set up frame > pointer in the function prologue. > > The patch that counts the pushes looks worse IMHO (ignore the code not in > generateEnter()): > https://bugs.webkit.org/attachment.cgi?id=448434&action=review But this is adding unnecessary tagging and untagging, which can affect on, 1. machine code generation size 2. performance since RegExp code is tiny. So, even it is more robust, I don't think this is OK since it hurts performance.
Comment on attachment 449208 [details] Patch So, since this is regressing ARM64 code, putting r-.
Created attachment 449345 [details] Patch
Created attachment 449450 [details] Patch
Created attachment 449495 [details] Patch
Comment on attachment 449495 [details] Patch r=me
Committed r288400 (246291@main): <https://commits.webkit.org/246291@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 449495 [details].
Re-opened since this is blocked by bug 235470
Reverted since it broke ARM64E build. According to Peng, the error is the following. /Volumes/Data/webkit/OpenSource/Source/JavaScriptCore/yarr/YarrJIT.h:335:32: error: no matching function for call to 'vmEntryToYarrJIT' return MatchResult(vmEntryToYarrJIT(input, start, length, output, &matchingContext, retagCodePtr<Yarr8BitPtrTag, YarrEntryPtrTag>(m_ref8.code().executableAddress())));
Created attachment 449825 [details] Patch
Comment on attachment 449825 [details] Patch r=me, nice.
Committed r288476 (246359@main): <https://commits.webkit.org/246359@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 449825 [details].