This is actually benign in release mode, but it will cause assertions to fire. It's benign because the only way that we would prove something to be true incorrectly if a watchpoint fires during compilation, is if we had thought that the watchpoint was valid and used that to set a watchpoint instead of emitting a check. But if we set a watchpoint then we would record this in the IR and then detect that the watchpoint was no longer valid, and preemptively jettison the compilation before even installing it. But in debug mode, we would see a watchpoint in the IR on a structure that had an invalid watchpoint. We would fire an assertion on this. I'd like to find a way to keep those assertions and make them do sensible things for the cases where we didn't have concurrent watchpoint firing, but to turn them off *exactly* in those cases where there was a race.
Created attachment 199874 [details] the patch
Comment on attachment 199874 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=199874&action=review r=me > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1353 > + if (m_jit.graph().m_watchpoints.isStillValid(m_jit.graph().globalObjectFor(node->codeOrigin)->masqueradesAsUndefinedWatchpoint())) { Let's add a helper function for this, since it's repeated in so many places. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:-3005 > - stringPrototypeStructure->addTransitionWatchpoint(speculationWatchpoint(NotStringObject)); In future, when we have a compilation thread, let's add an ASSERT to WatchpointSet that prohibits access from the compilation thread. We'll see -- it might be appropriate to add stuff like that to SymbolTable and/or Structure, as well.
(In reply to comment #2) > (From update of attachment 199874 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=199874&action=review > > r=me > > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1353 > > + if (m_jit.graph().m_watchpoints.isStillValid(m_jit.graph().globalObjectFor(node->codeOrigin)->masqueradesAsUndefinedWatchpoint())) { > > Let's add a helper function for this, since it's repeated in so many places. I'll do that and also add a helper for: m_jit.addLazily( speculationWatchpoint(), m_jit.graph().globalObjectFor(node->codeOrigin)->masqueradesAsUndefinedWatchpoint()); These two things happen in pairs. > > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:-3005 > > - stringPrototypeStructure->addTransitionWatchpoint(speculationWatchpoint(NotStringObject)); > > In future, when we have a compilation thread, let's add an ASSERT to WatchpointSet that prohibits access from the compilation thread. > > We'll see -- it might be appropriate to add stuff like that to SymbolTable and/or Structure, as well. Filed: https://bugs.webkit.org/show_bug.cgi?id=115297
Created attachment 199886 [details] patch for landing Includes suggested changes.
Landed in http://trac.webkit.org/changeset/149233