Bug 170976 – Add signaling API

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

Add signaling API

[Keith Miller]

Created attachment 307450 [details]
Proposed API

[Keith Miller]

Comment on attachment 307450 [details]
Proposed API

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

> Source/JavaScriptCore/tools/SignalHandler.h:31
> +    Trap,

This should not be below LastIgnoredSignal.

[JF Bastien]

Comment on attachment 307450 [details]
Proposed API

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

> Source/JavaScriptCore/tools/SignalHandler.cpp:84
> +    auto currentNode = handlers[static_cast<size_t>(signal)].load();

You only need std::memory_order_acquire here.

> Source/JavaScriptCore/tools/SignalHandler.cpp:87
> +            didHandle = true;

Shouldn't this break out? I guess you're cascading all the installed ones all the way to the original one, and I'd expect a way for an installed handler to say "bail out".

> Source/JavaScriptCore/tools/SignalHandler.cpp:89
> +        currentNode = currentNode->next.load();

Ditto.

> Source/JavaScriptCore/tools/SignalHandler.cpp:121
> +    newNode->next.store(nullptr);

This doesn't need to be atomic (or can just be relaxed) because you haven't published it yet. The cmpxchg below publishes it.

> Source/JavaScriptCore/tools/SignalHandler.cpp:123
> +    auto currentNode = handlers[static_cast<size_t>(signal)].compareExchangeStrong(nullptr, newNode);

This can be acq_rel.

Why not fold this into the while loop below so you only have one cmpxchg?

> Source/JavaScriptCore/tools/SignalHandler.cpp:131
> +    }

There's a window here where you've successfully set the `handlers[signal]` value, but haven't called `call_once` yet to initialize `oldActions[signal]`. If a signal gets handled here, is the code correct?

> Source/JavaScriptCore/tools/SignalHandler.h:36
> +    Bus,

SIGBUS versus SIGSEGV are tricky because MacOS raises SIGBUS where Linux raises SIGSEGV, except when it's an actual x86 segmentation fault. The API we're offering here is portable in that it'll install and deliver signals, but it's not portable in that different platforms will deliver different signals in particular situations.

Is that OK?

> Source/JavaScriptCore/tools/SignalHandler.h:39
> +};

Why only those?

FWIW C11 knows about:
SIGABRT
SIGFPE
SIGILL
SIGINT
SIGSEGV
SIGTERM

I guess we don't want to let users of this code raise() as well? C11 does both.

> Source/JavaScriptCore/tools/SignalHandler.h:50
> +// Thus it is your responibility to be able to detect the signal fired was yours or someone elses.

"else's"

[Keith Miller]

Comment on attachment 307450 [details]
Proposed API

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

>> Source/JavaScriptCore/tools/SignalHandler.cpp:84
>> +    auto currentNode = handlers[static_cast<size_t>(signal)].load();
> 
> You only need std::memory_order_acquire here.

Yeah, I didn't think too hard about the memory ordering constraints here. I can fix those.

>> Source/JavaScriptCore/tools/SignalHandler.cpp:87
>> +            didHandle = true;
> 
> Shouldn't this break out? I guess you're cascading all the installed ones all the way to the original one, and I'd expect a way for an installed handler to say "bail out".

Yeah, it's not really clear what that means though. There isn't really any way to know if the signal was intended for you exclusively you or someone else too. e.g. IIUC there could be two threads A and B sending a SigUsr1 to thread C. By the time the SigUsr1 is actually delivered the other one could have been discarded. If thread A's handler exits early then thread B's handler may never gets a chance to run.

>> Source/JavaScriptCore/tools/SignalHandler.cpp:89
>> +        currentNode = currentNode->next.load();
> 
> Ditto.

ditto.

>> Source/JavaScriptCore/tools/SignalHandler.cpp:121
>> +    newNode->next.store(nullptr);
> 
> This doesn't need to be atomic (or can just be relaxed) because you haven't published it yet. The cmpxchg below publishes it.

Ditto.

>> Source/JavaScriptCore/tools/SignalHandler.cpp:123
>> +    auto currentNode = handlers[static_cast<size_t>(signal)].compareExchangeStrong(nullptr, newNode);
> 
> This can be acq_rel.
> 
> Why not fold this into the while loop below so you only have one cmpxchg?

Ditto.

I feel like it's easier to read this way. Otherwise you need to be iterating over the Atomic itself which is harder to work with.

