Bug 172073 – [WTF] Make ThreadMessage portable

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[WTF] Make ThreadMessage portable

[Yusuke Suzuki]

Created attachment 310023 [details]
Patch

[Yusuke Suzuki]

Created attachment 310024 [details]
Patch

[Yusuke Suzuki]

Created attachment 310025 [details]
Patch

[Yusuke Suzuki]

Interesting thing is suspending / resuming threads is more portable than emitting signals :)

[Mark Lam]

Comment on attachment 310025 [details]
Patch

The mach exception patch has been rolled out.  Let's hold off on this patch until that gets resolved first.

[Yusuke Suzuki]

Created attachment 312424 [details]
Patch

[Yusuke Suzuki]

Now, the mach exception patch is landed.
I think this patch is ready to be reviewed.

[Mark Lam]

Comment on attachment 312424 [details]
Patch

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

Looks good in general but I have some questions and concerns.

> Source/JavaScriptCore/runtime/MachineContext.h:39
> +void*& stackPointer(PlatformRegisters&);

Why make this only specific to #if OS(WINDOWS) || HAVE(MACHINE_CONTEXT)?  What's the rationale?

I suspect that this will break the build for platform that are !(OS(WINDOWS) || HAVE(MACHINE_CONTEXT)).  For example, the VMTraps and SigillCrashAnalyzer's SignalContext constructor initializer lists use this function.  One option is to fix them to use the const PlatformRegister& form.  Another option is to disable VMTraps and SigillCrashAnalyzer for !(OS(WINDOWS) || HAVE(MACHINE_CONTEXT)).  I like the second option better.  What do you think?

> Source/JavaScriptCore/runtime/MachineContext.h:121
> +#else

Since this #else is so far away from its corresponding #if, can you add a comment like so:
#else // not OS(WINDOWS) || HAVE(MACHINE_CONTEXT)

> Source/JavaScriptCore/runtime/MachineContext.h:126
> +#endif

Ditto, add a comment:
#endif // OS(WINDOWS) || HAVE(MACHINE_CONTEXT)

> Source/WTF/wtf/ThreadingPthreads.cpp:438
> +#endif

nit: use a comment to pair with the #if please.
#endif // OS(DARWIN)

> Source/WTF/wtf/ThreadingPthreads.cpp:458
> +void Thread::setRegisters(const PlatformRegisters& registers)

I see this defined but not used anywhere.  What's the motivation for introducing this?

[Mark Lam]

Comment on attachment 312424 [details]
Patch

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

> Source/WTF/wtf/ThreadMessage.cpp:-201
> -
> -#endif // USE(PTHREADS)

The whole of ThreadMessage.h appears to be duplicated below when I click on the "100 above" link.  Can you make sure there's no duplication?

[Mark Lam]

Comment on attachment 312424 [details]
Patch

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

Some of my comments seemed to have just disappeared.  I'll try to reconstruct them as much as possible.

> Source/WTF/wtf/ThreadingPthreads.cpp:172
> +#if HAVE(MACHINE_CONTEXT)
> +    registersFromUContext(userContext) = thread->m_platformRegisters;
> +#endif

Is it reasonable to expect HAVE(MACHINE_CONTEXT) for pthreads?  If so, should we add a:

#else
#error "Unable to modify platform registers because !HAVE(MACHINE_CONTEXT)

This way, there's no silent failure catching anyone by surprise.

> Source/WTF/wtf/ThreadingPthreads.cpp:465
> +    ASSERT_WITH_MESSAGE(m_suspendCount, "We can get registers only if the thread is suspended.");

typo: /get/set/.

[Yusuke Suzuki]

Comment on attachment 312424 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/runtime/MachineContext.h:39
>> +void*& stackPointer(PlatformRegisters&);
> 
> Why make this only specific to #if OS(WINDOWS) || HAVE(MACHINE_CONTEXT)?  What's the rationale?
> 
> I suspect that this will break the build for platform that are !(OS(WINDOWS) || HAVE(MACHINE_CONTEXT)).  For example, the VMTraps and SigillCrashAnalyzer's SignalContext constructor initializer lists use this function.  One option is to fix them to use the const PlatformRegister& form.  Another option is to disable VMTraps and SigillCrashAnalyzer for !(OS(WINDOWS) || HAVE(MACHINE_CONTEXT)).  I like the second option better.  What do you think?

