[WTF] Make ThreadMessage portable
Created attachment 310023 [details] Patch
Created attachment 310024 [details] Patch
Created attachment 310025 [details] Patch
Interesting thing is suspending / resuming threads is more portable than emitting signals :)
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.
Created attachment 312424 [details] Patch
Now, the mach exception patch is landed. I think this patch is ready to be reviewed.
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?
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?
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/.
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.
Created attachment 312605 [details] Patch
Created attachment 312607 [details] Patch
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?
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 :)
Committed r218080: <http://trac.webkit.org/changeset/218080>