Bug 171865 – Use Mach exceptions instead of signals where possible

• Component: JavaScriptCore
• Status: REOPENED

[Keith Miller]

Use Mach exceptions instead of signals where possible

[Keith Miller]

Created attachment 309513 [details]
WIP

Passes at least one test

[Mark Lam]

Comment on attachment 309513 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=309513&action=review

> Source/JavaScriptCore/runtime/VMTraps.cpp:75
> -    mcontext_t& mcontext;
> +    PlatformRegisters& context;

I think it's better to rename this to registers.

> Source/JavaScriptCore/tools/SigillCrashAnalyzer.cpp:81
> +    SignalContext(WTF::PlatformRegisters& context)

Don't need the WTF:: qualifier.  And rename context to registers.

[Keith Miller]

Created attachment 309673 [details]
Patch

[Build Bot]

Attachment 309673 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:424:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:125:  catch_mach_exception_raise is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:131:  catch_mach_exception_raise_state_identity is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:137:  catch_mach_exception_raise_state is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 309680 [details]
Patch

[Build Bot]

Attachment 309680 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:424:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:125:  catch_mach_exception_raise is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:131:  catch_mach_exception_raise_state_identity is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:137:  catch_mach_exception_raise_state is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 309715 [details]
Patch

[Build Bot]

Attachment 309715 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:424:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:125:  catch_mach_exception_raise is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:131:  catch_mach_exception_raise_state_identity is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:137:  catch_mach_exception_raise_state is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 309715 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=309715&action=review

> Source/JavaScriptCore/runtime/Options.cpp:490
> +

Please remove.

> Source/JavaScriptCore/runtime/Options.h:448
> +    v(bool, useBBQTierUpChecks, true, Normal, "Enables tier up checks for our BBQ code.") \

Is this meant for this patch?

> Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:245
> +        TierUpCount* tierUp = Options::useBBQTierUpChecks() ? &m_tierUpCounts[functionIndex] : nullptr;

Is this meant for this patch?

[Mark Lam]

Comment on attachment 309715 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=309715&action=review

> Source/JavaScriptCore/runtime/Options.h:423
> +    v(bool, useMachForExceptions, true, Normal, "Use mach exceptions rather than signals to handle faults and pass thread messages. (This does nothing on platforms without mach)") \

I think this is a bit misleading to be set to true all the time.  An alternate way to do this is to default to false, but to override it to true in overrideDefaults() for OS(DARWIN) only.  I think that that makes it clearer which platform uses this.

[Keith Miller]

Created attachment 309722 [details]
Patch

[Build Bot]

Attachment 309722 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:424:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:125:  catch_mach_exception_raise is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:131:  catch_mach_exception_raise_state_identity is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:137:  catch_mach_exception_raise_state is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 5 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 309722 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=309722&action=review

> Source/WTF/wtf/threads/Signals.cpp:33
> +extern "C" {
> +#include "MachExceptionsServer.h"
> +};

Need #if OS(DARWIN)?

> Source/WTF/wtf/threads/Signals.cpp:40
> +#include <mach/mach.h>
> +#include <mach/thread_act.h>

Need #if OS(DARWIN).

[Keith Miller]

Created attachment 309726 [details]
Patch

[Keith Miller]

