Closed Bug 907791 Opened 11 years ago Closed 11 years ago

Suppress the data-flow-sensitive static analysis hazards in AsmJS.cpp

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch hazards_asmjs-v0.diff (obsolete) — Splinter Review
The static analysis is apparently not smart enough to see that failName returns false all the way up to the compiler root, so is currently identifying uses after this as hazards despite being dynamically unreachable.

The GC path we are suppressing looks like:

AsmJS.cpp:uint8 CheckCallArgs(FunctionCompiler*, js::frontend::ParseNode*, (uint8)(FunctionCompiler*,js::frontend::ParseNode*,Type)*, FunctionCompiler::Call*)
GC Function: AsmJS.cpp:uint8 CheckCallArgs(FunctionCompiler*, js::frontend::ParseNode*, (uint8)(FunctionCompiler*,js::frontend::ParseNode*,Type)*, FunctionCompiler::Call*)
AsmJS.cpp:uint8 CheckExpr(FunctionCompiler*, js::frontend::ParseNode*, js::ion::MDefinition**, Type*)
AsmJS.cpp:uint8 CheckBitwise(FunctionCompiler*, js::frontend::ParseNode*, js::ion::MDefinition**, Type*)
AsmJS.cpp:uint8 CheckCall(FunctionCompiler*, js::frontend::ParseNode*, RetType, js::ion::MDefinition**, Type*)
AsmJS.cpp:uint8 CheckFuncPtrCall(FunctionCompiler*, js::frontend::ParseNode*, RetType, js::ion::MDefinition**, Type*)
AsmJS.cpp:uint8 CheckFuncPtrTableAgainstExisting(ModuleCompiler*, js::frontend::ParseNode*, js::PropertyName*, class mozilla::MoveRef<Signature>, uint32, ModuleCompiler::FuncPtrTable**)
AsmJS.cpp:uint8 {anonymous}::ModuleCompiler::failName(js::frontend::ParseNode*, int8*, js::PropertyName*)
int8* js::AtomToPrintableString(js::ExclusiveContext*, JSAtom*, JSAutoByteString*)
int8* js_ValueToPrintable(JSContext*, JS::Value*, JSAutoByteString*, uint8)
JSString* js::ValueToSource(JSContext*, class JS::Handle<JS::Value>)
uint8 js::Invoke(JSContext*, JS::Value*, JS::Value*, uint32, JS::Value*, class JS::MutableHandle<JS::Value>)
uint8 js::Invoke(JSContext*, JS::CallArgs, uint32)
JSScript* JSFunction::getOrCreateScript(JSContext*)
uint8 JSFunction::createScriptForLazilyInterpretedFunction(JSContext*, class JS::Handle<JSFunction*>)
uint8 JSRuntime::cloneSelfHostedFunctionScript(JSContext*, class JS::Handle<js::PropertyName*>, class JS::Handle<JSFunction*>)
JSScript* js::CloneScript(JSContext*, class JS::Handle<JSObject*>, class JS::Handle<JSFunction*>, const class JS::Handle<JSScript*>, uint32)
JSObject* js::CloneObjectLiteral(JSContext*, class JS::Handle<JSObject*>, class JS::Handle<JSObject*>)
JSObject* js::GlobalObject::getOrCreateObjectPrototype(JSContext*)
JSObject* js::GlobalObject::initFunctionAndObjectClasses(JSContext*)
uint8 js::ObjectImpl::preventExtensions(JSContext*, class JS::Handle<js::ObjectImpl*>)
uint8 js::ObjectImpl::isExtensible(js::ExclusiveContext*, class JS::Handle<js::ObjectImpl*>, uint8*)
uint8 js::Proxy::isExtensible(JSContext*, class JS::Handle<JSObject*>, uint8*)
uint8 CPOWProxyHandler::isExtensible(JSContext*, class JS::Handle<JSObject*>, uint8*)
uint8 mozilla::jsipc::JavaScriptParent::isExtensible(JSContext*, class JS::Handle<JSObject*>, uint8*)
uint8 mozilla::jsipc::JavaScriptParent::ok(JSContext*, mozilla::jsipc::ReturnStatus*)
uint8 mozilla::jsipc::JavaScriptShared::toValue(JSContext*, mozilla::jsipc::JSVariant*, class JS::MutableHandle<JS::Value>)
JSObject* xpc_NewIDObject(JSContext*, class JS::Handle<JSObject*>, nsID*)
FieldCall: nsIXPConnect.WrapNative

And this is triggering hazards like:

