Attachment 349562 Details for Bug 188598 – Patch

[patch] Patch

bug-188598-20180912201351.patch (text/plain), 16.46 KB, created by Guillaume Emont on 2018-09-12 11:13:53 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Guillaume Emont

Created: 2018-09-12 11:13:53 PDT

Size: 16.46 KB

patch

obsolete

>Subversion Revision: 235943
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 38c6e6702983b8c281b71fb9d4ab2b6a14b20ad5..c31457addd280fa0ab52e6909db43ead195900eb 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-09-12  Guillaume Emont  <guijemont@igalia.com>
> 
>         Add IGNORE_WARNING_.* macros
>diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
>index 605aa7dce35773baf575c314f7f1dccd986b9d7b..c8604be5c4b20bccc8ae46ae6007cad3d1d3cfb3 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_WARNINGS_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-09-12  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..7f36abfb83401fad1497ab7825bb5fe7c6a4e970 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_WARNINGS_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_WARNINGS_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..41ff2082929edced0e9c5ee1216ab59f3a5e7107 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_WARNINGS_BEGIN
>         EXCEPTION_ASSERT(throwScope.exception() == reinterpret_cast<Exception*>(error));
>+        IGNORE_CAST_ALIGN_WARNINGS_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_WARNINGS_BEGIN
>         EXCEPTION_ASSERT(throwScope.exception() == reinterpret_cast<Exception*>(compileError));
>+        IGNORE_CAST_ALIGN_WARNINGS_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_WARNINGS_BEGIN
>         EXCEPTION_ASSERT(throwScope.exception() == reinterpret_cast<Exception*>(compileError));
>+        IGNORE_CAST_ALIGN_WARNINGS_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_WARNINGS_BEGIN
>     EXCEPTION_ASSERT(throwScope.exception() == reinterpret_cast<Exception*>(error));
>+    IGNORE_CAST_ALIGN_WARNINGS_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_WARNINGS_BEGIN
>         EXCEPTION_ASSERT(throwScope.exception() == reinterpret_cast<Exception*>(compileError));
>+        IGNORE_CAST_ALIGN_WARNINGS_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_WARNINGS_BEGIN
>         EXCEPTION_ASSERT(throwScope.exception() == reinterpret_cast<Exception*>(compileError));
>+        IGNORE_CAST_ALIGN_WARNINGS_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..d524aa6790891d034b3e5fc566efa84bef62a394 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_WARNINGS_BEGIN
>         EXCEPTION_ASSERT(throwScope.exception() == reinterpret_cast<Exception*>(error));
>+        IGNORE_CAST_ALIGN_WARNINGS_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_WARNINGS_BEGIN
>         EXCEPTION_ASSERT_UNUSED(throwScope, throwScope.exception() == reinterpret_cast<Exception*>(error));
>+        IGNORE_CAST_ALIGN_WARNINGS_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_WARNINGS_BEGIN
>         EXCEPTION_ASSERT(throwScope.exception() == reinterpret_cast<Exception*>(error));
>+        IGNORE_CAST_ALIGN_WARNINGS_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..c5c45b1d09045216c53eafd959d57d7e39142709 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_WARNINGS_BEGIN
>     return reinterpret_cast<Digit*>(reinterpret_cast<char*>(this) + offsetOfData());
>+    IGNORE_CAST_ALIGN_WARNINGS_END
> }
> 
> inline JSBigInt::Digit JSBigInt::digit(unsigned n)
>diff --git a/Source/JavaScriptCore/tools/JSDollarVM.cpp b/Source/JavaScriptCore/tools/JSDollarVM.cpp
>index 8a461e76ec8011a9b2378d6d4275d31ac121109c..2621117689a88e1f42c5ff330bd7fa03bcf84821 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_WARNINGS_BEGIN
>             candidateCodeBlock = reinterpret_cast<CodeBlock*>(value.asCell());
>+            IGNORE_CAST_ALIGN_WARNINGS_END
>+        }
>     }
> 
>     if (candidateCodeBlock && VMInspector::isValidCodeBlock(exec, candidateCodeBlock))
>diff --git a/Source/WTF/wtf/Compiler.h b/Source/WTF/wtf/Compiler.h
>index 71e6b8e6a08e53754e148a8e8390796d1c9aac0a..4a33ac072c64895f760b64ca050cd9dc1c8d49e0 100644
>--- a/Source/WTF/wtf/Compiler.h
>+++ b/Source/WTF/wtf/Compiler.h
>@@ -464,4 +464,8 @@
> #define IGNORE_NULL_CHECK_WARNINGS_BEGIN IGNORE_WARNINGS_BEGIN("nonnull")
> #define IGNORE_NULL_CHECK_WARNINGS_END IGNORE_WARNINGS_END
> 
>+#define IGNORE_CAST_ALIGN_WARNINGS_BEGIN IGNORE_WARNINGS_BEGIN("cast-align")
>+#define IGNORE_CAST_ALIGN_WARNINGS_END IGNORE_WARNINGS_END
>+
>+
> #endif /* WTF_Compiler_h */
>diff --git a/Source/bmalloc/bmalloc/IsoDirectoryPageInlines.h b/Source/bmalloc/bmalloc/IsoDirectoryPageInlines.h
>index a149e108d137c941e900b573e98fd35e1b014b98..8fd8016e6a09a4a9032d61a925eb4e9e41c36083 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_WARNINGS_BEGIN
>     return reinterpret_cast<IsoDirectoryPage<Config>*>(
>         reinterpret_cast<char*>(payload) - BOFFSETOF(IsoDirectoryPage, payload));
>+    IGNORE_CAST_ALIGN_WARNINGS_END
> }
> 
> } // namespace bmalloc
>diff --git a/Source/bmalloc/bmalloc/IsoPageInlines.h b/Source/bmalloc/bmalloc/IsoPageInlines.h
>index 0c47864c7b49a0eec8a2610cef67fa328a4ef4c4..8ed5183d2e4681184d2da48a0a56cab4a056f6f9 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_WARNINGS_BEGIN
>+#endif
>         FreeCell* cell = reinterpret_cast<FreeCell*>(cellByte);
>+#if CPU(ARM) || CPU(MIPS)
>+        IGNORE_CAST_ALIGN_WARNINGS_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