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)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: terrence, Assigned: terrence)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
luke
:
review+
|
Details | Diff | 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)
Assignee | ||
Comment 1•11 years ago
|
||
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 2•11 years ago
|
||
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)
Assignee | ||
Comment 3•11 years ago
|
||
That makes sense.
Attachment #793535 -
Attachment is obsolete: true
Attachment #793568 -
Flags: review?(bhackett1024)
Comment 4•11 years ago
|
||
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 5•11 years ago
|
||
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+
Assignee | ||
Comment 6•11 years ago
|
||
(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.
Assignee | ||
Comment 7•11 years ago
|
||
Assignee | ||
Comment 8•11 years ago
|
||
Comment 9•11 years ago
|
||
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_.
Comment 10•11 years ago
|
||
Assignee | ||
Comment 11•11 years ago
|
||
D'oh, I relanded before I saw Luke's comment.
https://hg.mozilla.org/integration/mozilla-inbound/rev/09bdab645185
And backed out again in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/147eaec6c545
Assignee | ||
Comment 12•11 years ago
|
||
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 13•11 years ago
|
||
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+
Assignee | ||
Comment 14•11 years ago
|
||
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
Comment 15•11 years ago
|
||
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.
Description
•