>> Source/JavaScriptCore/tools/SignalHandler.cpp:131
>> +    }
> 
> There's a window here where you've successfully set the `handlers[signal]` value, but haven't called `call_once` yet to initialize `oldActions[signal]`. If a signal gets handled here, is the code correct?

I think it's safe to assume that you cannot send signals to any thread until your handler has been registered. Otherwise, your signal could just as easily arrive before this function is even called.

>> Source/JavaScriptCore/tools/SignalHandler.h:36
>> +    Bus,
> 
> SIGBUS versus SIGSEGV are tricky because MacOS raises SIGBUS where Linux raises SIGSEGV, except when it's an actual x86 segmentation fault. The API we're offering here is portable in that it'll install and deliver signals, but it's not portable in that different platforms will deliver different signals in particular situations.
> 
> Is that OK?

We could make this more abstract and bunch Bus and SegV into a Memory case. I'm not opposed to such an idea as I expect anyone trying to handle one of those also wants to handle the other.

>> Source/JavaScriptCore/tools/SignalHandler.h:39
>> +};
> 
> Why only those?
> 
> FWIW C11 knows about:
> SIGABRT
> SIGFPE
> SIGILL
> SIGINT
> SIGSEGV
> SIGTERM
> 
> I guess we don't want to let users of this code raise() as well? C11 does both.

Right now sending a signal is your own business. Perhaps we should add such an API but I'm not sure what that would look like just yet. Each user of signals does very different things right now.

>> Source/JavaScriptCore/tools/SignalHandler.h:50
>> +// Thus it is your responibility to be able to detect the signal fired was yours or someone elses.
> 
> "else's"

fixed.

[Mark Lam]

Comment on attachment 307450 [details]
Proposed API

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

Some comments for now while I think about this some more.

> Source/JavaScriptCore/ChangeLog:9
> +        * tools/SignalHandler.cpp: Added.

Let's put this file in the runtime folder instead since it is part of the runtime.  I typically only put debugging tools for VM hackers in the tools folder.

> Source/JavaScriptCore/ChangeLog:14
> +        * tools/SignalHandler.h: Added.

Ditto.

>>> Source/JavaScriptCore/tools/SignalHandler.h:36
>>> +    Bus,
>> 
>> SIGBUS versus SIGSEGV are tricky because MacOS raises SIGBUS where Linux raises SIGSEGV, except when it's an actual x86 segmentation fault. The API we're offering here is portable in that it'll install and deliver signals, but it's not portable in that different platforms will deliver different signals in particular situations.
>> 
>> Is that OK?
> 
> We could make this more abstract and bunch Bus and SegV into a Memory case. I'm not opposed to such an idea as I expect anyone trying to handle one of those also wants to handle the other.

I vote for keeping them distinct for now unless strong evidence arises to suggest we should do otherwise.  The implementation can choose to map one signal onto another, but at the abstraction level, let's keep the intent clear as to what condition / scenario we are actually trying to catch / trap on.

>>> Source/JavaScriptCore/tools/SignalHandler.h:50
>>> +// Thus it is your responibility to be able to detect the signal fired was yours or someone elses.
>> 
>> "else's"
> 
> fixed.

Typo: /responibility/responsibility/.

Let's rephrase "to be able to detect the signal fired was yours" to "to discern if the signal fired was yours".

> Source/JavaScriptCore/tools/SignalHandler.h:51
> +// This function is currently a one way street.

I would clarify a bit as follows:
"... one way street i.e. once installed, a signal handler cannot be uninstalled."

One might be able to infer this, but I think it's better to be specific about what "one way street" actually means.  We could get rid of the "one way street" and just state what it means, but I like poetic language, and it's easier to remember than the stuff after the "i.e." (once the context has been clarified).  So, I vote for keeping it.

[Keith Miller]

Thinking about this more I think we could have a message passing API using SIGUSR1. This would work by having each thread hold a lockless queue of SharedTasks. Whenever a thread receives a SIGUSR1 it iterates this queue and runs all the tasks. This is essentially what we do in VMTraps for SIGUSR1 and what I want to do with it for Wasm.

[Keith Miller]

Comment on attachment 307450 [details]
Proposed API

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

>> Source/JavaScriptCore/ChangeLog:9
>> +        * tools/SignalHandler.cpp: Added.
> 
> Let's put this file in the runtime folder instead since it is part of the runtime.  I typically only put debugging tools for VM hackers in the tools folder.

