Attachment 348927 Details for Bug 188598 – Patch

[patch] Patch

bug-188598-20180905175453.patch (text/plain), 16.16 KB, created by Guillaume Emont on 2018-09-05 08:54:55 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Guillaume Emont

Created: 2018-09-05 08:54:55 PDT

Size: 16.16 KB

patch

obsolete

>Subversion Revision: 235600
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 4274756e768256e2a054c51323ca83d118a05821..64737cd9de38efd52e4b6d1b1e1d5cda1b7f3ab3 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,44 @@
>+2018-09-05  Guillaume Emont  <guijemont@igalia.com>
>+
>+        [JSC] Remove gcc warnings on mips and armv7
>+        https://bugs.webkit.org/show_bug.cgi?id=188598
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Remove many gcc/clang warnings that are false positives, mostly
>+        alignment issues.
>+
>+        * assembler/MacroAssemblerPrinter.cpp:
>+        (JSC::Printer::printMemory): a Memory keeps size information, so we
>+        know the original value was of the same size and can ignore the
>+        -Wcast-align warning.
>+        * assembler/testmasm.cpp:
>+        (JSC::floatOperands): marked as potentially unused as it is not used
>+        on all platforms.
>+        (JSC::testProbeModifiesStackValues): modifiedFlags is not used on
>+        mips, so don't declare it.
>+        * interpreter/Interpreter.cpp:
>+        (JSC::Interpreter::executeProgram):
>+        (JSC::Interpreter::executeCall):
>+        (JSC::Interpreter::executeConstruct):
>+        (JSC::Interpreter::prepareForRepeatCall):
>+        (JSC::Interpreter::execute):
>+        (JSC::Interpreter::executeModuleProgram):
>+        The prepareForExecution() method either returns nullptr or an
>+        Exception* cast to a JSObject*. It is therefore safe to cast it back
>+        to Exception* even though it has stricter alignment requirements.
>+        * jit/JITOperations.cpp:
>+        Same as for Interpreter.cpp.
>+        * runtime/JSBigInt.cpp:
>+        (JSC::JSBigInt::dataStorage):
>+        Pointer arithmetics done correctly, but gcc doesn't understand this,
>+        so we ignore -Wcast-align
>+        * tools/JSDollarVM.cpp:
>+        (JSC::codeBlockFromArg):
>+        After the cast, a check is done to make sure that the pointer is to a
>+        known CodeBlock before using it, so if the result is badly aligned it
>+        won't be used anyways.
>+
> 2018-08-27  Guillaume Emont  <guijemont@igalia.com>
> 
>         Add IGNORE_WARNING_.* macros
>diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
>index e6d2ddbd093cabdc4102659e4fc95936d885c43a..892ee9690873879e74f3f227f8b9664b6a5ffb1c 100644
>--- a/Source/WTF/ChangeLog
>+++ b/Source/WTF/ChangeLog
>@@ -1,3 +1,16 @@
>+2018-09-05  Guillaume Emont  <guijemont@igalia.com>
>+
>+        [JSC] Remove gcc warnings on mips and armv7
>+        https://bugs.webkit.org/show_bug.cgi?id=188598
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Added IGNORE_CAST_ALIGN_BEGIN/_END macros to be able to
>+        simply ignore warnings on non-respect of alignment in casts for a well
>+        defined code subset.
>+
>+        * wtf/Compiler.h:
>+
> 2018-08-27  Guillaume Emont  <guijemont@igalia.com>
> 
>         Add IGNORE_WARNING_.* macros
>diff --git a/Source/bmalloc/ChangeLog b/Source/bmalloc/ChangeLog
>index d469040dde3f989ef453123f934d329c6c83c59c..732a1344df22bb2f4abbf5fa77623d8fdc1d9bcf 100644
>--- a/Source/bmalloc/ChangeLog
>+++ b/Source/bmalloc/ChangeLog
>@@ -1,3 +1,18 @@
>+2018-09-05  Guillaume Emont  <guijemont@igalia.com>
>+
>+        [JSC] Remove gcc warnings on mips and armv7
>+        https://bugs.webkit.org/show_bug.cgi?id=188598
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Use IGNORE_CAST_ALIGN_BEGIN/END to allow some casts after pointer
>+        calculations without triggering alignment warnings on mips and armv7.
>+
>+        * bmalloc/IsoDirectoryPageInlines.h:
>+        (bmalloc::IsoDirectoryPage<Config>::pageFor):
>+        * bmalloc/IsoPageInlines.h:
>+        (bmalloc::IsoPage<Config>::startAllocating):
>+
> 2018-08-27  Keith Rollin  <krollin@apple.com>
> 
>         Unreviewed build fix -- disable LTO for production builds
>diff --git a/Source/JavaScriptCore/assembler/MacroAssemblerPrinter.cpp b/Source/JavaScriptCore/assembler/MacroAssemblerPrinter.cpp
>index 3ed64b90ba5fbc93b2c992b40ec21f0710d15dd7..b4a4bf9f6072e8b7d38e088a8a957259f07ea101 100644
>--- a/Source/JavaScriptCore/assembler/MacroAssemblerPrinter.cpp
>+++ b/Source/JavaScriptCore/assembler/MacroAssemblerPrinter.cpp
>@@ -137,6 +137,10 @@ void printMemory(PrintStream& out, Context& context)
>             out.printf("%p:<0x%02x %d>", p, *p, *p);
>             return;
>         }
>+        // assuming memory is not malformed, it originately pointed to a value
>+        // of the required size, which should be properly aligned on platforms
>+        // that require it.
>+        IGNORE_CAST_ALIGN_BEGIN
>         if (memory.numBytes == sizeof(int16_t)) {
>             auto p = reinterpret_cast<int16_t*>(ptr);
>             out.printf("%p:<0x%04x %d>", p, *p, *p);
>@@ -152,6 +156,7 @@ void printMemory(PrintStream& out, Context& context)
>             out.printf("%p:<0x%016" PRIx64 " %" PRId64 ">", p, *p, *p);
>             return;
>         }
>+        IGNORE_CAST_ALIGN_END
>         // Else, unknown word size. Fall thru and dump in the generic way.
>     }
> 
>diff --git a/Source/JavaScriptCore/assembler/testmasm.cpp b/Source/JavaScriptCore/assembler/testmasm.cpp
>index 1d77d808bc0a1f09c1be12800c9419aa71a6aebd..4c54c5a23d4bc5b46be049e4be472f1d2e36da7c 100644
>--- a/Source/JavaScriptCore/assembler/testmasm.cpp
>+++ b/Source/JavaScriptCore/assembler/testmasm.cpp
>@@ -246,7 +246,7 @@ static Vector<double> doubleOperands()
> }
> 
> 
>-static Vector<float> floatOperands()
>+static Vector<float> UNUSED_FUNCTION floatOperands()
> {
>     return Vector<float> {
>         0,
>@@ -747,7 +747,9 @@ void testProbeModifiesStackValues()
>     CPUState originalState;
>     void* originalSP { nullptr };
>     void* newSP { nullptr };
>+#if !(CPU(MIPS))
>     uintptr_t modifiedFlags { 0 };
>+#endif
>     size_t numberOfExtraEntriesToWrite { 10 }; // ARM64 requires that this be 2 word aligned.
> 
> #if CPU(X86) || CPU(X86_64)
>diff --git a/Source/JavaScriptCore/interpreter/Interpreter.cpp b/Source/JavaScriptCore/interpreter/Interpreter.cpp
>index 7126d029f6223b2ea20026a9265dd952eb405255..cbe7e7ab41da108a57cc4ad6776bb61e8d9867db 100644
>--- a/Source/JavaScriptCore/interpreter/Interpreter.cpp
>+++ b/Source/JavaScriptCore/interpreter/Interpreter.cpp
>@@ -809,7 +809,9 @@ failedJSONP:
>     {
>         CodeBlock* tempCodeBlock;
>         JSObject* error = program->prepareForExecution<ProgramExecutable>(vm, nullptr, scope, CodeForCall, tempCodeBlock);
>+        IGNORE_CAST_ALIGN_BEGIN
>         EXCEPTION_ASSERT(throwScope.exception() == reinterpret_cast<Exception*>(error));
>+        IGNORE_CAST_ALIGN_END
>         if (UNLIKELY(error))
>             return checkedReturn(error);
>         codeBlock = jsCast<ProgramCodeBlock*>(tempCodeBlock);
>@@ -867,7 +869,9 @@ JSValue Interpreter::executeCall(CallFrame* callFrame, JSObject* function, CallT
>     if (isJSCall) {
>         // Compile the callee:
>         JSObject* compileError = callData.js.functionExecutable->prepareForExecution<FunctionExecutable>(vm, jsCast<JSFunction*>(function), scope, CodeForCall, newCodeBlock);
>+        IGNORE_CAST_ALIGN_BEGIN
>         EXCEPTION_ASSERT(throwScope.exception() == reinterpret_cast<Exception*>(compileError));
>+        IGNORE_CAST_ALIGN_END
>         if (UNLIKELY(!!compileError))
>             return checkedReturn(compileError);
> 
>@@ -934,7 +938,9 @@ JSObject* Interpreter::executeConstruct(CallFrame* callFrame, JSObject* construc
>     if (isJSConstruct) {
>         // Compile the callee:
>         JSObject* compileError = constructData.js.functionExecutable->prepareForExecution<FunctionExecutable>(vm, jsCast<JSFunction*>(constructor), scope, CodeForConstruct, newCodeBlock);
>+        IGNORE_CAST_ALIGN_BEGIN
>         EXCEPTION_ASSERT(throwScope.exception() == reinterpret_cast<Exception*>(compileError));
>+        IGNORE_CAST_ALIGN_END
>         if (UNLIKELY(!!compileError))
>             return checkedReturn(compileError);
> 
>@@ -982,7 +988,9 @@ CallFrameClosure Interpreter::prepareForRepeatCall(FunctionExecutable* functionE
>     // Compile the callee:
>     CodeBlock* newCodeBlock;
>     JSObject* error = functionExecutable->prepareForExecution<FunctionExecutable>(vm, function, scope, CodeForCall, newCodeBlock);
>+    IGNORE_CAST_ALIGN_BEGIN
>     EXCEPTION_ASSERT(throwScope.exception() == reinterpret_cast<Exception*>(error));
>+    IGNORE_CAST_ALIGN_END
>     if (UNLIKELY(error))
>         return CallFrameClosure();
>     newCodeBlock->m_shouldAlwaysBeInlined = false;
>@@ -1040,7 +1048,9 @@ JSValue Interpreter::execute(EvalExecutable* eval, CallFrame* callFrame, JSValue
>     {
>         CodeBlock* tempCodeBlock;
>         JSObject* compileError = eval->prepareForExecution<EvalExecutable>(vm, nullptr, scope, CodeForCall, tempCodeBlock);
>+        IGNORE_CAST_ALIGN_BEGIN
>         EXCEPTION_ASSERT(throwScope.exception() == reinterpret_cast<Exception*>(compileError));
>+        IGNORE_CAST_ALIGN_END
>         if (UNLIKELY(!!compileError))
>             return checkedReturn(compileError);
>         codeBlock = jsCast<EvalCodeBlock*>(tempCodeBlock);
>@@ -1163,7 +1173,9 @@ JSValue Interpreter::executeModuleProgram(ModuleProgramExecutable* executable, C
>     {
>         CodeBlock* tempCodeBlock;
>         JSObject* compileError = executable->prepareForExecution<ModuleProgramExecutable>(vm, nullptr, scope, CodeForCall, tempCodeBlock);
>+        IGNORE_CAST_ALIGN_BEGIN
>         EXCEPTION_ASSERT(throwScope.exception() == reinterpret_cast<Exception*>(compileError));
>+        IGNORE_CAST_ALIGN_END
>         if (UNLIKELY(!!compileError))
>             return checkedReturn(compileError);
>         codeBlock = jsCast<ModuleProgramCodeBlock*>(tempCodeBlock);
>diff --git a/Source/JavaScriptCore/jit/JITOperations.cpp b/Source/JavaScriptCore/jit/JITOperations.cpp
>index b2eb418cff89d523022a61adb0b8be4f68802e62..260d732969dbbf747ce334aa956e4340d1022173 100644
>--- a/Source/JavaScriptCore/jit/JITOperations.cpp
>+++ b/Source/JavaScriptCore/jit/JITOperations.cpp
>@@ -1045,7 +1045,9 @@ SlowPathReturnType JIT_OPERATION operationLinkCall(ExecState* execCallee, CallLi
> 
>         CodeBlock** codeBlockSlot = execCallee->addressOfCodeBlock();
>         JSObject* error = functionExecutable->prepareForExecution<FunctionExecutable>(*vm, callee, scope, kind, *codeBlockSlot);
>+        IGNORE_CAST_ALIGN_BEGIN
>         EXCEPTION_ASSERT(throwScope.exception() == reinterpret_cast<Exception*>(error));
>+        IGNORE_CAST_ALIGN_END
>         if (error)
>             return handleThrowException();
>         codeBlock = *codeBlockSlot;
>@@ -1101,7 +1103,9 @@ void JIT_OPERATION operationLinkDirectCall(ExecState* exec, CallLinkInfo* callLi
>         RELEASE_ASSERT(isCall(kind) || functionExecutable->constructAbility() != ConstructAbility::CannotConstruct);
>         
>         JSObject* error = functionExecutable->prepareForExecution<FunctionExecutable>(*vm, callee, scope, kind, codeBlock);
>+        IGNORE_CAST_ALIGN_BEGIN
>         EXCEPTION_ASSERT_UNUSED(throwScope, throwScope.exception() == reinterpret_cast<Exception*>(error));
>+        IGNORE_CAST_ALIGN_END
>         if (error)
>             return;
>         unsigned argumentStackSlots = callLinkInfo->maxNumArguments();
>@@ -1151,7 +1155,9 @@ inline SlowPathReturnType virtualForWithFunction(
> 
>         CodeBlock** codeBlockSlot = execCallee->addressOfCodeBlock();
>         JSObject* error = functionExecutable->prepareForExecution<FunctionExecutable>(*vm, function, scope, kind, *codeBlockSlot);
>+        IGNORE_CAST_ALIGN_BEGIN
>         EXCEPTION_ASSERT(throwScope.exception() == reinterpret_cast<Exception*>(error));
>+        IGNORE_CAST_ALIGN_END
>         if (error) {
>             return encodeResult(
>                 vm->getCTIStub(throwExceptionFromCallSlowPathGenerator).retaggedCode<JSEntryPtrTag>().executableAddress(),
>diff --git a/Source/JavaScriptCore/runtime/JSBigInt.cpp b/Source/JavaScriptCore/runtime/JSBigInt.cpp
>index ba355bd743a3e8764809a8f27d39eea4b69e6a3f..700bfd0e8d6371046397fe78c67276cf6a1b93d6 100644
>--- a/Source/JavaScriptCore/runtime/JSBigInt.cpp
>+++ b/Source/JavaScriptCore/runtime/JSBigInt.cpp
>@@ -1312,7 +1312,13 @@ JSBigInt* JSBigInt::parseInt(ExecState* exec, VM& vm, CharType* data, unsigned l
> 
> inline JSBigInt::Digit* JSBigInt::dataStorage()
> {
>+    // offsetOfData() makes sure that its return value is aligned to the size of
>+    // Digit, so even though we cast to char* for pointer arithmetics, the cast to
>+    // Digit* is properly aligned, though the compiler doesn't know about it,
>+    // therefore we disable this warning.
>+    IGNORE_CAST_ALIGN_BEGIN
>     return reinterpret_cast<Digit*>(reinterpret_cast<char*>(this) + offsetOfData());
>+    IGNORE_CAST_ALIGN_END
> }
> 
> inline JSBigInt::Digit JSBigInt::digit(unsigned n)
>diff --git a/Source/JavaScriptCore/tools/JSDollarVM.cpp b/Source/JavaScriptCore/tools/JSDollarVM.cpp
>index 8a461e76ec8011a9b2378d6d4275d31ac121109c..db14fa0e5c7ed53cb70325c90496dcdf5b82fafa 100644
>--- a/Source/JavaScriptCore/tools/JSDollarVM.cpp
>+++ b/Source/JavaScriptCore/tools/JSDollarVM.cpp
>@@ -1463,8 +1463,16 @@ static CodeBlock* codeBlockFromArg(ExecState* exec)
>                 candidateCodeBlock = nullptr;
>             else
>                 candidateCodeBlock = func->jsExecutable()->eitherCodeBlock();
>-        } else
>+        } else {
>+            // Here it's OK to ignore the alignment requirements since
>+            // candidateCodeBlock will only be accessed if
>+            // VMInspector::isValidCodeBlock() returns true, which only happens
>+            // if the address is that of a known existing, properly aligned
>+            // codeBlock.
>+            IGNORE_CAST_ALIGN_BEGIN
>             candidateCodeBlock = reinterpret_cast<CodeBlock*>(value.asCell());
>+            IGNORE_CAST_ALIGN_END
>+        }
>     }
> 
>     if (candidateCodeBlock && VMInspector::isValidCodeBlock(exec, candidateCodeBlock))
>diff --git a/Source/WTF/wtf/Compiler.h b/Source/WTF/wtf/Compiler.h
>index 379e32caf27658b1aa4dd5a4c08b9f55dc92c408..4666ed0d5c46ad61646cde5321bf4a5b8807ae77 100644
>--- a/Source/WTF/wtf/Compiler.h
>+++ b/Source/WTF/wtf/Compiler.h
>@@ -476,4 +476,10 @@
> #define ALLOW_NULL_ARGUMENT_END IGNORE_WARNING_END
> #endif
> 
>+#if !defined(IGNORE_CAST_ALIGN_BEGIN)
>+#define IGNORE_CAST_ALIGN_BEGIN IGNORE_WARNING_BEGIN("-Wcast-align")
>+#define IGNORE_CAST_ALIGN_END IGNORE_WARNING_END
>+#endif
>+
>+
> #endif /* WTF_Compiler_h */
>diff --git a/Source/bmalloc/bmalloc/IsoDirectoryPageInlines.h b/Source/bmalloc/bmalloc/IsoDirectoryPageInlines.h
>index a149e108d137c941e900b573e98fd35e1b014b98..7f6ca24a1439d185c876af7a32d8e389146a39cc 100644
>--- a/Source/bmalloc/bmalloc/IsoDirectoryPageInlines.h
>+++ b/Source/bmalloc/bmalloc/IsoDirectoryPageInlines.h
>@@ -39,8 +39,13 @@ IsoDirectoryPage<Config>::IsoDirectoryPage(IsoHeapImpl<Config>& heap, unsigned i
> template<typename Config>
> IsoDirectoryPage<Config>* IsoDirectoryPage<Config>::pageFor(IsoDirectory<Config, numPages>* payload)
> {
>+    // the char * cast is only used to do a pointer calculation, and said
>+    // calculation results in a pointer to an existing, correctly aligned
>+    // IsoDirectoryPage.
>+    IGNORE_CAST_ALIGN_BEGIN
>     return reinterpret_cast<IsoDirectoryPage<Config>*>(
>         reinterpret_cast<char*>(payload) - BOFFSETOF(IsoDirectoryPage, payload));
>+    IGNORE_CAST_ALIGN_END
> }
> 
> } // namespace bmalloc
>diff --git a/Source/bmalloc/bmalloc/IsoPageInlines.h b/Source/bmalloc/bmalloc/IsoPageInlines.h
>index 0c47864c7b49a0eec8a2610cef67fa328a4ef4c4..e7ce4518b5b8dfdcdc2bb2eec98f37596993068a 100644
>--- a/Source/bmalloc/bmalloc/IsoPageInlines.h
>+++ b/Source/bmalloc/bmalloc/IsoPageInlines.h
>@@ -188,7 +188,20 @@ FreeList IsoPage<Config>::startAllocating()
>         char* cellByte = reinterpret_cast<char*>(this) + index * Config::objectSize;
>         if (verbose)
>             fprintf(stderr, "%p: putting %p on free list.\n", this, cellByte);
>+
>+#if CPU(ARM) || CPU(MIPS)
>+        // the cast below is safe on these platforms as long as
>+        // both Config::objectSize and the alignment of this are multiples of
>+        // the required alignment of FreeCell
>+        static_assert(!(Config::objectSize % alignof(FreeCell)), "Config::objectSize should respect alignment of FreeCell");
>+        static_assert(!(alignof(IsoPage<Config>) % alignof(FreeCell)), "Alignment of IsoPage<Config> should match that of FreeCell");
>+
>+        IGNORE_CAST_ALIGN_BEGIN
>+#endif
>         FreeCell* cell = reinterpret_cast<FreeCell*>(cellByte);
>+#if CPU(ARM) || CPU(MIPS)
>+        IGNORE_CAST_ALIGN_END
>+#endif
>         cell->setNext(head, secret);
>         head = cell;
>         bytes += Config::objectSize;

Actions: View | Formatted Diff | Diff

Attachments on bug 188598: 347156 | 347180 | 348927 | 349562 | 349756 | 350104 | 351282 | 351289 | 351974 | 352124