VMTraps has some races
Created attachment 314066 [details] Patch
Attachment 314066 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:311: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 314069 [details] Patch
Attachment 314069 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:310: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 314096 [details] Patch
Attachment 314096 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/VMTraps.cpp:310: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 314096 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314096&action=review r=me > Source/JavaScriptCore/runtime/VMTraps.cpp:275 > + // FIXME: We need to use the machine threads because it is the only non-TLS source > + // for the stack bounds of this thread. We should keep in on the WTF::Thread instead. Add bug id for this FIXME. > Source/JavaScriptCore/runtime/VMTraps.cpp:292 > + nit: Revert whitespace.
Committed r218936: <http://trac.webkit.org/changeset/218936>
Comment on attachment 314096 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314096&action=review LGTM too. Just a few comments. > Source/JavaScriptCore/dfg/DFGCommonData.cpp:108 > + LockHolder locker(pcCodeBlockMapLock); > + auto& map = pcCodeBlockMap(locker); > + for (auto& jumpReplacement : jumpReplacements) > + map.remove(jumpReplacement.dataLocation()); You should make a function for this since it's duplicated below. > Source/JavaScriptCore/dfg/DFGCommonData.cpp:160 > + if (result == map.end()) > + return nullptr; Seems like this should be a RELEASE_ASSERT(result != map.end()) since we always assume we find it. > Source/JavaScriptCore/runtime/VMTraps.cpp:113 > + auto codeBlockSetLocker = isJITPC(trapPC) ? holdLock(lock) : tryHoldLock(lock); isJITPC should be (isJITPC || isLLIntPC) Maybe it's worth making a function for this since you use this general concept in another place too. > Source/JavaScriptCore/runtime/VMTraps.cpp:208 > + SignalContext context(registers); It seems sketchy to do the below work on a different thread than the execution thread. To be super duper safe, I'd just return to a thunk that did this for you. > Source/JavaScriptCore/runtime/VMTraps.cpp:284 > + stackBounds = StackBounds(machineThread->stackBase(), machineThread->stackEnd()); why can't you just get this from the VM?