I actually moved it to WTF since that seems like the more appropriate place for it anyway.

>> Source/JavaScriptCore/ChangeLog:14
>> +        * tools/SignalHandler.h: Added.
> 
> Ditto.

ditto.

>>>> Source/JavaScriptCore/tools/SignalHandler.h:50
>>>> +// Thus it is your responibility to be able to detect the signal fired was yours or someone elses.
>>> 
>>> "else's"
>> 
>> fixed.
> 
> Typo: /responibility/responsibility/.
> 
> Let's rephrase "to be able to detect the signal fired was yours" to "to discern if the signal fired was yours".

Fixed.

>> Source/JavaScriptCore/tools/SignalHandler.h:51
>> +// This function is currently a one way street.
> 
> I would clarify a bit as follows:
> "... one way street i.e. once installed, a signal handler cannot be uninstalled."
> 
> One might be able to infer this, but I think it's better to be specific about what "one way street" actually means.  We could get rid of the "one way street" and just state what it means, but I like poetic language, and it's easier to remember than the stuff after the "i.e." (once the context has been clarified).  So, I vote for keeping it.

Sounds good.

[Keith Miller]

Created attachment 307530 [details]
updated API

[Keith Miller]

Created attachment 307531 [details]
updated API

[Mark Lam]

Comment on attachment 307531 [details]
updated API

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

> Source/WTF/wtf/LocklessBag.h:56
> +        while (!m_head.value.compare_exchange_strong(oldHead, newNode)) {
> +            newNode->next = oldHead;
> +        }

Don't need { } because there's only 1 line in the body.

Also, nit: m_head is a WTF:Atomic but you're relying on its underlying value implementation being a std::atomic.  Why not use the Atomic::compareExchangeStrong() instead?

> Source/WTF/wtf/LocklessBag.h:63
> +    void iterate(const Functor& func)

At first I was wondering how this iterate() can be safe.  And then I see how you use it below.  Basically, it's safe because:
1. there's no race with consumeAll() because it is only used by the consumer, and
2. we don't worry about the race with add() because add() always adds to the front of the list.

It's be nice if we can at least document this detail so that no one modifies add() in such a way that breaks this in the future.  I can't think of a way to assert or guarantee it in code.

> Source/WTF/wtf/ThreadMessage.cpp:58
> +    Read = 0,
> +    Write = 1,
> +    Num = 2,

Can you spell out Num fully as NumberOfFileDescriptors?

> Source/WTF/wtf/ThreadMessage.cpp:62
> +static const char* magicByte = "d";

You can make this "static const char* const magicByte" to indicate that you don't intend to ever change the value of magicByte.  The "const char*" only says that the initial value "d" is not mutable via magicByte.  It doesn't say anything about the const-ness of magicByte itself.

