Attachment 370294 Details for Bug 198063 – Patch

[patch] Patch

bug-198063-20190520194746.patch (text/plain), 15.59 KB, created by Keith Miller on 2019-05-20 19:47:49 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Keith Miller

Created: 2019-05-20 19:47:49 PDT

Size: 15.59 KB

patch

obsolete

>Subversion Revision: 245536
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 51815ba8a61c41ced21fea0c08ddf6de4bb57687..4256842695b846304fb883774ed69dfda3edbd16 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,28 @@
>+2019-05-20  Keith Miller  <keith_miller@apple.com>
>+
>+        Cleanup Yarr regexp code around paren contexts.
>+        https://bugs.webkit.org/show_bug.cgi?id=198063
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        There are three refactoring changes around paren contexts:
>+        1. Make EncodedMatchResult the same type as MatchResult on X86_64 and arm64 and uint64_t elsewhere.
>+        2. All function pointer types for Yarr JIT generated code reserve space for paren contexts.
>+        3. initParenContextFreeList should bail based on VM::patternContextBufferSize as that's the buffer size anyway.
>+
>+        * runtime/MatchResult.h:
>+        (JSC::MatchResult::MatchResult):
>+        * runtime/RegExpInlines.h:
>+        (JSC::PatternContextBufferHolder::PatternContextBufferHolder):
>+        (JSC::PatternContextBufferHolder::~PatternContextBufferHolder):
>+        (JSC::PatternContextBufferHolder::size):
>+        (JSC::RegExp::matchInline):
>+        * runtime/VM.h:
>+        * yarr/YarrJIT.cpp:
>+        (JSC::Yarr::YarrGenerator::initParenContextFreeList):
>+        * yarr/YarrJIT.h:
>+        (JSC::Yarr::YarrCodeBlock::execute):
>+
> 2019-05-20  Ross Kirsling  <ross.kirsling@sony.com>
> 
>         [WinCairo] Implement Remote Web Inspector Client.
>diff --git a/Source/JavaScriptCore/runtime/MatchResult.h b/Source/JavaScriptCore/runtime/MatchResult.h
>index ecee4e9dafea0f77c94a327e3d1a456e520d743a..c4aecaac76a41f94e1849d55c52f79392f1dd7c1 100644
>--- a/Source/JavaScriptCore/runtime/MatchResult.h
>+++ b/Source/JavaScriptCore/runtime/MatchResult.h
>@@ -30,7 +30,12 @@
> 
> namespace JSC {
> 
>-typedef uint64_t EncodedMatchResult;
>+struct MatchResult;
>+#if CPU(ARM64) || CPU(X86_64)
>+using EncodedMatchResult = MatchResult;
>+#else
>+using EncodedMatchResult = uint64_t;
>+#endif
> 
> struct MatchResult {
>     MatchResult()
>@@ -45,19 +50,13 @@ struct MatchResult {
>     {
>     }
> 
>-    explicit ALWAYS_INLINE MatchResult(EncodedMatchResult encoded)
>+#if !(CPU(ARM64) || CPU(X86_64))
>+    ALWAYS_INLINE MatchResult(EncodedMatchResult match)
>+        : start(bitwise_cast<MatchResult>(match).start)
>+        , end(bitwise_cast<MatchResult>(match).end)
>     {
>-        union u {
>-            uint64_t encoded;
>-            struct s {
>-                size_t start;
>-                size_t end;
>-            } split;
>-        } value;
>-        value.encoded = encoded;
>-        start = value.split.start;
>-        end = value.split.end;
>     }
>+#endif
> 
>     ALWAYS_INLINE static MatchResult failed()
>     {
>@@ -80,4 +79,6 @@ struct MatchResult {
>     size_t end;
> };
> 
>+static_assert(sizeof(MatchResult) == sizeof(EncodedMatchResult), "Match result and EncodedMatchResult should be the same size");
>+
> } // namespace JSC
>diff --git a/Source/JavaScriptCore/runtime/RegExpInlines.h b/Source/JavaScriptCore/runtime/RegExpInlines.h
>index ddd9bc413d2138be7f725bbbc17953eb3f774b70..3a2e8a1ec58eae742ea11522681ec4ae9147c126 100644
>--- a/Source/JavaScriptCore/runtime/RegExpInlines.h
>+++ b/Source/JavaScriptCore/runtime/RegExpInlines.h
>@@ -86,38 +86,38 @@ ALWAYS_INLINE bool RegExp::hasCodeFor(Yarr::YarrCharSize charSize)
>     return false;
> }
> 
>-#if ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS)
> class PatternContextBufferHolder {
> public:
>     PatternContextBufferHolder(VM& vm, bool needBuffer)
>         : m_vm(vm)
>         , m_needBuffer(needBuffer)
>     {
>-        if (m_needBuffer) {
>+#if ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS)
>+        if (m_needBuffer)
>             m_buffer = m_vm.acquireRegExpPatternContexBuffer();
>-            m_size = VM::patternContextBufferSize;
>-        } else {
>-            m_buffer = nullptr;
>-            m_size = 0;
>-        }
>+#endif
>+
>     }
> 
>     ~PatternContextBufferHolder()
>     {
>+#if ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS)
>         if (m_needBuffer)
>             m_vm.releaseRegExpPatternContexBuffer();
>+#else
>+        UNUSED_PARAM(m_vm);
>+        UNUSED_PARAM(m_needBuffer);
>+#endif
>     }
> 
>     void* buffer() { return m_buffer; }
>-    unsigned size() { return m_size; }
>+    unsigned size() { return m_buffer ? VM::patternContextBufferSize : 0; }
> 
> private:
>     VM& m_vm;
>     bool m_needBuffer;
>     void* m_buffer;
>-    unsigned m_size;
> };
>-#endif
> 
> ALWAYS_INLINE void RegExp::compileIfNecessary(VM& vm, Yarr::YarrCharSize charSize)
> {
>@@ -158,20 +158,12 @@ ALWAYS_INLINE int RegExp::matchInline(VM& vm, const String& s, unsigned startOff
>     if (m_state == JITCode) {
>         {
>             ASSERT(m_regExpJITCode);
>-#if ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS)
>             PatternContextBufferHolder patternContextBufferHolder(vm, m_regExpJITCode->usesPatternContextBuffer());
> 
>-#define EXTRA_JIT_PARAMS  , patternContextBufferHolder.buffer(), patternContextBufferHolder.size()
>-#else
>-#define EXTRA_JIT_PARAMS
>-#endif
>-
>             if (s.is8Bit())
>-                result = m_regExpJITCode->execute(s.characters8(), startOffset, s.length(), offsetVector EXTRA_JIT_PARAMS).start;
>+                result = m_regExpJITCode->execute(s.characters8(), startOffset, s.length(), offsetVector, patternContextBufferHolder.buffer(), patternContextBufferHolder.size()).start;
>             else
>-                result = m_regExpJITCode->execute(s.characters16(), startOffset, s.length(), offsetVector EXTRA_JIT_PARAMS).start;
>-
>-#undef EXTRA_JIT_PARAMS
>+                result = m_regExpJITCode->execute(s.characters16(), startOffset, s.length(), offsetVector, patternContextBufferHolder.buffer(), patternContextBufferHolder.size()).start;
>         }
> 
>         if (result == Yarr::JSRegExpJITCodeFailure) {
>@@ -284,20 +276,11 @@ ALWAYS_INLINE MatchResult RegExp::matchInline(VM& vm, const String& s, unsigned
>     if (m_state == JITCode) {
>         {
>             ASSERT(m_regExpJITCode);
>-#if ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS)
>             PatternContextBufferHolder patternContextBufferHolder(vm, m_regExpJITCode->usesPatternContextBuffer());
>-
>-#define EXTRA_JIT_PARAMS  , patternContextBufferHolder.buffer(), patternContextBufferHolder.size()
>-#else
>-#define EXTRA_JIT_PARAMS
>-#endif
>-
>             if (s.is8Bit())
>-                result = m_regExpJITCode->execute(s.characters8(), startOffset, s.length() EXTRA_JIT_PARAMS);
>+                result = m_regExpJITCode->execute(s.characters8(), startOffset, s.length(), patternContextBufferHolder.buffer(), patternContextBufferHolder.size());
>             else
>-                result = m_regExpJITCode->execute(s.characters16(), startOffset, s.length() EXTRA_JIT_PARAMS);
>-
>-#undef EXTRA_JIT_PARAMS
>+                result = m_regExpJITCode->execute(s.characters16(), startOffset, s.length(), patternContextBufferHolder.buffer(), patternContextBufferHolder.size());
>         }
> 
> #if ENABLE(REGEXP_TRACING)
>diff --git a/Source/JavaScriptCore/runtime/VM.h b/Source/JavaScriptCore/runtime/VM.h
>index b985d47ea4d95edddc9c0cf36689653376f7d1df..501a37d6c2034f35ffcf90bb62ff334f3f0596f6 100644
>--- a/Source/JavaScriptCore/runtime/VM.h
>+++ b/Source/JavaScriptCore/runtime/VM.h
>@@ -799,6 +799,8 @@ public:
>     Lock m_regExpPatternContextLock;
>     char* acquireRegExpPatternContexBuffer();
>     void releaseRegExpPatternContexBuffer();
>+#else
>+    static constexpr size_t patternContextBufferSize = 0; // Space allocated to save nested parenthesis context
> #endif
> 
>     Ref<CompactVariableMap> m_compactVariableMap;
>diff --git a/Source/JavaScriptCore/yarr/YarrJIT.cpp b/Source/JavaScriptCore/yarr/YarrJIT.cpp
>index a9b5834d96f12430c0abc0fbfcd4e8ddd0cd7104..a81c88b66d09d8e324459a2bcf762ab2f3840b4a 100644
>--- a/Source/JavaScriptCore/yarr/YarrJIT.cpp
>+++ b/Source/JavaScriptCore/yarr/YarrJIT.cpp
>@@ -229,7 +229,7 @@ class YarrGenerator : public YarrJITInfo, private MacroAssembler {
>         parenContextSize = WTF::roundUpToMultipleOf<sizeof(uintptr_t)>(parenContextSize);
> 
>         // Check that the paren context is a reasonable size.
>-        if (parenContextSize > INT16_MAX)
>+        if (parenContextSize > VM::patternContextBufferSize)
>             m_abortExecution.append(jump());
> 
>         Jump emptyFreeList = branchTestPtr(Zero, freelistRegister);
>diff --git a/Source/JavaScriptCore/yarr/YarrJIT.h b/Source/JavaScriptCore/yarr/YarrJIT.h
>index 7a30035d5dc0a4755645535e57c4b95e4080deca..5b58f06fe3a810c9c003d26d208971346ffbd363 100644
>--- a/Source/JavaScriptCore/yarr/YarrJIT.h
>+++ b/Source/JavaScriptCore/yarr/YarrJIT.h
>@@ -38,10 +38,6 @@
> #define YARR_CALL
> #endif
> 
>-#if ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS)
>-constexpr size_t patternContextBufferSize = 8192; // Space caller allocates to save nested parenthesis context
>-#endif
>-
> namespace JSC {
> 
> class VM;
>@@ -61,24 +57,11 @@ enum class JITFailureReason : uint8_t {
> };
> 
> class YarrCodeBlock {
>-#if CPU(X86_64) || CPU(ARM64)
>-#if ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS)
>-    typedef MatchResult (*YarrJITCode8)(const LChar* input, unsigned start, unsigned length, int* output, void* freeParenContext, unsigned parenContextSize) YARR_CALL;
>-    typedef MatchResult (*YarrJITCode16)(const UChar* input, unsigned start, unsigned length, int* output, void* freeParenContext, unsigned parenContextSize) YARR_CALL;
>-    typedef MatchResult (*YarrJITCodeMatchOnly8)(const LChar* input, unsigned start, unsigned length, void*, void* freeParenContext, unsigned parenContextSize) YARR_CALL;
>-    typedef MatchResult (*YarrJITCodeMatchOnly16)(const UChar* input, unsigned start, unsigned length, void*, void* freeParenContext, unsigned parenContextSize) YARR_CALL;
>-#else
>-    typedef MatchResult (*YarrJITCode8)(const LChar* input, unsigned start, unsigned length, int* output) YARR_CALL;
>-    typedef MatchResult (*YarrJITCode16)(const UChar* input, unsigned start, unsigned length, int* output) YARR_CALL;
>-    typedef MatchResult (*YarrJITCodeMatchOnly8)(const LChar* input, unsigned start, unsigned length) YARR_CALL;
>-    typedef MatchResult (*YarrJITCodeMatchOnly16)(const UChar* input, unsigned start, unsigned length) YARR_CALL;
>-#endif
>-#else
>-    typedef EncodedMatchResult (*YarrJITCode8)(const LChar* input, unsigned start, unsigned length, int* output) YARR_CALL;
>-    typedef EncodedMatchResult (*YarrJITCode16)(const UChar* input, unsigned start, unsigned length, int* output) YARR_CALL;
>-    typedef EncodedMatchResult (*YarrJITCodeMatchOnly8)(const LChar* input, unsigned start, unsigned length) YARR_CALL;
>-    typedef EncodedMatchResult (*YarrJITCodeMatchOnly16)(const UChar* input, unsigned start, unsigned length) YARR_CALL;
>-#endif
>+    // Technically freeParenContext and parenContextSize are only used if ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS) is set. Fortunately, all the calling conventions we supporthave caller save argument registers.
>+    using YarrJITCode8 = EncodedMatchResult (*)(const LChar* input, unsigned start, unsigned length, int* output, void* freeParenContext, unsigned parenContextSize) YARR_CALL;
>+    using YarrJITCode16 = EncodedMatchResult (*)(const UChar* input, unsigned start, unsigned length, int* output, void* freeParenContext, unsigned parenContextSize) YARR_CALL;
>+    using YarrJITCodeMatchOnly8 = EncodedMatchResult (*)(const LChar* input, unsigned start, unsigned length, void*, void* freeParenContext, unsigned parenContextSize) YARR_CALL;
>+    using YarrJITCodeMatchOnly16 = EncodedMatchResult (*)(const UChar* input, unsigned start, unsigned length, void*, void* freeParenContext, unsigned parenContextSize) YARR_CALL;
> 
> public:
>     YarrCodeBlock() = default;
>@@ -96,9 +79,10 @@ public:
>     void set8BitCodeMatchOnly(MacroAssemblerCodeRef<YarrMatchOnly8BitPtrTag> matchOnly) { m_matchOnly8 = matchOnly; }
>     void set16BitCodeMatchOnly(MacroAssemblerCodeRef<YarrMatchOnly16BitPtrTag> matchOnly) { m_matchOnly16 = matchOnly; }
> 
>-#if ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS)
>     bool usesPatternContextBuffer() { return m_usesPatternContextBuffer; }
>+#if ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS)
>     void setUsesPatternContextBuffer() { m_usesPatternContextBuffer = true; }
>+#endif
> 
>     MatchResult execute(const LChar* input, unsigned start, unsigned length, int* output, void* freeParenContext, unsigned parenContextSize)
>     {
>@@ -123,31 +107,6 @@ public:
>         ASSERT(has16BitCodeMatchOnly());
>         return MatchResult(untagCFunctionPtr<YarrJITCodeMatchOnly16, YarrMatchOnly16BitPtrTag>(m_matchOnly16.code().executableAddress())(input, start, length, 0, freeParenContext, parenContextSize));
>     }
>-#else
>-    MatchResult execute(const LChar* input, unsigned start, unsigned length, int* output)
>-    {
>-        ASSERT(has8BitCode());
>-        return MatchResult(reinterpret_cast<YarrJITCode8>(m_ref8.code().executableAddress())(input, start, length, output));
>-    }
>-
>-    MatchResult execute(const UChar* input, unsigned start, unsigned length, int* output)
>-    {
>-        ASSERT(has16BitCode());
>-        return MatchResult(reinterpret_cast<YarrJITCode16>(m_ref16.code().executableAddress())(input, start, length, output));
>-    }
>-
>-    MatchResult execute(const LChar* input, unsigned start, unsigned length)
>-    {
>-        ASSERT(has8BitCodeMatchOnly());
>-        return MatchResult(reinterpret_cast<YarrJITCodeMatchOnly8>(m_matchOnly8.code().executableAddress())(input, start, length));
>-    }
>-
>-    MatchResult execute(const UChar* input, unsigned start, unsigned length)
>-    {
>-        ASSERT(has16BitCodeMatchOnly());
>-        return MatchResult(reinterpret_cast<YarrJITCodeMatchOnly16>(m_matchOnly16.code().executableAddress())(input, start, length));
>-    }
>-#endif
> 
> #if ENABLE(REGEXP_TRACING)
>     void *get8BitMatchOnlyAddr()
>@@ -202,9 +161,7 @@ private:
>     MacroAssemblerCodeRef<Yarr16BitPtrTag> m_ref16;
>     MacroAssemblerCodeRef<YarrMatchOnly8BitPtrTag> m_matchOnly8;
>     MacroAssemblerCodeRef<YarrMatchOnly16BitPtrTag> m_matchOnly16;
>-#if ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS)
>-    bool m_usesPatternContextBuffer;
>-#endif
>+    bool m_usesPatternContextBuffer { false };
>     Optional<JITFailureReason> m_failureReason;
> };
> 
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index 0daf3b1294cafcfd0e516766b2879c85a122e0d5..855afc6cf3924307cc8eca2fabad98229a8d7332 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,14 @@
>+2019-05-20  Keith Miller  <keith_miller@apple.com>
>+
>+        Cleanup Yarr regexp code around paren contexts.
>+        https://bugs.webkit.org/show_bug.cgi?id=198063
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * stress/regexp-many-named-sequential-capture-groups.js: Added.
>+        (i.s):
>+        * stress/regexp-many-unnamed-sequential-capture-groups.js: Added.
>+
> 2019-05-17  Justin Michaud  <justin_michaud@apple.com>
> 
>         [WASM-References] Add support for Anyref in parameters and return types, Ref.null and Ref.is_null for Anyref values.
>diff --git a/JSTests/stress/regexp-many-named-sequential-capture-groups.js b/JSTests/stress/regexp-many-named-sequential-capture-groups.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..a8b8c71970bd0c24ef0beef3b7f887fb30dd1c91
>--- /dev/null
>+++ b/JSTests/stress/regexp-many-named-sequential-capture-groups.js
>@@ -0,0 +1,8 @@
>+let s = '';
>+for (let i = 0; i < 1000; i++) {
>+    s += `(?<foo${i}>a){0,2}`;
>+}
>+
>+let r = new RegExp(s);
>+for (let i = 0; i < 1000; i++)
>+    ''.match(r);
>diff --git a/JSTests/stress/regexp-many-unnamed-sequential-capture-groups.js b/JSTests/stress/regexp-many-unnamed-sequential-capture-groups.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..563e6fb3979f0d7c795dbaf4fd543eda909871d5
>--- /dev/null
>+++ b/JSTests/stress/regexp-many-unnamed-sequential-capture-groups.js
>@@ -0,0 +1,8 @@
>+let s = '';
>+for (let i = 0; i < 1000; i++) {
>+    s += '(?:a){0,2}';
>+}
>+
>+let r = new RegExp(s);
>+for (let i = 0; i < 1000; i++)
>+    ''.match(r);

Flags:

ysuzuki: review+
ews-watchlist: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 198063: 370294 | 370301