Looks like adding the YARR JIT 8K buffer as a stack variable in https://trac.webkit.org/changeset/225695/webkit (relanded in https://trac.webkit.org/changeset/225930/webkit) causes an out of stack crash. This buffer should be moved to the owning VM. Crashes look similar to: Thread 0 Crashed ↩:: Dispatch queue: com.apple.main-thread 0 libsystem_kernel.dylib 0x00007fff5c5f4afe __pthread_kill + 10 1 libsystem_pthread.dylib 0x00007fff5c7b3150 pthread_kill + 333 2 libsystem_c.dylib 0x00007fff5c55024d __abort + 144 3 libsystem_c.dylib 0x00007fff5c550af8 __stack_chk_fail + 205 4 com.apple.JavaScriptCore 0x00007fff370da396 JSC::RegExp::match(JSC::VM&, WTF::String const&, unsigned int) + 630 5 com.apple.JavaScriptCore 0x00007fff37b1929b JSC::RegExpObject::matchInline(JSC::ExecState*, JSC::JSGlobalObject*, JSC::JSString*) + 235 6 com.apple.JavaScriptCore 0x00007fff37b1cd6b JSC::regExpProtoFuncTestFast(JSC::ExecState*) + 267 7 ??? 0x00005db07fa01178 0 + 103012636823928 8 ??? 0x00005db08054fc08 0 + 103012648680456 9 ??? 0x00005db08054e42e 0 + 103012648674350 10 ??? 0x00005db08055139b 0 + 103012648686491 11 ??? 0x00005db0803e53c2 0 + 103012647195586 12 ??? 0x00005db0805271d3 0 + 103012648514003 13 ??? 0x00005db0804f1c31 0 + 103012648295473 14 com.apple.JavaScriptCore 0x00007fff372384be llint_entry + 28860 ...
<rdar://problem/36940735>
Created attachment 333718 [details] Patch
Comment on attachment 333718 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333718&action=review r=me with suggested improvements. > Source/JavaScriptCore/runtime/RegExpInlines.h:122 > + if (m_regExpJITCode.usesPatternContextBuffer()) { > + patternContextBuffer = vm.acquireRegExpPatternContexBuffer(); > + patternContextBufferSize = VM::patternContextBufferSize; > + } > #define EXTRA_JIT_PARAMS , patternContextBuffer, patternContextBufferSize This seems like a prime candidate for using the RAII pattern. How about something like this: RegExpPatternContextBuffer buffer(vm); #define EXTRA_JIT_PARAMS , buffer.buffer(), buffer.size() You can either make RegExpPatternContextBuffer an embedded class in VM, or make it a friend of VM so that it can do the real work. > Source/JavaScriptCore/runtime/RegExpInlines.h:135 > +#if ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS) > + if (m_regExpJITCode.usesPatternContextBuffer()) > + vm.releaseRegExpPatternContexBuffer(); > +#endif This can be handled automatically by the RAII object. > Source/JavaScriptCore/yarr/YarrJIT.h:102 > +#endif > + > +#if ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS) Why is this needed?
(In reply to Mark Lam from comment #3) > Comment on attachment 333718 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=333718&action=review > > r=me with suggested improvements. > > > Source/JavaScriptCore/runtime/RegExpInlines.h:122 > > + if (m_regExpJITCode.usesPatternContextBuffer()) { > > + patternContextBuffer = vm.acquireRegExpPatternContexBuffer(); > > + patternContextBufferSize = VM::patternContextBufferSize; > > + } > > #define EXTRA_JIT_PARAMS , patternContextBuffer, patternContextBufferSize > > This seems like a prime candidate for using the RAII pattern. How about > something like this: > > RegExpPatternContextBuffer buffer(vm); > #define EXTRA_JIT_PARAMS , buffer.buffer(), buffer.size() Better yet, just pass a reference to the RAII object: #define EXTRA_JIT_PARAMS , buffer
Created attachment 333813 [details] Patch updated after reviewer's comments
(In reply to Mark Lam from comment #3) > Comment on attachment 333718 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=333718&action=review > > r=me with suggested improvements. > > > Source/JavaScriptCore/runtime/RegExpInlines.h:122 > > + if (m_regExpJITCode.usesPatternContextBuffer()) { > > + patternContextBuffer = vm.acquireRegExpPatternContexBuffer(); > > + patternContextBufferSize = VM::patternContextBufferSize; > > + } > > #define EXTRA_JIT_PARAMS , patternContextBuffer, patternContextBufferSize > > This seems like a prime candidate for using the RAII pattern. How about > something like this: > > RegExpPatternContextBuffer buffer(vm); > #define EXTRA_JIT_PARAMS , buffer.buffer(), buffer.size() > > You can either make RegExpPatternContextBuffer an embedded class in VM, or > make it a friend of VM so that it can do the real work. I added a RAII class to RegExpInlines.h. > > Source/JavaScriptCore/runtime/RegExpInlines.h:135 > > +#if ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS) > > + if (m_regExpJITCode.usesPatternContextBuffer()) > > + vm.releaseRegExpPatternContexBuffer(); > > +#endif > > This can be handled automatically by the RAII object. > > > Source/JavaScriptCore/yarr/YarrJIT.h:102 > > +#endif > > + > > +#if ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS) > > Why is this needed? Removed.
Attachment 333813 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/RegExpInlines.h:147: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Source/JavaScriptCore/runtime/RegExpInlines.h:257: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 333813 [details] Patch updated after reviewer's comments r=me if EWS bots show no issues.
Comment on attachment 333813 [details] Patch updated after reviewer's comments Clearing flags on attachment: 333813 Committed r228481: <https://trac.webkit.org/changeset/228481>
All reviewed patches have been landed. Closing bug.