Closed Bug 872191 Opened 11 years ago Closed 11 years ago

Rooting hazard in ion/Lowering.cpp

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: sfink, Assigned: sfink)

Details

Attachments

(1 file)

Function 'uint8 js::ion::LIRGenerator::visitApplyArgs(js::ion::MApplyArgs*)' has unrooted 'target' of type 'JSFunction*' live across GC call 'uint8 js::ion::LIRGeneratorX64::useBoxFixed(js::ion::LInstruction*, uint64, js::ion::MDefinition*, js::ion::Register, js::ion::Register)' at js/src/ion/Lowering.cpp:446
target is barely used
Attachment #749439 - Flags: review?(jcoppeard)
Attachment #749439 - Flags: review?(jcoppeard) → review+
The lowering phase is not supposed to cause any GC as well as anything which is used during a compilation, except IonBuilder.  I agree that the previous patch prevent a rooting hazard, but doing any GC here is an error in the first place as this code is executed in another thread.

CC: shu & nmatsakis, as the GC case involves ParallelArrayVisitor::visitCall.

uint8 js::ion::LIRGeneratorX64::useBoxFixed(js::ion::LInstruction*, uint64, js::ion::MDefinition*, js::ion::Register, js::ion::Register)
GC Function: uint8 js::ion::LIRGeneratorX64::useBoxFixed(js::ion::LInstruction*, uint64, js::ion::MDefinition*, js::ion::Register, js::ion::Register)
uint8 js::ion::LIRGeneratorShared::ensureDefined(js::ion::MDefinition*)
uint8 js::ion::MCall::accept(js::ion::MInstructionVisitor*)
uint8 js::ion::ParallelArrayVisitor::visitCall(js::ion::MCall*)
ParallelArrayAnalysis.cpp:uint8 js::ion::GetPossibleCallees(JSContext*, const class JS::Handle<JSScript*>, uint8*, js::types::StackTypeSet*, js::ion::MIRGraph*)
JSFunction* js::CloneFunctionAtCallsite(JSContext*, class JS::Handle<JSFunction*>, const class JS::Handle<JSScript*>, uint8*)
JSFunction* js::CloneFunctionObject(JSContext*, class JS::Handle<JSFunction*>, class JS::Handle<JSObject*>, uint32)
JSScript* JSFunction::getOrCreateScript(JSContext*)
uint8 JSFunction::initializeLazyScript(JSContext*)
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*>)
JSObject* js::CloneStaticBlockObject(JSContext*, class JS::Handle<JSObject*>, class JS::Handle<js::StaticBlockObject*>)
js::StaticBlockObject* js::StaticBlockObject::create(JSContext*)
js::Shape* js::EmptyShape::getInitialShape(JSContext*, js::Class*, js::TaggedProto, JSObject*, uint32, uint32)
js::Shape* js::EmptyShape::getInitialShape(JSContext*, js::Class*, js::TaggedProto, JSObject*, uint64, uint32)
js::UnownedBaseShape* js::BaseShape::getUnowned(JSContext*, js::StackBaseShape*)
js::BaseShape* js_NewGCBaseShape(JSContext*) [with js::AllowGC allowGC = (js::AllowGC)1u; JSContext = JSContext]
js::BaseShape* js::gc::NewGCThing(JSContext*, uint32, uint64, uint32) [with T = js::BaseShape; js::AllowGC allowGC = (js::AllowGC)1u; JSContext = JSContext; size_t = long unsigned int]
void* js::gc::ArenaLists::refillFreeList(JSContext*, uint32) [with js::AllowGC allowGC = (js::AllowGC)1u; JSContext = JSContext]
jsgc.cpp:void* RunLastDitchGC(JSContext*, JS::Zone*, uint32)
void js::GC(JSRuntime*, uint32, uint32)
GC
nbp: Thanks for pointing that out! Currently, off-main-thread-compilation is disabled whenever ParallelArrayVisitor would be used. However, in bug 862892, I am working on fixing that. It should be relatively straightforward to move the code in question to the final linking phase, however.

One thing that is confusing to me: is this a backtrace that you pasted in? I would not expect useBoxFixed to call into the parallel aray visitor.
Flags: needinfo?
Did you mean to needinfo anybody in particular, Niko?
Flags: needinfo?
Anyone who knew the answer to my question (probably nbp).
Attachment #749439 - Flags: checkin+
Sorry, didn't really mean to clear the need info there...
Flags: needinfo?(nicolas.b.pierron)
https://hg.mozilla.org/mozilla-central/rev/d80a0d89f701
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
(In reply to Niko Matsakis [:nmatsakis] from comment #3)
> One thing that is confusing to me: is this a backtrace that you pasted in? I
> would not expect useBoxFixed to call into the parallel aray visitor.

The backtrace is the answer that mrgiggles gave me in #jsapi.
Flags: needinfo?(nicolas.b.pierron)
The stack is just one arbitrarily-chosen path through the static callgraph. That particular one would never happen in practice, though there are probably plenty of others would.

Much of the compilation stuff is guarded by an AutoSuppressGC, which the analysis is aware of. I didn't look to see why this one doesn't fall into that category. I half expected that this patch is just covering up a false positive in the analysis, but the code is slightly better this way, and I have several other major sources of false positives to chase down, so I don't really care. :-)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: