Attachment 347455 Details for Bug 188716 – Patch

[patch] Patch

bug-188716-20180819172150.patch (text/plain), 17.97 KB, created by Yusuke Suzuki on 2018-08-19 01:21:51 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2018-08-19 01:21:51 PDT

Size: 17.97 KB

patch

obsolete

>Subversion Revision: 235015
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index a474cc17276ad560cf824150f0ef1a067665f15e..7af575f9acb5406946cbf959364cf9b61149e604 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,31 @@
>+2018-08-19  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
>+
>+        [WTF] Add WTF::unalignedLoad and WTF::unalignedStore
>+        https://bugs.webkit.org/show_bug.cgi?id=188716
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Use WTF::unalignedLoad and WTF::unalignedStore to avoid undefined behavior.
>+        The compiler can emit appropriate mov operations in x86 even if we use these
>+        helper functions.
>+
>+        * assembler/AssemblerBuffer.h:
>+        (JSC::AssemblerBuffer::LocalWriter::putIntegralUnchecked):
>+        (JSC::AssemblerBuffer::putIntegral):
>+        (JSC::AssemblerBuffer::putIntegralUnchecked):
>+        * assembler/MacroAssemblerX86.h:
>+        (JSC::MacroAssemblerX86::readCallTarget):
>+        * assembler/X86Assembler.h:
>+        (JSC::X86Assembler::linkJump):
>+        (JSC::X86Assembler::readPointer):
>+        (JSC::X86Assembler::replaceWithHlt):
>+        (JSC::X86Assembler::replaceWithJump):
>+        (JSC::X86Assembler::setPointer):
>+        (JSC::X86Assembler::setInt32):
>+        (JSC::X86Assembler::setInt8):
>+        * interpreter/InterpreterInlines.h:
>+        (JSC::Interpreter::getOpcodeID): Embedded opcode may be misaligned. Actually UBSan detects misaligned accesses here.
>+
> 2018-08-17  Saam barati  <sbarati@apple.com>
> 
>         intersectionOfPastValuesAtHead must filter values after they've observed an invalidation point
>diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
>index 668e9b81be5948bdf4083ff3653d57f442e4d510..c4dc020d7b52df0209824a96f5e7df834253760e 100644
>--- a/Source/WTF/ChangeLog
>+++ b/Source/WTF/ChangeLog
>@@ -1,3 +1,29 @@
>+2018-08-19  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
>+
>+        [WTF] Add WTF::unalignedLoad and WTF::unalignedStore
>+        https://bugs.webkit.org/show_bug.cgi?id=188716
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        While some CPUs allow unaligned accesses to memory, doing it in C++ with `reinterpret_cast<>` is
>+        undefined behavior. This patch adds WTF::{unalignedLoad,unalignedStore} helper functions, which
>+        can load from and store to the pointer in an unaligned manner.
>+        Actual implementation uses `memcpy`. This can be optimized to direct unaligned access operations
>+        in supported CPUs like x86. Even though a CPU does not support unaligned accesses, memcpy is still
>+        safe and the compiler emits appropriate code.
>+
>+        We name these functions `unalignedLoad` and `unalignedStore` instead of `loadUnaligned` and `storeUnaligned`
>+        in order to align them to `atomicLoad` and `atomicStore`.
>+
>+        * WTF.xcodeproj/project.pbxproj:
>+        * wtf/CMakeLists.txt:
>+        * wtf/UnalignedAccess.h: Added.
>+        (WTF::unalignedLoad):
>+        (WTF::unalignedStore):
>+        * wtf/text/StringCommon.h:
>+        (WTF::equal):
>+        (WTF::loadUnaligned): Deleted.
>+
> 2018-08-17  David Kilzer  <ddkilzer@apple.com>
> 
>         WTF's internal std::optional implementation should release assert on all bad accesses
>diff --git a/Source/JavaScriptCore/assembler/AssemblerBuffer.h b/Source/JavaScriptCore/assembler/AssemblerBuffer.h
>index 7340952d53c4f5d4f58855feac7511324e5ef9dd..54b2e9b1eb162e03b9a1e37e9e43161ee0cb1fce 100644
>--- a/Source/JavaScriptCore/assembler/AssemblerBuffer.h
>+++ b/Source/JavaScriptCore/assembler/AssemblerBuffer.h
>@@ -34,6 +34,7 @@
> #include <wtf/Assertions.h>
> #include <wtf/FastMalloc.h>
> #include <wtf/StdLibExtras.h>
>+#include <wtf/UnalignedAccess.h>
> 
> namespace JSC {
> 
>@@ -239,7 +240,7 @@ namespace JSC {
>             void putIntegralUnchecked(IntegralType value)
>             {
>                 ASSERT(m_index + sizeof(IntegralType) <= m_buffer.m_storage.capacity());
>-                *reinterpret_cast_ptr<IntegralType*>(m_storageBuffer + m_index) = value;
>+                WTF::unalignedStore<IntegralType>(m_storageBuffer + m_index, value);
>                 m_index += sizeof(IntegralType);
>             }
>             AssemblerBuffer& m_buffer;
>@@ -258,16 +259,14 @@ namespace JSC {
>             unsigned nextIndex = m_index + sizeof(IntegralType);
>             if (UNLIKELY(nextIndex > m_storage.capacity()))
>                 outOfLineGrow();
>-            ASSERT(isAvailable(sizeof(IntegralType)));
>-            *reinterpret_cast_ptr<IntegralType*>(m_storage.buffer() + m_index) = value;
>-            m_index = nextIndex;
>+            putIntegralUnchecked<IntegralType>(value);
>         }
> 
>         template<typename IntegralType>
>         void putIntegralUnchecked(IntegralType value)
>         {
>             ASSERT(isAvailable(sizeof(IntegralType)));
>-            *reinterpret_cast_ptr<IntegralType*>(m_storage.buffer() + m_index) = value;
>+            WTF::unalignedStore<IntegralType>(m_storage.buffer() + m_index, value);
>             m_index += sizeof(IntegralType);
>         }
> 
>diff --git a/Source/JavaScriptCore/assembler/MacroAssemblerX86.h b/Source/JavaScriptCore/assembler/MacroAssemblerX86.h
>index 8bc023d571a06ae6f614087bf9b0dc24f719ed96..72def72c6b4b7188909693ac5f1fa0bcbaad4399 100644
>--- a/Source/JavaScriptCore/assembler/MacroAssemblerX86.h
>+++ b/Source/JavaScriptCore/assembler/MacroAssemblerX86.h
>@@ -302,7 +302,7 @@ class MacroAssemblerX86 : public MacroAssemblerX86Common {
>     template<PtrTag resultTag, PtrTag locationTag>
>     static FunctionPtr<resultTag> readCallTarget(CodeLocationCall<locationTag> call)
>     {
>-        intptr_t offset = reinterpret_cast<int32_t*>(call.dataLocation())[-1];
>+        intptr_t offset = WTF::unalignedLoad<int32_t>(bitwise_cast<int32_t*>(call.dataLocation()) - 1);
>         return FunctionPtr<resultTag>(reinterpret_cast<void*>(reinterpret_cast<uintptr_t>(call.dataLocation()) + offset));
>     }
> 
>diff --git a/Source/JavaScriptCore/assembler/X86Assembler.h b/Source/JavaScriptCore/assembler/X86Assembler.h
>index f3a4083c121d571886fdec1d0919eaac9ec0b841..2f5d0c26de6e4531d4a1a20bec349c375118472f 100644
>--- a/Source/JavaScriptCore/assembler/X86Assembler.h
>+++ b/Source/JavaScriptCore/assembler/X86Assembler.h
>@@ -3680,7 +3680,7 @@ class X86Assembler {
>         ASSERT(to.isSet());
> 
>         char* code = reinterpret_cast<char*>(m_formatter.data());
>-        ASSERT(!reinterpret_cast<int32_t*>(code + from.m_offset)[-1]);
>+        ASSERT(!WTF::unalignedLoad<int32_t>(bitwise_cast<int32_t*>(code + from.m_offset) - 1));
>         setRel32(code + from.m_offset, code + to.m_offset);
>     }
>     
>@@ -3739,22 +3739,21 @@ class X86Assembler {
>     
>     static void* readPointer(void* where)
>     {
>-        return reinterpret_cast<void**>(where)[-1];
>+        return WTF::unalignedLoad<void*>(bitwise_cast<void**>(where) - 1);
>     }
> 
>     static void replaceWithHlt(void* instructionStart)
>     {
>-        uint8_t* ptr = reinterpret_cast<uint8_t*>(instructionStart);
>-        ptr[0] = static_cast<uint8_t>(OP_HLT);
>+        WTF::unalignedStore<uint8_t>(instructionStart, static_cast<uint8_t>(OP_HLT));
>     }
> 
>     static void replaceWithJump(void* instructionStart, void* to)
>     {
>-        uint8_t* ptr = reinterpret_cast<uint8_t*>(instructionStart);
>-        uint8_t* dstPtr = reinterpret_cast<uint8_t*>(to);
>+        uint8_t* ptr = bitwise_cast<uint8_t*>(instructionStart);
>+        uint8_t* dstPtr = bitwise_cast<uint8_t*>(to);
>         intptr_t distance = (intptr_t)(dstPtr - (ptr + 5));
>-        ptr[0] = static_cast<uint8_t>(OP_JMP_rel32);
>-        *reinterpret_cast<int32_t*>(ptr + 1) = static_cast<int32_t>(distance);
>+        WTF::unalignedStore<uint8_t>(ptr, OP_JMP_rel32);
>+        WTF::unalignedStore<uint32_t>(ptr + 1, distance);
>     }
>     
>     static ptrdiff_t maxJumpReplacementSize()
>@@ -3956,17 +3955,17 @@ class X86Assembler {
> 
>     static void setPointer(void* where, void* value)
>     {
>-        reinterpret_cast<void**>(where)[-1] = value;
>+        WTF::unalignedStore<void*>(bitwise_cast<void**>(where) - 1, value);
>     }
> 
>     static void setInt32(void* where, int32_t value)
>     {
>-        reinterpret_cast<int32_t*>(where)[-1] = value;
>+        WTF::unalignedStore<int32_t>(bitwise_cast<int32_t*>(where) - 1, value);
>     }
>     
>     static void setInt8(void* where, int8_t value)
>     {
>-        reinterpret_cast<int8_t*>(where)[-1] = value;
>+        WTF::unalignedStore<int8_t>(bitwise_cast<int8_t*>(where) - 1, value);
>     }
> 
>     static void setRel32(void* from, void* to)
>diff --git a/Source/JavaScriptCore/interpreter/InterpreterInlines.h b/Source/JavaScriptCore/interpreter/InterpreterInlines.h
>index fc89a189d6057d8e4e0ab10a8791f856b49f9071..41a4551a67d9df54cd7cc91ea5230625996a5c2a 100644
>--- a/Source/JavaScriptCore/interpreter/InterpreterInlines.h
>+++ b/Source/JavaScriptCore/interpreter/InterpreterInlines.h
>@@ -33,6 +33,7 @@
> #include "JSCPtrTag.h"
> #include "LLIntData.h"
> #include "UnlinkedCodeBlock.h"
>+#include <wtf/UnalignedAccess.h>
> 
> namespace JSC {
> 
>@@ -51,7 +52,7 @@ inline OpcodeID Interpreter::getOpcodeID(Opcode opcode)
>     // in LowLevelInterpreter.cpp).
>     auto codePtr = MacroAssemblerCodePtr<BytecodePtrTag>::createFromExecutableAddress(opcode);
>     int32_t* opcodeIDAddress = codePtr.dataLocation<int32_t*>() - 1;
>-    OpcodeID opcodeID = static_cast<OpcodeID>(*opcodeIDAddress);
>+    OpcodeID opcodeID = static_cast<OpcodeID>(WTF::unalignedLoad<int32_t>(opcodeIDAddress));
>     ASSERT(opcodeID < NUMBER_OF_BYTECODE_IDS);
>     return opcodeID;
> #else
>diff --git a/Source/WTF/WTF.xcodeproj/project.pbxproj b/Source/WTF/WTF.xcodeproj/project.pbxproj
>index 704338a524d074a73e2824cfce26a1067e8d2a3e..45b5ceaecf0e02ec5d65167787675c689d35245d 100644
>--- a/Source/WTF/WTF.xcodeproj/project.pbxproj
>+++ b/Source/WTF/WTF.xcodeproj/project.pbxproj
>@@ -623,6 +623,8 @@
> 		E311FB161F0A568B003C08DE /* ThreadGroup.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ThreadGroup.h; sourceTree = "<group>"; };
> 		E3200AB41E9A536D003B59D2 /* PlatformRegisters.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = PlatformRegisters.h; sourceTree = "<group>"; };
> 		E33D5F871FBED66700BF625E /* RecursableLambda.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = RecursableLambda.h; sourceTree = "<group>"; };
>+		E360C7642127B85B00C90F0E /* UnalignedAccess.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = UnalignedAccess.h; sourceTree = "<group>"; };
>+		E360C7652127B85C00C90F0E /* Unexpected.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = Unexpected.h; sourceTree = "<group>"; };
> 		E388886D20C9095100E632BC /* WorkerPool.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = WorkerPool.cpp; sourceTree = "<group>"; };
> 		E388886E20C9095100E632BC /* WorkerPool.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WorkerPool.h; sourceTree = "<group>"; };
> 		E38C41241EB4E04C0042957D /* CPUTimeCocoa.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = CPUTimeCocoa.cpp; sourceTree = "<group>"; };
>@@ -1117,6 +1119,8 @@
> 				0FED67B51B22D4D80066CE15 /* TinyPtrSet.h */,
> 				149EF16216BBFE0D000A4331 /* TriState.h */,
> 				83FBA93119DF459700F30ADB /* TypeCasts.h */,
>+				E360C7642127B85B00C90F0E /* UnalignedAccess.h */,
>+				E360C7652127B85C00C90F0E /* Unexpected.h */,
> 				A8A4735C151A825B004123FF /* UnionFind.h */,
> 				E300E521203D645F00DA79BE /* UniqueArray.h */,
> 				5C7C88D31D0A3A0A009D2F6D /* UniqueRef.h */,
>diff --git a/Source/WTF/wtf/CMakeLists.txt b/Source/WTF/wtf/CMakeLists.txt
>index 9798b9b8a10585f34f0792c9a3149e09c38ce964..9adb55543d013fea9af5a7aae49432f95f3240f2 100644
>--- a/Source/WTF/wtf/CMakeLists.txt
>+++ b/Source/WTF/wtf/CMakeLists.txt
>@@ -240,6 +240,7 @@ set(WTF_PUBLIC_HEADERS
>     TriState.h
>     TypeCasts.h
>     UUID.h
>+    UnalignedAccess.h
>     Unexpected.h
>     UniStdExtras.h
>     UnionFind.h
>diff --git a/Source/WTF/wtf/UnalignedAccess.h b/Source/WTF/wtf/UnalignedAccess.h
>new file mode 100644
>index 0000000000000000000000000000000000000000..1a37b4eec1ad2124a35c83c6bc0ecb8edca65336
>--- /dev/null
>+++ b/Source/WTF/wtf/UnalignedAccess.h
>@@ -0,0 +1,50 @@
>+/*
>+ * Copyright (C) 2018 Yusuke Suzuki <yusukesuzuki@slowstart.org>.
>+ *
>+ * Redistribution and use in source and binary forms, with or without
>+ * modification, are permitted provided that the following conditions
>+ * are met:
>+ * 1. Redistributions of source code must retain the above copyright
>+ *    notice, this list of conditions and the following disclaimer.
>+ * 2. Redistributions in binary form must reproduce the above copyright
>+ *    notice, this list of conditions and the following disclaimer in the
>+ *    documentation and/or other materials provided with the distribution.
>+ *
>+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY
>+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
>+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
>+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE INC. OR
>+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
>+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
>+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
>+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
>+ * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>+ */
>+
>+#pragma once
>+
>+#include <type_traits>
>+#include <wtf/Platform.h>
>+#include <wtf/StdLibExtras.h>
>+
>+namespace WTF {
>+
>+template<typename IntegralType>
>+inline IntegralType unalignedLoad(const void* pointer)
>+{
>+    static_assert(std::is_integral<IntegralType>::value || std::is_pointer<IntegralType>::value, "");
>+    IntegralType result { };
>+    memcpy(&result, pointer, sizeof(IntegralType));
>+    return result;
>+}
>+
>+template<typename IntegralType>
>+inline void unalignedStore(void* pointer, IntegralType value)
>+{
>+    static_assert(std::is_integral<IntegralType>::value || std::is_pointer<IntegralType>::value, "");
>+    memcpy(pointer, &value, sizeof(IntegralType));
>+}
>+
>+} // namespace WTF
>diff --git a/Source/WTF/wtf/text/StringCommon.h b/Source/WTF/wtf/text/StringCommon.h
>index 5a49f8c55a2666160a4fc77d03739c3e4db81477..688346866f3f0c4d6c8665ffa78401d762b454dd 100644
>--- a/Source/WTF/wtf/text/StringCommon.h
>+++ b/Source/WTF/wtf/text/StringCommon.h
>@@ -30,6 +30,7 @@
> #include <unicode/uchar.h>
> #include <wtf/ASCIICType.h>
> #include <wtf/NotFound.h>
>+#include <wtf/UnalignedAccess.h>
> 
> namespace WTF {
> 
>@@ -48,19 +49,6 @@ template<typename StringClass, unsigned length> bool equalLettersIgnoringASCIICa
> bool equalIgnoringASCIICase(const char*, const char*);
> template<unsigned lowercaseLettersLength> bool equalLettersIgnoringASCIICase(const char*, const char (&lowercaseLetters)[lowercaseLettersLength]);
> 
>-template<typename T>
>-inline T loadUnaligned(const char* s)
>-{
>-#if COMPILER(CLANG)
>-    T tmp;
>-    memcpy(&tmp, s, sizeof(T));
>-    return tmp;
>-#else
>-    // This may result in undefined behavior due to unaligned access.
>-    return *reinterpret_cast<const T*>(s);
>-#endif
>-}
>-
> // Do comparisons 8 or 4 bytes-at-a-time on architectures where it's safe.
> #if (CPU(X86_64) || CPU(ARM64)) && !ASAN_ENABLED
> ALWAYS_INLINE bool equal(const LChar* aLChar, const LChar* bLChar, unsigned length)
>@@ -72,7 +60,7 @@ ALWAYS_INLINE bool equal(const LChar* aLChar, const LChar* bLChar, unsigned leng
> 
>     if (dwordLength) {
>         for (unsigned i = 0; i != dwordLength; ++i) {
>-            if (loadUnaligned<uint64_t>(a) != loadUnaligned<uint64_t>(b))
>+            if (unalignedLoad<uint64_t>(a) != unalignedLoad<uint64_t>(b))
>                 return false;
> 
>             a += sizeof(uint64_t);
>@@ -81,7 +69,7 @@ ALWAYS_INLINE bool equal(const LChar* aLChar, const LChar* bLChar, unsigned leng
>     }
> 
>     if (length & 4) {
>-        if (loadUnaligned<uint32_t>(a) != loadUnaligned<uint32_t>(b))
>+        if (unalignedLoad<uint32_t>(a) != unalignedLoad<uint32_t>(b))
>             return false;
> 
>         a += sizeof(uint32_t);
>@@ -89,7 +77,7 @@ ALWAYS_INLINE bool equal(const LChar* aLChar, const LChar* bLChar, unsigned leng
>     }
> 
>     if (length & 2) {
>-        if (loadUnaligned<uint16_t>(a) != loadUnaligned<uint16_t>(b))
>+        if (unalignedLoad<uint16_t>(a) != unalignedLoad<uint16_t>(b))
>             return false;
> 
>         a += sizeof(uint16_t);
>@@ -111,7 +99,7 @@ ALWAYS_INLINE bool equal(const UChar* aUChar, const UChar* bUChar, unsigned leng
> 
>     if (dwordLength) {
>         for (unsigned i = 0; i != dwordLength; ++i) {
>-            if (loadUnaligned<uint64_t>(a) != loadUnaligned<uint64_t>(b))
>+            if (unalignedLoad<uint64_t>(a) != unalignedLoad<uint64_t>(b))
>                 return false;
> 
>             a += sizeof(uint64_t);
>@@ -120,7 +108,7 @@ ALWAYS_INLINE bool equal(const UChar* aUChar, const UChar* bUChar, unsigned leng
>     }
> 
>     if (length & 2) {
>-        if (loadUnaligned<uint32_t>(a) != loadUnaligned<uint32_t>(b))
>+        if (unalignedLoad<uint32_t>(a) != unalignedLoad<uint32_t>(b))
>             return false;
> 
>         a += sizeof(uint32_t);
>@@ -140,7 +128,7 @@ ALWAYS_INLINE bool equal(const LChar* aLChar, const LChar* bLChar, unsigned leng
> 
>     unsigned wordLength = length >> 2;
>     for (unsigned i = 0; i != wordLength; ++i) {
>-        if (loadUnaligned<uint32_t>(a) != loadUnaligned<uint32_t>(b))
>+        if (unalignedLoad<uint32_t>(a) != unalignedLoad<uint32_t>(b))
>             return false;
>         a += sizeof(uint32_t);
>         b += sizeof(uint32_t);
>@@ -168,7 +156,7 @@ ALWAYS_INLINE bool equal(const UChar* aUChar, const UChar* bUChar, unsigned leng
> 
>     unsigned wordLength = length >> 1;
>     for (unsigned i = 0; i != wordLength; ++i) {
>-        if (loadUnaligned<uint32_t>(a) != loadUnaligned<uint32_t>(b))
>+        if (unalignedLoad<uint32_t>(a) != unalignedLoad<uint32_t>(b))
>             return false;
>         a += sizeof(uint32_t);
>         b += sizeof(uint32_t);

Flags:

darin: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 188716: 347421 | 347423 | 347455