Bug 115083 – fourthTier: CFA should defend against results seeming inconsistent due to a watchpoint firing during compilation

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 199874 [details]
the patch

[Geoffrey Garen]

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]

(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]

Created attachment 199886 [details]
patch for landing

Includes suggested changes.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/149233