Closed
Bug 872191
Opened 11 years ago
Closed 11 years ago
Rooting hazard in ion/Lowering.cpp
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: sfink, Assigned: sfink)
Details
Attachments
(1 file)
1.90 KB,
patch
|
jonco
:
review+
sfink
:
checkin+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•11 years ago
|
||
target is barely used
Attachment #749439 -
Flags: review?(jcoppeard)
Updated•11 years ago
|
Attachment #749439 -
Flags: review?(jcoppeard) → review+
Comment 2•11 years ago
|
||
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
Comment 3•11 years ago
|
||
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?
Comment 5•11 years ago
|
||
Anyone who knew the answer to my question (probably nbp).
Assignee | ||
Updated•11 years ago
|
Attachment #749439 -
Flags: checkin+
Assignee | ||
Comment 6•11 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/d80a0d89f701
Comment 7•11 years ago
|
||
Sorry, didn't really mean to clear the need info there...
Flags: needinfo?(nicolas.b.pierron)
Comment 8•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d80a0d89f701
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment 9•11 years ago
|
||
(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)
Assignee | ||
Comment 10•11 years ago
|
||
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.
Description
•