(In reply to Mark Lam from comment #9)
> Comment on attachment 309715 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309715&action=review
> 
> > Source/JavaScriptCore/runtime/Options.cpp:490
> > +
> 
> Please remove.
> 
> > Source/JavaScriptCore/runtime/Options.h:448
> > +    v(bool, useBBQTierUpChecks, true, Normal, "Enables tier up checks for our BBQ code.") \
> 
> Is this meant for this patch?

Yeah. 

> 
> > Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:245
> > +        TierUpCount* tierUp = Options::useBBQTierUpChecks() ? &m_tierUpCounts[functionIndex] : nullptr;
> 
> Is this meant for this patch?

Ditto.

[Build Bot]

Attachment 309726 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:424:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:129:  catch_mach_exception_raise is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:135:  catch_mach_exception_raise_state_identity is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:141:  catch_mach_exception_raise_state is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 309732 [details]
Patch

[Build Bot]

Attachment 309732 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:424:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:129:  catch_mach_exception_raise is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:135:  catch_mach_exception_raise_state_identity is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:141:  catch_mach_exception_raise_state is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 309751 [details]
Patch

[Build Bot]

Attachment 309751 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:424:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:129:  catch_mach_exception_raise is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:135:  catch_mach_exception_raise_state_identity is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:141:  catch_mach_exception_raise_state is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 309763 [details]
Patch

[Build Bot]

Attachment 309763 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:424:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:129:  catch_mach_exception_raise is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:135:  catch_mach_exception_raise_state_identity is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:141:  catch_mach_exception_raise_state is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 309768 [details]
Patch

[Build Bot]

Attachment 309768 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:424:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:129:  catch_mach_exception_raise is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:135:  catch_mach_exception_raise_state_identity is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:141:  catch_mach_exception_raise_state is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 309769 [details]
Patch

[Build Bot]

Attachment 309769 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:424:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:129:  catch_mach_exception_raise is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:135:  catch_mach_exception_raise_state_identity is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:141:  catch_mach_exception_raise_state is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 309772 [details]
Patch

[Keith Miller]

Created attachment 309774 [details]
Patch

[Build Bot]

Attachment 309774 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:424:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:129:  catch_mach_exception_raise is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:135:  catch_mach_exception_raise_state_identity is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:141:  catch_mach_exception_raise_state is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 309776 [details]
Patch

[Build Bot]

Attachment 309776 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:424:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:129:  catch_mach_exception_raise is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:135:  catch_mach_exception_raise_state_identity is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:141:  catch_mach_exception_raise_state is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 309780 [details]
Patch

[Build Bot]

Attachment 309780 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:424:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:129:  catch_mach_exception_raise is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:135:  catch_mach_exception_raise_state_identity is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:141:  catch_mach_exception_raise_state is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 309790 [details]
Patch

[Build Bot]

Attachment 309790 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:424:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:129:  catch_mach_exception_raise is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:135:  catch_mach_exception_raise_state_identity is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:141:  catch_mach_exception_raise_state is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 309790 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=309790&action=review

> Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:35
> +#include <dispatch/dispatch.h>

Needs #if OS(DARWIN).  You should also enclose the new test section in #if OS(DARWIN).  We need to keep the Windows build green.

[Mark Lam]

Comment on attachment 309790 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=309790&action=review

> Source/JavaScriptCore/runtime/VMTraps.cpp:75
> +    PlatformRegisters& context;

Please rename "context" to "registers".

> Source/JavaScriptCore/runtime/VMTraps.cpp:137
> +            dataLogLn("trapping pc: ", RawPointer(context.trapPC), " jit start: ", startOfFixedExecutableMemoryPool, " jit end: ", endOfFixedExecutableMemoryPool);

This should be conditional on "if (verbose)".  I don't think we want the signal handler logging stuff normally.

> Source/JavaScriptCore/tools/SigillCrashAnalyzer.cpp:82
> +    SignalContext(PlatformRegisters& context)
> +        : context(context)

Let's rename "context" to "registers".

> Source/JavaScriptCore/tools/SigillCrashAnalyzer.cpp:136
> +    PlatformRegisters& context;

Ditto.

> Source/WTF/wtf/ThreadMessage.cpp:201
> +void deliverMessagesUsingMach() { useMach = true; }

Please fix for Webkit style requirements.

> Source/WTF/wtf/ThreadMessage.cpp:212
> +    return MessageStatus::ThreadExited;

Unreachable code.

[Keith Miller]

Comment on attachment 309790 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=309790&action=review

>> Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:35
>> +#include <dispatch/dispatch.h>
> 
> Needs #if OS(DARWIN).  You should also enclose the new test section in #if OS(DARWIN).  We need to keep the Windows build green.

I wrapped it in HAVE(MACH_EXCEPTIONS)

>> Source/JavaScriptCore/runtime/VMTraps.cpp:75
>> +    PlatformRegisters& context;
> 
> Please rename "context" to "registers".

Fixed.

>> Source/JavaScriptCore/runtime/VMTraps.cpp:137
>> +            dataLogLn("trapping pc: ", RawPointer(context.trapPC), " jit start: ", startOfFixedExecutableMemoryPool, " jit end: ", endOfFixedExecutableMemoryPool);
> 
> This should be conditional on "if (verbose)".  I don't think we want the signal handler logging stuff normally.

I removed it, that's old debugging code.

>> Source/JavaScriptCore/tools/SigillCrashAnalyzer.cpp:82
>> +        : context(context)
> 
> Let's rename "context" to "registers".

fixed.

>> Source/JavaScriptCore/tools/SigillCrashAnalyzer.cpp:136
>> +    PlatformRegisters& context;
> 
> Ditto.

fixed.

>> Source/WTF/wtf/ThreadMessage.cpp:201
>> +void deliverMessagesUsingMach() { useMach = true; }
> 
> Please fix for Webkit style requirements.

fixed.

>> Source/WTF/wtf/ThreadMessage.cpp:212
>> +    return MessageStatus::ThreadExited;
> 
> Unreachable code.

Whoops, fixed.

[Keith Miller]

Created attachment 309803 [details]
Patch

[Build Bot]

Attachment 309803 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:428:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:129:  catch_mach_exception_raise is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:135:  catch_mach_exception_raise_state_identity is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:141:  catch_mach_exception_raise_state is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 309803 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=309803&action=review

Mostly LGTM, just a few comments/questions.

Also a suggestion for a test: Start 10-20 workers running Wasm, have each worker run for a few seconds, and just continually do OOB accesses such that we keep hitting the mach message handler.

> Source/WTF/ChangeLog:22
> +

I would add a link here to some of the overview you sent me.

> Source/JavaScriptCore/runtime/Options.cpp:361
> +#if HAVE(MACH_EXCEPTIONS)
> +    Options::useMachForExceptions() = true;
> +#endif

Why is this needed since this already defaults to true?

> Source/WTF/wtf/Platform.h:666
> +#if __has_include(<mach/mach_exc.defs>) && !(PLATFORM(WATCHOS) || PLATFORM(APPLETV))

Is this true on iOS? What about EWS bots?

> Source/WTF/wtf/ThreadMessage.cpp:168
> +    auto result = thread.suspend();

Nit: a type here would be helpful

> Source/WTF/wtf/ThreadMessage.cpp:194
> +    thread_set_state(thread.machThread(), flavor, (thread_state_t)&registers, userCount);

style nit: Please you static_cast here.

> Source/WTF/wtf/threads/Signals.cpp:64
> +static constexpr size_t maxMessageSize = 1 * KB;

This is scary. Can we make sure there is no number defined somewhere that is more precise?

> Source/WTF/wtf/threads/Signals.cpp:66
> +static void startMachExceptionHandlerThread()

suggestion: I think adding a link to some of the documentation/overview of mach ports would be helpful.

> Source/WTF/wtf/threads/Signals.cpp:73
> +        if (mach_port_insert_right(mach_task_self(), exceptionPort, exceptionPort, MACH_MSG_TYPE_MAKE_SEND)!= KERN_SUCCESS)

style nit: Need space around "!="

> Source/WTF/wtf/threads/Signals.cpp:76
> +        // FIXME: This should use a dispatch queue.

please link a bug or remove.

> Source/WTF/wtf/threads/Signals.cpp:77
> +        (void)Thread::create(

Do we need this here because unused there is a compiler warning for unused?

> Source/WTF/wtf/threads/Signals.cpp:95
> +                        dataLogLn("Failed to receive mach message due to ", mach_error_string(messageResult));

Do we ever expect this to happen?

> Source/WTF/wtf/threads/Signals.cpp:155
> +    dataLogLn("receivedSignal ", exceptionType);
> +    dataLogLn("Thread: ", exceptionData[0]);

Remove.

> Source/WTF/wtf/threads/Signals.cpp:159
> +    *outStateCount = inStateCount;

what does this count mean?

> Source/WTF/wtf/threads/Signals.cpp:178
> +        info.faultingAddress = reinterpret_cast<void*>(exceptionData[1]);

Do we not care about this for SigIll?

> Source/WTF/wtf/threads/Signals.cpp:184
> +        dataLogLn(static_cast<int>(handlerResult));

remove

> Source/WTF/wtf/threads/Signals.cpp:187
> +            didHandle = true;

Do we want to bail out here now that it's handled?

> Source/WTF/wtf/threads/Signals.cpp:252
> +    if (signal == Signal::Bus && useMach)

What about SegV?

> Source/WTF/wtf/threads/Signals.cpp:254
> +    ASSERT(!useMach || signal != Signal::Usr);

Why does this assertion hold given he above check?

[Build Bot]

Comment on attachment 309803 [details]
Patch

Attachment 309803 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3721774

New failing tests:
wasm.yaml/wasm/function-tests/trap-store.js.wasm-eager-jettison
wasm.yaml/wasm/function-tests/trap-store.js.wasm-no-call-ic
wasm.yaml/wasm/function-tests/trap-store.js.default-wasm

[Keith Miller]

Created attachment 309908 [details]
Patch

[Build Bot]

Attachment 309908 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:428:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:137:  catch_mach_exception_raise is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:143:  catch_mach_exception_raise_state_identity is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:149:  catch_mach_exception_raise_state is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 4 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 309908 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=309908&action=review

r=me with fixes.

> Source/WTF/ChangeLog:11
> +        that port in a thread. When another thread raises a exception (say

/a exception/an exception/

> Source/WTF/ChangeLog:20
> +        simply suspending the thread then running the message at that

/thread then/thread, and then/.

> Source/WTF/ChangeLog:23
> +        You can read more abount mach exceptions here:

/abount/about/

> Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:93
> +bool dispatchTerminateCallbackCalled = false;
> +static bool dispatchTermitateCallback(JSContextRef, void*)
> +{
> +    dispatchTerminateCallbackCalled = true;
> +    return true;
> +}

You should enclose this in #if HAVE(MACH_EXCEPTIONS) too because the static function dispatchTermitateCallback is not used otherwise.  If we ever force HAVE(MACH_EXCEPTIONS) to be false for a test build, we'll get a build failure due to this.

> Source/JavaScriptCore/runtime/Options.h:423
> +    v(bool, useMachForExceptions, true, Normal, "Use mach exceptions rather than signals to handle faults and pass thread messages. (This does nothing on platforms without mach)") \

Should default to false.  We override it to true only when HAVE(MACH_EXCEPTIONS) in overrideDefaults().

> Source/WTF/wtf/ThreadMessage.cpp:94
> +        installSignalHandler(signal, [] (Signal, SigInfo&, PlatformRegisters& context) {

Rename context to registers.

> Source/WTF/wtf/ThreadMessage.cpp:104
> +                data->message(context);

Ditto.

> Source/WTF/wtf/ThreadMessage.cpp:214
> +#if HAVE(MACH_EXCEPTIONS)
> +    if (useMach)
> +        return sendMessageMach(thread, message);
> +#endif
> +    return sendMessageSignal(thread, message);

suggestion: sendMessageViaMach instead of sendMessageMach, and sendMessageViaSignal instead of sendMessageSignal?

> Source/WTF/wtf/threads/Signals.cpp:63
> +// You can read more abount mach exceptions here:

typo: /abount/about/.

> Source/WTF/wtf/threads/Signals.cpp:196
> +        switch (handlerResult) {
> +        case SignalAction::Handled:
> +            didHandle = true;
> +            break;
> +        default:
> +            break;
> +        }

You can just say:
    didHandle = didHandle || (handlerResult == SignalAction::Handled);

> Source/WTF/wtf/threads/Signals.cpp:232
> +void registerThread(Thread* thread)

Let's name this "registerThreadForMachExceptionHandling" so that it adds some context in the client code on what the registration is for.

> Source/WTF/wtf/threads/Signals.cpp:241
> +void removeThread(Thread* thread)

small nit: I'd prefer "unregisterThread" to match "registerThread" simply for symmetry.  So, let's name this "unregisterThreadForMachExceptionHandling".

> Source/WTF/wtf/threads/Signals.cpp:257
> +    if (signal == Signal::Bus && useMach)

We should get rid of Signal::Bus as an option that the client can select, and force the signal version to make the Signal::Segv handler handle SIGBUS as well.  For now, just add a FIXME with bug url to fix that later.  Maybe rename Signal::Segv to Signal::BadAccess.

> Source/WTF/wtf/threads/Signals.h:107
> +using WTF::SigInfo;

Somewhere below this, you should add:
#if HAVE(MACH_EXCEPTIONS)
using WTF::registerThreadForMachExceptionHandling;
using WTF::unregisterThreadForMachExceptionHandling;
#endif

The webkit way is to not have to qualify WTF APIs with WTF:: in the client code.  Also using more descriptive names make it clearer what the registration is for in client code.

[Keith Miller]

Comment on attachment 309908 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=309908&action=review

Thanks for the review!

>> Source/WTF/ChangeLog:11
>> +        that port in a thread. When another thread raises a exception (say
> 
> /a exception/an exception/

Fixed.

>> Source/WTF/ChangeLog:20
>> +        simply suspending the thread then running the message at that
> 
> /thread then/thread, and then/.

Fixed.

>> Source/WTF/ChangeLog:23
>> +        You can read more abount mach exceptions here:
> 
> /abount/about/

Fixed.

>> Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:93
>> +}
> 
> You should enclose this in #if HAVE(MACH_EXCEPTIONS) too because the static function dispatchTermitateCallback is not used otherwise.  If we ever force HAVE(MACH_EXCEPTIONS) to be false for a test build, we'll get a build failure due to this.

Fixed.

>> Source/JavaScriptCore/runtime/Options.h:423
>> +    v(bool, useMachForExceptions, true, Normal, "Use mach exceptions rather than signals to handle faults and pass thread messages. (This does nothing on platforms without mach)") \
> 
> Should default to false.  We override it to true only when HAVE(MACH_EXCEPTIONS) in overrideDefaults().

I think it makes more sense to override to false when there is no Mach exception capability. Making it false here would make me think it's actually not running by default.

>> Source/WTF/wtf/ThreadMessage.cpp:94
>> +        installSignalHandler(signal, [] (Signal, SigInfo&, PlatformRegisters& context) {
> 
> Rename context to registers.

Fixed.

>> Source/WTF/wtf/ThreadMessage.cpp:104
>> +                data->message(context);
> 
> Ditto.

Fixed.

>> Source/WTF/wtf/ThreadMessage.cpp:214
>> +    return sendMessageSignal(thread, message);
> 
> suggestion: sendMessageViaMach instead of sendMessageMach, and sendMessageViaSignal instead of sendMessageSignal?

Fair enough. I changed it to sendMessageUsingMach/UsingSignal since that's more consistent with the naming I used elsewhere.

>> Source/WTF/wtf/threads/Signals.cpp:63
>> +// You can read more abount mach exceptions here:
> 
> typo: /abount/about/.

Fixed.

>> Source/WTF/wtf/threads/Signals.cpp:196
>> +        }
> 
> You can just say:
>     didHandle = didHandle || (handlerResult == SignalAction::Handled);

Changed. I did |= though.

>> Source/WTF/wtf/threads/Signals.cpp:232
>> +void registerThread(Thread* thread)
> 
> Let's name this "registerThreadForMachExceptionHandling" so that it adds some context in the client code on what the registration is for.

Fair enough. Changed.

>> Source/WTF/wtf/threads/Signals.cpp:241
>> +void removeThread(Thread* thread)
> 
> small nit: I'd prefer "unregisterThread" to match "registerThread" simply for symmetry.  So, let's name this "unregisterThreadForMachExceptionHandling".

Sounds good.

>> Source/WTF/wtf/threads/Signals.cpp:257
>> +    if (signal == Signal::Bus && useMach)
> 
> We should get rid of Signal::Bus as an option that the client can select, and force the signal version to make the Signal::Segv handler handle SIGBUS as well.  For now, just add a FIXME with bug url to fix that later.  Maybe rename Signal::Segv to Signal::BadAccess.

Done.

>> Source/WTF/wtf/threads/Signals.h:107
>> +using WTF::SigInfo;
> 
> Somewhere below this, you should add:
> #if HAVE(MACH_EXCEPTIONS)
> using WTF::registerThreadForMachExceptionHandling;
> using WTF::unregisterThreadForMachExceptionHandling;
> #endif
> 
> The webkit way is to not have to qualify WTF APIs with WTF:: in the client code.  Also using more descriptive names make it clearer what the registration is for in client code.

Changed.

[Keith Miller]

Created attachment 309977 [details]
Patch for landing

[Keith Miller]

Created attachment 309980 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 309980 [details]
Patch for landing

Clearing flags on attachment: 309980

Committed r216808: <http://trac.webkit.org/changeset/216808>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

(In reply to WebKit Commit Bot from comment #49)
> Comment on attachment 309980 [details]
> Patch for landing
> 
> Clearing flags on attachment: 309980
> 
> Committed r216808: <http://trac.webkit.org/changeset/216808>

Wooohooo!

[Yusuke Suzuki]

(In reply to Filip Pizlo from comment #51)
> (In reply to WebKit Commit Bot from comment #49)
> > Comment on attachment 309980 [details]
> > Patch for landing
> > 
> > Clearing flags on attachment: 309980
> > 
> > Committed r216808: <http://trac.webkit.org/changeset/216808>
> 
> Wooohooo!

One question: Is it OK to execute the message in a different thread?
When using sendMessageUsingMach(), we execute the message handler in the sender thread instead of the target thread. It may change the behavior of TLS in the message handler.

And if the above thing is OK, I think we can make ThreadMessage more portable...
We can just implement ThreadMessage with Thread::suspend(), Thread::getRegisters(), Thread::setRegisters() (It can be implemented easily), and Thread::resume().
If we do so, we can implement ThreadMessage in Windows, which does not have signals.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 172075

[Simon Fraser (smfr)]

This caused lldb to hang when debugging: rdar://problem/32178892