Changeset 219182 in webkit

Timestamp:

Jul 5, 2017 7:31:35 PM (7 years ago)

Author:

Yusuke Suzuki

Message:

WTF::StringImpl::copyChars segfaults when built with GCC 7
​https://bugs.webkit.org/show_bug.cgi?id=173407

Reviewed by Andreas Kling.

JSTests:

• stress/string-repeat-copy-chars-crash.js: Added.

(shouldBe):

Source/WTF:

With GCC 7, StringImpl::copyChars() behaves as unexpected.
This function violates strict aliasing rule.

This optimization is originally introduced to improve performance
in SunSpider's string tests in 2008. When running it in my Linux
box, it no longer causes any observable difference. So, we just
remove this optimization.

baseline patched

string-base64 7.7544+-0.1761 7.6138+-0.2071 might be 1.0185x faster
string-fasta 10.5429+-0.2746 ? 10.7500+-0.2669 ? might be 1.0196x slower
string-tagcloud 14.8588+-0.2828 14.8039+-0.3039
string-unpack-code 36.1769+-0.4251 35.3397+-0.5398 might be 1.0237x faster
string-validate-input 8.5182+-0.2206 8.3514+-0.2179 might be 1.0200x faster

• wtf/text/StringImpl.h:

(WTF::StringImpl::copyChars):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/string-repeat-copy-chars-crash.js (added)
• Source/WTF/ChangeLog (modified) (1 diff)
• Source/WTF/wtf/text/StringImpl.h (modified) (2 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2017-07-05  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        WTF::StringImpl::copyChars segfaults when built with GCC 7
+        https://bugs.webkit.org/show_bug.cgi?id=173407
+
+        Reviewed by Andreas Kling.
+
+        * stress/string-repeat-copy-chars-crash.js: Added.
+        (shouldBe):
+
 2017-07-03  Saam Barati  <sbarati@apple.com>
 

trunk/Source/WTF/ChangeLog

@@ +1 @@
+2017-07-05  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        WTF::StringImpl::copyChars segfaults when built with GCC 7
+        https://bugs.webkit.org/show_bug.cgi?id=173407
+
+        Reviewed by Andreas Kling.
+
+        With GCC 7, StringImpl::copyChars() behaves as unexpected.
+        This function violates strict aliasing rule.
+
+        This optimization is originally introduced to improve performance
+        in SunSpider's string tests in 2008. When running it in my Linux
+        box, it no longer causes any observable difference. So, we just
+        remove this optimization.
+
+                                        baseline                  patched
+
+        string-base64                7.7544+-0.1761            7.6138+-0.2071          might be 1.0185x faster
+        string-fasta                10.5429+-0.2746     ?     10.7500+-0.2669        ? might be 1.0196x slower
+        string-tagcloud             14.8588+-0.2828           14.8039+-0.3039
+        string-unpack-code          36.1769+-0.4251           35.3397+-0.5398          might be 1.0237x faster
+        string-validate-input        8.5182+-0.2206            8.3514+-0.2179          might be 1.0200x faster
+
+        * wtf/text/StringImpl.h:
+        (WTF::StringImpl::copyChars):
+
 2017-07-05  Yusuke Suzuki  <utatane.tea@gmail.com>
 

trunk/Source/WTF/wtf/text/StringImpl.h

@@ -628 +628 @@
             return;
         }
-
-        if (numCharacters <= s_copyCharsInlineCutOff) {
-            unsigned i = 0;
-#if (CPU(X86) || CPU(X86_64))
-            const unsigned charsPerInt = sizeof(uint32_t) / sizeof(T);
-
-            if (numCharacters > charsPerInt) {
-                unsigned stopCount = numCharacters & ~(charsPerInt - 1);
-
-                const uint32_t* srcCharacters = reinterpret_cast<const uint32_t*>(source);
-                uint32_t* destCharacters = reinterpret_cast<uint32_t*>(destination);
-                for (unsigned j = 0; i < stopCount; i += charsPerInt, ++j)
-                    destCharacters[j] = srcCharacters[j];
-            }
-#endif
-            for (; i < numCharacters; ++i)
-                destination[i] = source[i];
-        } else
-            memcpy(destination, source, numCharacters * sizeof(T));
+        memcpy(destination, source, numCharacters * sizeof(T));
     }
 
@@ -860 +842 @@
     }
 
-    // This number must be at least 2 to avoid sharing empty, null as well as 1 character strings from SmallStrings.
-    static const unsigned s_copyCharsInlineCutOff = 20;
-
     enum class CaseConvertType { Upper, Lower };
     template<CaseConvertType type, typename CharacterType> static Ref<StringImpl> convertASCIICase(StringImpl&, const CharacterType*, unsigned);