Function 'AsmJS.cpp:uint8 CheckInternalCall(FunctionCompiler*, js::frontend::ParseNode*, js::PropertyName*, RetType, js::ion::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
    home/sfink/src/MI-upstream/js/src/jit/AsmJS.cpp:3393: Assign(1,2, __temp_1 := retType*)
    home/sfink/src/MI-upstream/js/src/jit/AsmJS.cpp:3393: Call(2,3, call.Call(f*,__temp_1*))
    home/sfink/src/MI-upstream/js/src/jit/AsmJS.cpp:3395: Call(3,4, __temp_2 := CheckCallArgs(f*,callNode*,CheckIsVarType,call))
    home/sfink/src/MI-upstream/js/src/jit/AsmJS.cpp:3395: Assume(4,7, !__temp_2*, false)
    home/sfink/src/MI-upstream/js/src/jit/AsmJS.cpp:3399: Call(7,8, __temp_4 := f*.m())
    home/sfink/src/MI-upstream/js/src/jit/AsmJS.cpp:3399: Call(8,9, __temp_6 := call.sig())
    home/sfink/src/MI-upstream/js/src/jit/AsmJS.cpp:3399: Call(9,10, __temp_5 := Move(__temp_6*))
    home/sfink/src/MI-upstream/js/src/jit/AsmJS.cpp:3399: Call(10,11, __temp_3 := CheckFunctionSignature(__temp_4*,callNode*,__temp_5*,calleeName*,callee))

Because even though CheckCallArgs can in fact GC, the above use of calleeName is not actually reachable in this case.

The attached patch annotates failName as non-GC to suppress the hazards, but this is a fairly ugly lie. Brian, should the static analysis be smart enough to catch this or is this suppression a reasonable solution?
Attachment #793535 - Flags: review?(bhackett1024)
Another way to solve this would be to SuppressGC in failName. This would be similarly inelegant but at least would have the benefit of being robust against accidental use of a GC thing after a failure.
Comment on attachment 793535 [details] [diff] [review]
hazards_asmjs-v0.diff

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

Yeah, I would just put an AutoSuppressGC in failName.  I don't see the inelegance though, we already do this sort of stuff in functions like js_ReportAllocationOverflow and it's generally better to make the code conform to the analysis rather than special casing stuff with annotations.
Attachment #793535 - Flags: review?(bhackett1024)
Attached patch hazards_asmjs-v1.diff (obsolete) — Splinter Review
That makes sense.
Attachment #793535 - Attachment is obsolete: true
Attachment #793568 - Flags: review?(bhackett1024)
The AutoSuppressGC changes behavior to suit the analysis, possibly causing an OOM that wouldn't otherwise happen, but I suppose it's no big deal.

We sort of want something like:

  AutoGCMonitor monitor(rt);
  if (!gcOnFailure(...))
    return false;
  monitor.assertNoGCs();

where AutoGCMonitor would just record the GC number, and assertNoGCs would check that it hadn't changed. Then the analysis could use that as an annotation. But perhaps it's not worth the trouble, especially since you might have to sprinkle a lot of these around.
Comment on attachment 793568 [details] [diff] [review]
hazards_asmjs-v1.diff

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

::: js/src/jit/AsmJS.cpp
@@ +1277,5 @@
>  
>      bool failName(ParseNode *pn, const char *fmt, PropertyName *name) {
> +        // While we do not actually access any GC things after a failName,
> +        // allowing a GC here is error prone and confuses the exact rooting
> +        // static analysis.

You can drop the comment here or simplify it.  'This function is invoked without the caller properly rooting its locals.'
Attachment #793568 - Flags: review?(bhackett1024) → review+
(In reply to Steve Fink [:sfink] from comment #4)
> The AutoSuppressGC changes behavior to suit the analysis, possibly causing
> an OOM that wouldn't otherwise happen, but I suppose it's no big deal.

Well, the OOM report can only happen in a case that is already reporting, so it just changes the error message to something that eats less memory.
 
> We sort of want something like:
> 
>   AutoGCMonitor monitor(rt);
>   if (!gcOnFailure(...))
>     return false;
>   monitor.assertNoGCs();
> 
> where AutoGCMonitor would just record the GC number, and assertNoGCs would
> check that it hadn't changed. Then the analysis could use that as an
> annotation. But perhaps it's not worth the trouble, especially since you
> might have to sprinkle a lot of these around.

I guess that might be easier than adding dataflow to the analysis, but honestly, I'd rather make the analysis smarter than add yet more runtime assertions for static properties.
Terrence: the goal is for this code to run off the main thread when parsing happens off the main thread so can you not use name->runtimeFromMainThread()?

It looks like suppressGC is in PerThreadData so you should be able to change AutoSuppressGC to take a ThreadSafeContext* instead of a JSContext* and then pass cx_.
Thanks for catching that, Luke! I tracked ModuleCompiler up as far as Parser<FullParseHandler>::asmJS, where I had thought that FullParseHandler implied main thread. Will the attached work better?
Attachment #793568 - Attachment is obsolete: true
Attachment #794179 - Flags: review?(luke)
Comment on attachment 794179 [details] [diff] [review]
hazards_asmjs-v2.diff

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

::: js/src/jsgc.h
@@ +1399,5 @@
>  {
>      int32_t &suppressGC_;
>  
>    public:
> +    AutoSuppressGC(ThreadSafeContext *cx);

Oh hah, Brian just changed AutoSuppressGC to take an ExclusiveContext in the last few days.  ModuleCompiler::cx_ is an ExclusiveContext so no need to touch AutoSuppressGC anymore.
Attachment #794179 - Flags: review?(luke) → review+
Sure thing! Figured you wanted a ThreadSafeContext for the PJS work, but that can get added whenever if ExclusiveContext will work now.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2121b7b8179a
https://hg.mozilla.org/mozilla-central/rev/2121b7b8179a
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: