Bug 115083

Summary: fourthTier: CFA should defend against results seeming inconsistent due to a watchpoint firing during compilation
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, ggaren, mark.lam, mhahnenberg, msaboff, oliver, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 115084    
Bug Blocks: 112839    
Attachments:
Description Flags
the patch
ggaren: review+
patch for landing none

Filip Pizlo
Reported 2013-04-23 21:26:08 PDT
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.
Attachments
the patch (30.13 KB, patch)
2013-04-26 16:41 PDT, Filip Pizlo 		ggaren: review+
DetailsFormatted DiffDiff
patch for landing (38.86 KB, patch)
2013-04-26 21:03 PDT, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Filip Pizlo
Comment 1 2013-04-26 16:41:37 PDT
Created attachment 199874 [details] the patch
Geoffrey Garen
Comment 2 2013-04-26 17:18:27 PDT
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.
Filip Pizlo
Comment 3 2013-04-26 20:48:00 PDT
(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
Filip Pizlo
Comment 4 2013-04-26 21:03:28 PDT
Created attachment 199886 [details] patch for landing Includes suggested changes.
Filip Pizlo
Comment 5 2013-04-26 22:43:51 PDT
Landed in http://trac.webkit.org/changeset/149233
Note You need to log in before you can comment on or make changes to this bug.