> Source/WTF/wtf/ThreadMessage.cpp:75
> +        installSignalHandler(Signal::Usr, [] (int, siginfo_t*, void*) {

There's a disconnect between specifying Signal::Usr here and then invoking thread.signal(SIGUSR2) below.  You should make the 2 consistent: either have installSignalHandler use the system SIGxxx values, or change Thread::signal() to take your Signal enums.  I suggest going with the system signal if possible.

> Source/WTF/wtf/ThreadMessage.cpp:79
> +                dataLogLn("We somehow got a message on a thread that didn't have a WTF::Thread initialized, we probably deadlocked, halp.");

/halp/help/.  Or are you being cute?

> Source/WTF/wtf/ThreadMessage.cpp:95
> +    bool result = thread.signal(SIGUSR2);

This is inconsistent with the signal id/type passed to installSignalHandler above.

> Source/WTF/wtf/ThreadMessage.cpp:119
> +        int flags = fcntl(fileDescriptors[Read], F_GETFL);
> +        ASSERT(!(flags & O_NONBLOCK));
> +        fcntl(fileDescriptors[Read], F_SETFL, flags | O_NONBLOCK);
> +
> +        while (true) {
> +            if (read(fileDescriptors[Read], buffer, 16) == -1) {
> +                ASSERT(errno == EAGAIN);
> +                break;
> +            }
> +        }
> +
> +        fcntl(fileDescriptors[Read], F_SETFL, flags);

Pardon my ignorance, but why is this needed?

Given the assertion that !(flags & O_NONBLOCK), I take it that the read() at line 105 is blocking.  What happens if the target thread dies and our SIGUSR2 just went into the aether.  Will we be stuck waiting forever at the blocking read()?

> Source/WTF/wtf/ThreadMessage.h:36
> +void initializeMessages();

Why make this public?  Why not make it static and just call it in the call_once in sendMessage().  We wouldn't need to call it otherwise, right?

> Source/WTF/wtf/threads/Signals.cpp:86
> +        dataLogLn("We somehow got called for an unknown signal, halp.");

halp?

[JF Bastien]

Comment on attachment 307531 [details]
updated API

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

> Source/WTF/wtf/LocklessBag.h:67
> +            func(node->data);

Is func allowed to modify data? As written it currently is, and that's bad if multiple consumers are doing it at the same time.

> Source/WTF/wtf/LocklessBag.h:77
> +            func(std::forward<T>(node->data));

I think you want move here?

From this code it looks like consumers are not allowed to iterate concurrently with consumeAll. This should be documented.

> Source/WTF/wtf/ThreadMessage.cpp:46
> +        : message(WTFMove(m))

forward

> Source/WTF/wtf/ThreadMessage.cpp:48
> +        ran.store(false);

Can you just initialize it in the constructor initializer list?

> Source/WTF/wtf/ThreadMessage.cpp:93
> +    std::unique_ptr<ThreadMessageData> data = std::make_unique<ThreadMessageData>(WTFMove(message));

forward

> Source/WTF/wtf/ThreadMessage.cpp:104
> +        char buffer[16];

Make 16 a constexpr, and use it here + below.

> Source/WTF/wtf/threads/Signals.cpp:67
> +static LocklessBag<SignalHandler> handlers[static_cast<size_t>(Signal::NumberOfSignals)];

Technically you need to do:
static LocklessBag<SignalHandler> handlers[...blah...] = { ATOMIC_INIT(nullptr) };

ATOMIC_INIT is guaranteed to be able to perform static initialization. Atomics need to be zero-initialized, which doesn't mean putting the zero value in them because reasons.

> Source/WTF/wtf/threads/Signals.cpp:86
> +        dataLogLn("We somehow got called for an unknown signal, halp.");

Log `sig` here.

> Source/WTF/wtf/threads/Signals.cpp:117
> +    handlers[static_cast<size_t>(signal)].add(WTFMove(handler));

forward

[Keith Miller]

Comment on attachment 307531 [details]
updated API

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

>> Source/WTF/wtf/LocklessBag.h:56
>> +        }
> 
> Don't need { } because there's only 1 line in the body.
> 
> Also, nit: m_head is a WTF:Atomic but you're relying on its underlying value implementation being a std::atomic.  Why not use the Atomic::compareExchangeStrong() instead?

If I used Atomic::compareExchangeStrong the code would need to be:

Node* head;
Node* oldHead = newNode->next
while((head = m_head.compareExchangeStrong(oldHead, newNode) == oldHead) {
    newNode->next = head;
    oldHead = head;
}

or some other variant that has an extra local, which I think is harder to read.

>> Source/WTF/wtf/LocklessBag.h:63
>> +    void iterate(const Functor& func)
> 
> At first I was wondering how this iterate() can be safe.  And then I see how you use it below.  Basically, it's safe because:
> 1. there's no race with consumeAll() because it is only used by the consumer, and
> 2. we don't worry about the race with add() because add() always adds to the front of the list.
> 
> It's be nice if we can at least document this detail so that no one modifies add() in such a way that breaks this in the future.  I can't think of a way to assert or guarantee it in code.

I added some more commentary at the top of the file and made the Consumer functions comment all caps.

>> Source/WTF/wtf/LocklessBag.h:67
>> +            func(node->data);
> 
> Is func allowed to modify data? As written it currently is, and that's bad if multiple consumers are doing it at the same time.

I made it harder for the func to think they can modify the data by forcing the func to take the data as a const.

There is only one consumer. That's the rule, if there is more than one consumer you're gonna have a bad time.

>> Source/WTF/wtf/LocklessBag.h:77
>> +            func(std::forward<T>(node->data));
> 
> I think you want move here?
> 
> From this code it looks like consumers are not allowed to iterate concurrently with consumeAll. This should be documented.

Ditto.

>> Source/WTF/wtf/ThreadMessage.cpp:46
>> +        : message(WTFMove(m))
> 
> forward

I think it should be move: ThreadMessage = Ref<SharedTask<void()>>. Isn't forward the same as move at that point.

>> Source/WTF/wtf/ThreadMessage.cpp:48
>> +        ran.store(false);
> 
> Can you just initialize it in the constructor initializer list?

I had to add a constructor to WTF::Atomic but yeah.

>> Source/WTF/wtf/ThreadMessage.cpp:58
>> +    Num = 2,
> 
> Can you spell out Num fully as NumberOfFileDescriptors?

Sure.

>> Source/WTF/wtf/ThreadMessage.cpp:62
>> +static const char* magicByte = "d";
> 
> You can make this "static const char* const magicByte" to indicate that you don't intend to ever change the value of magicByte.  The "const char*" only says that the initial value "d" is not mutable via magicByte.  It doesn't say anything about the const-ness of magicByte itself.

Done.

>> Source/WTF/wtf/ThreadMessage.cpp:75
>> +        installSignalHandler(Signal::Usr, [] (int, siginfo_t*, void*) {
> 
> There's a disconnect between specifying Signal::Usr here and then invoking thread.signal(SIGUSR2) below.  You should make the 2 consistent: either have installSignalHandler use the system SIGxxx values, or change Thread::signal() to take your Signal enums.  I suggest going with the system signal if possible.

I would rather describe the exact set of signals we are going to use, which the enum does. I'll just export the toSystemSignal and fromSystemSignal functions.

>> Source/WTF/wtf/ThreadMessage.cpp:79
>> +                dataLogLn("We somehow got a message on a thread that didn't have a WTF::Thread initialized, we probably deadlocked, halp.");
> 
> /halp/help/.  Or are you being cute?

cute!

This shouldn't actually ever been seen

>> Source/WTF/wtf/ThreadMessage.cpp:93
>> +    std::unique_ptr<ThreadMessageData> data = std::make_unique<ThreadMessageData>(WTFMove(message));
> 
> forward

ditto.

>> Source/WTF/wtf/ThreadMessage.cpp:104
>> +        char buffer[16];
> 
> Make 16 a constexpr, and use it here + below.

fixed.

>> Source/WTF/wtf/ThreadMessage.cpp:119
>> +        fcntl(fileDescriptors[Read], F_SETFL, flags);
> 
> Pardon my ignorance, but why is this needed?
> 
> Given the assertion that !(flags & O_NONBLOCK), I take it that the read() at line 105 is blocking.  What happens if the target thread dies and our SIGUSR2 just went into the aether.  Will we be stuck waiting forever at the blocking read()?

I added this comment:

It's always safe to clear the pipe because only one thread can ever block trying to read from the pipe. Thus, each byte we clear from the pipe actually just corresponds to some task that has already finished. We actively want to ensure that the pipe does not overfill because otherwise our writers might spin trying to write.

Also we don't have to worry about the Thread dying because it should be strongly referenced by the sending thread.

>> Source/WTF/wtf/ThreadMessage.h:36
>> +void initializeMessages();
> 
> Why make this public?  Why not make it static and just call it in the call_once in sendMessage().  We wouldn't need to call it otherwise, right?

We want to be extra sure we get our pipe, I plan on calling this from initialized threading.

>> Source/WTF/wtf/threads/Signals.cpp:67
>> +static LocklessBag<SignalHandler> handlers[static_cast<size_t>(Signal::NumberOfSignals)];
> 
> Technically you need to do:
> static LocklessBag<SignalHandler> handlers[...blah...] = { ATOMIC_INIT(nullptr) };
> 
> ATOMIC_INIT is guaranteed to be able to perform static initialization. Atomics need to be zero-initialized, which doesn't mean putting the zero value in them because reasons.

My LocklessBag has a trivial construction which sets the atomic to nullptr is it good enough to call that global constructor?

>>> Source/WTF/wtf/threads/Signals.cpp:86
>>> +        dataLogLn("We somehow got called for an unknown signal, halp.");
>> 
>> halp?
> 
> Log `sig` here.

Fixed.

>> Source/WTF/wtf/threads/Signals.cpp:117
>> +    handlers[static_cast<size_t>(signal)].add(WTFMove(handler));
> 
> forward

It's a WTF::Function, so that's the same as a move.

[Filip Pizlo]

Comment on attachment 307531 [details]
updated API

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

>>> Source/WTF/wtf/LocklessBag.h:56
>>> +        }
>> 
>> Don't need { } because there's only 1 line in the body.
>> 
>> Also, nit: m_head is a WTF:Atomic but you're relying on its underlying value implementation being a std::atomic.  Why not use the Atomic::compareExchangeStrong() instead?
> 
> If I used Atomic::compareExchangeStrong the code would need to be:
> 
> Node* head;
> Node* oldHead = newNode->next
> while((head = m_head.compareExchangeStrong(oldHead, newNode) == oldHead) {
>     newNode->next = head;
>     oldHead = head;
> }
> 
> or some other variant that has an extra local, which I think is harder to read.

I find the current version super hard to read.  I think the biggest problem with your algo is that you actually have two places where you set newNode->next, which makes it hard to see the workflow.

I would have written this as:

Node* newNode = newNode();
newNode->data = std::forward<T>(element);
for (;;) {
    Node* oldHead = m_head.load();
    newNode->next = oldHead;
    if (m_head.compareExchangeWeak(oldHead, newNode))
        break;
}

I think it's better to use weak CAS here because your algorithm is OK with spurious CAS failure.  You can probably write this loop a million other ways.  I like it better than your loop for two reasons:

- There is one place in the code where we load from m_head.  I think this makes it easier to reason about interleavings.
- There's no need to use the ugly std::atomic API.  I think that the & behavior of the first argument to std::atomic::compare_exchange is an anti-pattern.

You can also accomplish this with Atomic<>::transaction:

m_head.transaction(
    [&] (Node*& head) {
        newNode->next = head;
        head = newNode;
    });

Though you'd have to test if that works well on ARM (It should, but there's the risk that the store cancels the reservation).  If it works then this is clearly better.

In general, I think we should use WTF::Atomic<> API.  If we _did_ find that there was a use for the return-both-a-boolean-and-the-old-value behavior of std::atomic::compare_exchange, then we should add a forwarding function for it, like a new WTF::Atomic<>::compareExchange overload or a new version that returns a struct that has both a bool result and the old value.

[Saam Barati]

Comment on attachment 307531 [details]
updated API

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

> Source/WTF/wtf/LocklessBag.h:48
> +    PushResult add(T element) {

I think you want T&& here to get forwarding semantics.

>>> Source/WTF/wtf/ThreadMessage.cpp:93
>>> +    std::unique_ptr<ThreadMessageData> data = std::make_unique<ThreadMessageData>(WTFMove(message));
>> 
>> forward
> 
> ditto.

Why not just stack allocate this thing?

> Source/WTF/wtf/ThreadMessage.cpp:109
> +        ASSERT(!(flags & O_NONBLOCK));

seems like this is worth a RELEASE_ASSERT since this algorithm would be broken otherwise.

[Saam Barati]

Comment on attachment 307531 [details]
updated API

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

>> Source/WTF/wtf/ThreadMessage.cpp:95
>> +    bool result = thread.signal(SIGUSR2);
> 
> This is inconsistent with the signal id/type passed to installSignalHandler above.

Nit: I think this should also use the nice enum for Signal::Usr.

[Build Bot]

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


ERROR: Source/WTF/wtf/ThreadMessage.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/LocklessBag.h:48:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/LocklessBag.h:56:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/threads/Signals.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 9 files


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

[Keith Miller]

Created attachment 307695 [details]
Patch

[Mark Lam]

(In reply to Keith Miller from comment #17)
> Created attachment 307695 [details]
> Patch

"patch does not apply to trunk of repository".  Please rebase.

[Keith Miller]

Created attachment 307731 [details]
Patch

[Keith Miller]

Created attachment 307739 [details]
Patch for landing

[Keith Miller]

Created attachment 307741 [details]
Patch for landing

[Keith Miller]

Created attachment 307744 [details]
Patch for landing

[JF Bastien]

Comment on attachment 307744 [details]
Patch for landing

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

> Source/WTF/wtf/LocklessBag.h:54
> +        newNode->data = WTFMove(element);

You should use T&& above, and std::forward here.

> Source/WTF/wtf/ThreadMessage.cpp:42
> +const bool verbose = true;

expeliamo verbose

> Source/WTF/wtf/ThreadMessage.cpp:85
> +SUPPRESS_ASAN

Ca you comment on what ASAN dislikes?

> Source/WTF/wtf/ThreadMessage.cpp:107
> +            // other calls which set it at the same time.

I don't understand this comment. errno is thread-safe.

> Source/WTF/wtf/ThreadMessage.cpp:144
> +            // other calls which set it at the same time.

Ditto

> Source/WTF/wtf/threads/Signals.cpp:103
> +    handlers[static_cast<size_t>(signal)].add(WTFMove(handler));

std::forward

> Source/WTF/wtf/threads/Signals.cpp:108
> +        sigfillset(&action.sa_mask);

RELEASE_ASSERT this returns 0.

> Source/WTF/wtf/threads/Signals.cpp:110
> +        sigaction(toSystemSignal(signal), &action, &oldActions[static_cast<size_t>(signal)]);

Ditto

> Source/WTF/wtf/threads/Signals.h:38
> +    Usr,

Why not call it USR2 since that's what we're using?

[Keith Miller]

Comment on attachment 307744 [details]
Patch for landing

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

>> Source/WTF/wtf/LocklessBag.h:54
>> +        newNode->data = WTFMove(element);
> 
> You should use T&& above, and std::forward here.

Fixed.

>> Source/WTF/wtf/ThreadMessage.cpp:42
>> +const bool verbose = true;
> 
> expeliamo verbose

I just removed this.

>> Source/WTF/wtf/ThreadMessage.cpp:85
>> +SUPPRESS_ASAN
> 
> Ca you comment on what ASAN dislikes?

We are touching the stack of another thread (see 118), in the past ASAN didn't like this very much.

>> Source/WTF/wtf/ThreadMessage.cpp:107
>> +            // other calls which set it at the same time.
> 
> I don't understand this comment. errno is thread-safe.

TIL, fixed

>> Source/WTF/wtf/ThreadMessage.cpp:144
>> +            // other calls which set it at the same time.
> 
> Ditto

ditto.

>> Source/WTF/wtf/threads/Signals.cpp:103
>> +    handlers[static_cast<size_t>(signal)].add(WTFMove(handler));
> 
> std::forward

We know the handlers type so we might as well move. I think move is cleaner.

>> Source/WTF/wtf/threads/Signals.cpp:108
>> +        sigfillset(&action.sa_mask);
> 
> RELEASE_ASSERT this returns 0.

done.

>> Source/WTF/wtf/threads/Signals.cpp:110
>> +        sigaction(toSystemSignal(signal), &action, &oldActions[static_cast<size_t>(signal)]);
> 
> Ditto

done.

>> Source/WTF/wtf/threads/Signals.h:38
>> +    Usr,
> 
> Why not call it USR2 since that's what we're using?

We are probably only ever going to use one Usr signal, since we have the API I just added. We can always add the numbers later if we add the other.

[Keith Miller]

Created attachment 307751 [details]
Patch for landing

[Keith Miller]

Created attachment 307755 [details]
Patch for landing

[Keith Miller]

Committed r215620: <http://trac.webkit.org/changeset/215620>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 171139

[WebKit Commit Bot]

Comment on attachment 307755 [details]
Patch for landing

Rejecting attachment 307755 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 307755, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
 succeeded at 1 with fuzz 3.
patching file Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
Hunk #1 FAILED at 174.
Hunk #2 FAILED at 1117.
Hunk #3 FAILED at 2179.
Hunk #4 FAILED at 2700.
4 out of 4 hunks FAILED -- saving rejects to file Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj.rej
patching file Tools/TestWebKitAPI/Tests/WTF/ThreadMessages.cpp

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/3578179

[Yusuke Suzuki]

SIGUSR2 signal is already used in Linux WebKit ports to suspend and resume threads.
I'm investigating the way to adopt these changes in Linux.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #30)
> SIGUSR2 signal is already used in Linux WebKit ports to suspend and resume
> threads.
> I'm investigating the way to adopt these changes in Linux.

Some user-defined signal handlers require some context. For example, in Linux port SIGUSR2, signal handler is invoked after some setup.
So, as a result, just redirecting SIGUSR2 to the old default signal handler causes incorrect behaivor of the original signal handler. In this case, ThreadMessage's SIGUSR2 redirects SIGUSR2 handler, and suspend/resume handler is invoked without the setup, and causes SEGV.
So, I think redirecting Signal::Usr is not meaningful. It means that the original SIGUSR2 handler will be invoked with ThreadMessage's kill() call. And it will pose incorrect behavior.