924206 - Avoid false positives when the only way to GC is to fail

Status: RESOLVED DUPLICATE of bug 909972

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

Example hazard report:

Function 'AsmJS.cpp:uint8 CheckInternalCall(FunctionCompiler*, js::frontend::ParseNode*, js::PropertyName*, RetType, js::jit::MDefinition**, Type*)' has unrooted 'calleeName' of type 'js::PropertyName*' live across GC call 'AsmJS.cpp:uint8 CheckCallArgs(FunctionCompiler*, js::frontend::ParseNode*, (uint8)(FunctionCompiler*,js::frontend::ParseNode*,Type)*, FunctionCompiler::Call*)' at js/src/jit/AsmJS.cpp:3664

If CheckCallArgs GC's, then it will also return false. The relevant calling code:

    if (!CheckCallArgs(f, callNode, CheckIsVarType, &call))
        return false;

so really, 'calleeName' is guaranteed to be dead if CheckCallArgs GC's, but it's dead in a flow-sensitive way.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Add gcFalseFunctions for a small amount of flow-sensitivity

Sneak in a little bit of flow-sensitivity by saying holding unrooted variables live across certain functions ("gcFalseFunctions") should not be considered to be hazards if (1) the hazard is postdominated by a 'return false' and (2) there are no references to the unrooted variable in between the function call and the 'return false'.

Comment 2

[Brian Hackett [Laid off!]]

Comment on attachment 814206 [details] [diff] [review]
Add gcFalseFunctions for a small amount of flow-sensitivity

Review of attachment 814206 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like a lot of analysis complexity to handle a couple random utility functions.  Can you just stick an AutoSuppressGC somewhere to make these false positives go away?

Comment 3

[Steve Fink [:sfink] [:s:]]

(In reply to Brian Hackett (:bhackett) from comment #2)
> Comment on attachment 814206 [details] [diff] [review]
> Add gcFalseFunctions for a small amount of flow-sensitivity
> 
> Review of attachment 814206 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks like a lot of analysis complexity to handle a couple random
> utility functions.  Can you just stick an AutoSuppressGC somewhere to make
> these false positives go away?

Hm. For just these, I could, although it bothers me to degrade behavior to appease the analysis. AutoSuppressGC would prevent it from GC'ing when it needs to in order to allocate an exception object, though admittedly in this case that only happens when you're feeding it malformed AsmJS code.

In general, I'm not sure. I have definitely encountered the exact same false positives in a totally different area. I can't remember what the previous instance was, but it turned out to be code that was going away for a completely different reason. I *think* there are more instances of this in the current set of browser hazards, though obviously not in AsmJS checking functions (so I'd need to annotate a few more.) But I will look to see how widespread it appears to be.

Comment 4

[Terrence Cole [:terrence]]

Just midaired with Steve, who said basically what I was going to say. Here's what I had, although most of it is pretty extraneous now:

Huh, I thought this work was being done in bug 909972. The discussion there may be useful.

To summarize, the problem is that the code causing the GC is JS_CHECK_RECURSION. We obviously don't want to suppress GC on that. We could split out and make a JS_CHECK_RECURSION_NO_GC, but afaict, that ends up calling out to firebug (crazy as that is), so suppressing GC there would not be great. Also, CheckExpr is extremely hot, so adding extra runtime overhead to help with an analysis bug is unpalatable.

Maybe we want something like AutoSuppressRootingAnalysis in CheckExpr? If that would be similarly complex, maybe just annotating that CheckExpr does not GC?

Comment 6

[Steve Fink [:sfink] [:s:]]

Argh. bugzilla checks for midair collisions, but apparently not for added 'blocks' entries, and yet it submits the stale set anyway... re-adding blocked bug.

Comment 7

[Brian Hackett [Laid off!]]

I think the most efficient thing to do would be to actually prevent the AsmJS checking code from ever GC'ing (if JS_CHECK_RECURSION is the only way this happens?) which could be done by setting an over-recursed flag somewhere on the compiler but not generating the exception, and checking the flag on a failed compilation and generating the exception / calling the debugger before returning.  Otherwise you could just use roots where needed, their cost here should be pretty insignificant.  In general I would like to avoid bending the analysis to fit what the code is doing, except when the pattern involved is widespread.  It's much better to have a simple analysis and code that conforms to it, as with the language type system and so forth.  This goes double when the analysis is unsound, as it appears to be from the use of annotations.