909972 - Fix the remaining hazards in AsmJS

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

The three remaining hazards being reported in AsmJS are:

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 home/sfink/src/MI-upstream/js/src/jit/AsmJS.cpp:3395

Function 'AsmJS.cpp:uint8 CheckFuncPtrCall(FunctionCompiler*, js::frontend::ParseNode*, RetType, js::jit::MDefinition**, Type*)' has unrooted 'name' 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 home/sfink/src/MI-upstream/js/src/jit/AsmJS.cpp:3471

Function 'AsmJS.cpp:uint8 CheckFFICall(FunctionCompiler*, js::frontend::ParseNode*, uint32, 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 home/sfink/src/MI-upstream/js/src/jit/AsmJS.cpp:3500

CheckCallArgs calls CheckExpr, which uses |JS_CHECK_RECURSION(f.cx(), return false);|, which calls |js_ReportOverRecursed|. This ends up calling |js_ExpandErrorArguments|, which calls through a function pointer for formatting. The format function we use cannot GC, but the analysis is not smart enough to follow this flow. Secondly, even if this did GC, it should not be a hazard: AsmJS immediately returns false and does not touch the property names again. The analysis seems to not be smart enough to unfold this data-flow either.

Comment 1

[Terrence Cole [:terrence]]

Later on, js_ReportErrorNumberVA calls ReportError, which will call the JSD1 hook, which will re-enter script, so this can actually trigger GC.

Comment 2

[Luke Wagner [:luke]]

The error paths should return before any possible uses of those PropertyNames.

Comment 3

[Terrence Cole [:terrence]]

Yes indeed! We need to pursue the "Secondly" path from comment 0: data-dependent flow analysis or just add an annotation for CheckExpr.

Comment 4

[Luke Wagner [:luke]]

Oops, I didn't see that the last paragraph was words, not spew :)

Comment 6

[Terrence Cole [:terrence]]

Note: the analysis in comment 0 is a bit wrong. js_ReportOverRecursed does GC, but not through the path indicated. Instead it calls the jsd1 hook which enters firebug.

Comment 7

[Terrence Cole [:terrence]]

Actually, it looks like this is going to be quite complicated to annotate our way out of, so lets just fix the code instead.

Comment 8

[Terrence Cole [:terrence]]

Attachment: hazard_AsmJS-v1.diff

The strategy taken here is to delay reporting until the module compiler destructor. Since AsmJS already does this for all errors except over-recursion, this is trivial.

Steve, does this fix the asm.js hazards that the analysis is reporting?

Comment 9

[Terrence Cole [:terrence]]

Comment on attachment 817562 [details] [diff] [review]
hazard_AsmJS-v1.diff

We can use try to test this now. Derp.

https://tbpl.mozilla.org/?tree=Try&rev=d47e567f3205

Comment 10

[Terrence Cole [:terrence]]

And it will likely even work now:
https://tbpl.mozilla.org/?tree=Try&rev=71cf21dcbe1b

Comment 11

[Terrence Cole [:terrence]]

Nope. The recent unrelated regressions triggered failure, which was killing the job before the upload.

A more recent successful job showed us a new regression: fail(nullptr) calls Parser::peekToken, which will atomize, which may GC. This is why so many new asm.js hazards popped up a couple weeks ago. Since this is only when we are already probably in OOM, I think suppressing here is the right thing to do: it will delay the GC until the parser is off the stack, which should allow us to free more anyway.

At long last, success: https://tbpl.mozilla.org/?tree=Try&rev=7db197c35663

Comment 12

[Terrence Cole [:terrence]]

Attachment: hazard_AsmJS-v2.diff

Luke, this implements the technique we discussed above: moving the reporting of over-recursed to the top level to match our other reporting. It also addresses the problems found in the last comment by suppressing GC in fail().

With this patch in place, we have 4 remaining hazard reports in the shell.

Comment 13

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e84bb9b38fa

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4e84bb9b38fa