We still have `void* stackPointer(const PlatformRegisters&);`. `void*& stackPointer(PlatformRegisters&);` is prepared to perform the operation like,

MachineContext::stackPointer(registers) = newValue;

But the above thing should be effective only in `OS(WINDOWS) || HAVE(MACHINE_CONTEXT)` environment. HAVE(MACHINE_CONTEXT) means that we know how to modify MachineContext.
If HAVE(MACHINE_CONTEXT) is false (and OS(WINDOWS) is false), our PlatformRegister just has approximate stack pointer. Modifying it by using the above form does not have any meaningful effect.

So I think we should not expose `void*& stackPointer(PlatformRegisters&);` function in `OS(WINDOWS) || HAVE(MACHINE_CONTEXT)` environment.

VMTrap depends on HAVE(MACHINE_CONTEXT). And SigillAnalyzer is only enabled in Darwin environment right now. So I think this change does not cause build failures.

>> Source/JavaScriptCore/runtime/MachineContext.h:121
>> +#else
> 
> Since this #else is so far away from its corresponding #if, can you add a comment like so:
> #else // not OS(WINDOWS) || HAVE(MACHINE_CONTEXT)

Nice, fixed.

>> Source/JavaScriptCore/runtime/MachineContext.h:126
>> +#endif
> 
> Ditto, add a comment:
> #endif // OS(WINDOWS) || HAVE(MACHINE_CONTEXT)

Done.

>> Source/WTF/wtf/ThreadMessage.cpp:-201
>> -#endif // USE(PTHREADS)
> 
> The whole of ThreadMessage.h appears to be duplicated below when I click on the "100 above" link.  Can you make sure there's no duplication?

Oh, that's strange... They are removed completely. I hope the updated patch's diff does not show it.

>> Source/WTF/wtf/ThreadingPthreads.cpp:172
>> +#endif
> 
> Is it reasonable to expect HAVE(MACHINE_CONTEXT) for pthreads?  If so, should we add a:
> 
> #else
> #error "Unable to modify platform registers because !HAVE(MACHINE_CONTEXT)
> 
> This way, there's no silent failure catching anyone by surprise.

Or, now, we do not use setRegister(). So, this functionality is not necessary.
We can drop this feature simply. Dropped this phase.

BTW, HAVE(MACHINE_CONTEXT) can be false in USE(PTHREAD) environment if we do not know much about machine context internals.
For example, on non-JIT-supported ISA, we do not have accessors for machine context. In this case, HAVE(MACHINE_CONTEXT) becomes false.

>> Source/WTF/wtf/ThreadingPthreads.cpp:438
>> +#endif
> 
> nit: use a comment to pair with the #if please.
> #endif // OS(DARWIN)

Fixed.

>> Source/WTF/wtf/ThreadingPthreads.cpp:458
>> +void Thread::setRegisters(const PlatformRegisters& registers)
> 
> I see this defined but not used anywhere.  What's the motivation for introducing this?

This is originally introduced to be used in sendMessageUsingMach in the previous patch.
But now, the newly landed one does not include it. So, we can drop this implementation! :)
Dropped.

>> Source/WTF/wtf/ThreadingPthreads.cpp:465
>> +    ASSERT_WITH_MESSAGE(m_suspendCount, "We can get registers only if the thread is suspended.");
> 
> typo: /get/set/.

Fixed.

[Yusuke Suzuki]

Created attachment 312605 [details]
Patch

[Yusuke Suzuki]

Created attachment 312607 [details]
Patch

[Keith Miller]

Comment on attachment 312607 [details]
Patch

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

r=me with comment.

> Source/WTF/wtf/ThreadingPthreads.cpp:402
> +struct ThreadStateOption {

What's the reasoning behind calling this ThreadStateOption? What do you think about calling this ThreadStateMetadata?

[Yusuke Suzuki]

Comment on attachment 312607 [details]
Patch

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

>> Source/WTF/wtf/ThreadingPthreads.cpp:402
>> +struct ThreadStateOption {
> 
> What's the reasoning behind calling this ThreadStateOption? What do you think about calling this ThreadStateMetadata?

It's fine. Renamed :)

[Yusuke Suzuki]

Committed r218080: <http://trac.webkit.org/changeset/218080>