diff --git a/devtools/shared/heapsnapshot/HeapSnapshot.cpp b/devtools/shared/heapsnapshot/HeapSnapshot.cpp index 206d2f95bac8..c0881b726463 100644 --- a/devtools/shared/heapsnapshot/HeapSnapshot.cpp +++ b/devtools/shared/heapsnapshot/HeapSnapshot.cpp @@ -695,13 +695,10 @@ static bool AddGlobalsAsRoots(HandleObjectVector globals, // If `boundaries` is incoherent, or we encounter an error while trying to // handle it, or we run out of memory, set `rv` appropriately and return // `false`. -// -// Return value is a pair of the status and an AutoCheckCannotGC token, -// forwarded from ubi::RootList::init(), to ensure that the caller does -// not GC while the RootList is live and initialized. -static std::pair EstablishBoundaries( - JSContext* cx, ErrorResult& rv, const HeapSnapshotBoundaries& boundaries, - ubi::RootList& roots, CompartmentSet& compartments) { +static bool EstablishBoundaries(JSContext* cx, ErrorResult& rv, + const HeapSnapshotBoundaries& boundaries, + ubi::RootList& roots, + CompartmentSet& compartments) { MOZ_ASSERT(!roots.initialized()); MOZ_ASSERT(compartments.empty()); @@ -712,49 +709,48 @@ static std::pair EstablishBoundaries( if (!boundaries.mRuntime.Value()) { rv.Throw(NS_ERROR_INVALID_ARG); - return {false, AutoCheckCannotGC(cx)}; + return false; } - auto [ok, nogc] = roots.init(); - if (!ok) { + if (!roots.init()) { rv.Throw(NS_ERROR_OUT_OF_MEMORY); - return {false, nogc}; + return false; } } if (boundaries.mDebugger.WasPassed()) { if (foundBoundaryProperty) { rv.Throw(NS_ERROR_INVALID_ARG); - return {false, AutoCheckCannotGC(cx)}; + return false; } foundBoundaryProperty = true; JSObject* dbgObj = boundaries.mDebugger.Value(); if (!dbgObj || !dbg::IsDebugger(*dbgObj)) { rv.Throw(NS_ERROR_INVALID_ARG); - return {false, AutoCheckCannotGC(cx)}; + return false; } RootedObjectVector globals(cx); if (!dbg::GetDebuggeeGlobals(cx, *dbgObj, &globals) || !PopulateCompartmentsWithGlobals(compartments, globals) || - !roots.init(compartments).first || !AddGlobalsAsRoots(globals, roots)) { + !roots.init(compartments) || !AddGlobalsAsRoots(globals, roots)) { rv.Throw(NS_ERROR_OUT_OF_MEMORY); - return {false, AutoCheckCannotGC(cx)}; + return false; } } if (boundaries.mGlobals.WasPassed()) { if (foundBoundaryProperty) { rv.Throw(NS_ERROR_INVALID_ARG); - return {false, AutoCheckCannotGC(cx)}; + return false; } foundBoundaryProperty = true; uint32_t length = boundaries.mGlobals.Value().Length(); if (length == 0) { rv.Throw(NS_ERROR_INVALID_ARG); - return {false, AutoCheckCannotGC(cx)}; + return false; } RootedObjectVector globals(cx); @@ -762,29 +758,28 @@ static std::pair EstablishBoundaries( JSObject* global = boundaries.mGlobals.Value().ElementAt(i); if (!JS_IsGlobalObject(global)) { rv.Throw(NS_ERROR_INVALID_ARG); - return {false, AutoCheckCannotGC(cx)}; + return false; } if (!globals.append(global)) { rv.Throw(NS_ERROR_OUT_OF_MEMORY); - return {false, AutoCheckCannotGC(cx)}; + return false; } } if (!PopulateCompartmentsWithGlobals(compartments, globals) || - !roots.init(compartments).first || !AddGlobalsAsRoots(globals, roots)) { + !roots.init(compartments) || !AddGlobalsAsRoots(globals, roots)) { rv.Throw(NS_ERROR_OUT_OF_MEMORY); - return {false, AutoCheckCannotGC(cx)}; + return false; } } - AutoCheckCannotGC nogc(cx); if (!foundBoundaryProperty) { rv.Throw(NS_ERROR_INVALID_ARG); - return {false, nogc}; + return false; } MOZ_ASSERT(roots.initialized()); - return {true, nogc}; + return true; } // A variant covering all the various two-byte strings that we can get from the @@ -1478,16 +1473,15 @@ void ChromeUtils::SaveHeapSnapshotShared( JSContext* cx = global.Context(); { - ubi::RootList rootList(cx, wantNames); - auto [ok, nogc] = - EstablishBoundaries(cx, rv, boundaries, rootList, compartments); - if (!ok) { + Maybe maybeNoGC; + ubi::RootList rootList(cx, maybeNoGC, wantNames); + if (!EstablishBoundaries(cx, rv, boundaries, rootList, compartments)) return; - } StreamWriter writer(cx, gzipStream, wantNames, !compartments.empty() ? &compartments : nullptr); + MOZ_ASSERT(maybeNoGC.isSome()); ubi::Node roots(&rootList); // Serialize the initial heap snapshot metadata to the core dump. @@ -1495,10 +1489,10 @@ void ChromeUtils::SaveHeapSnapshotShared( // Serialize the heap graph to the core dump, starting from our list of // roots. !WriteHeapGraph(cx, roots, writer, wantNames, - !compartments.empty() ? &compartments : nullptr, nogc, - nodeCount, edgeCount)) { + !compartments.empty() ? &compartments : nullptr, + maybeNoGC.ref(), nodeCount, edgeCount)) { rv.Throw(zeroCopyStream.failed() ? zeroCopyStream.result() - : NS_ERROR_UNEXPECTED); + : NS_ERROR_UNEXPECTED); return; } } diff --git a/js/examples/jorendb.js b/js/examples/jorendb.js index 7ab6393f1738..a920c5880d34 100644 --- a/js/examples/jorendb.js +++ b/js/examples/jorendb.js @@ -59,10 +59,7 @@ wrap(this, 'putstr'); // Convert a debuggee value v to a string. function dvToString(v) { - if (typeof(v) === 'object' && v !== null) { - return `[object ${v.class}]`; - } - return uneval(v); + return (typeof v !== 'object' || v === null) ? uneval(v) : "[object " + v.class + "]"; } function summaryObject(dv) { @@ -79,7 +76,7 @@ function summaryObject(dv) { function debuggeeValueToString(dv, style) { var dvrepr = dvToString(dv); - if (!style.pretty || (typeof dv !== 'object') || (dv === null)) + if (!style.pretty || (typeof dv !== 'object')) return [dvrepr, undefined]; const exec = debuggeeGlobalWrapper.executeInGlobalWithBindings.bind(debuggeeGlobalWrapper); @@ -877,7 +874,6 @@ while (rerun) { } catch (exc) { print("Caught exception " + exc); print(exc.stack); - break; } } else if (task.action == 'repl') { repl(); diff --git a/js/public/GCAPI.h b/js/public/GCAPI.h index 9126901d0429..8e5221fe77c6 100644 --- a/js/public/GCAPI.h +++ b/js/public/GCAPI.h @@ -989,7 +989,6 @@ class JS_PUBLIC_API AutoRequireNoGC { */ class JS_PUBLIC_API AutoAssertNoGC : public AutoRequireNoGC { #ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED - protected: JSContext* cx_; public: @@ -1064,19 +1063,13 @@ class JS_PUBLIC_API AutoAssertGCCallback : public AutoSuppressGCAnalysis { class JS_PUBLIC_API AutoCheckCannotGC : public AutoAssertNoGC { public: explicit AutoCheckCannotGC(JSContext* cx = nullptr) : AutoAssertNoGC(cx) {} -# ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED - AutoCheckCannotGC(const AutoCheckCannotGC& other) - : AutoCheckCannotGC(other.cx_) {} -# else - AutoCheckCannotGC(const AutoCheckCannotGC& other) : AutoCheckCannotGC() {} -# endif -#else -class JS_PUBLIC_API AutoCheckCannotGC : public AutoRequireNoGC{ - public : - explicit AutoCheckCannotGC(JSContext* cx = nullptr){} AutoCheckCannotGC( - const AutoCheckCannotGC& other) : AutoCheckCannotGC(){} -#endif } JS_HAZ_GC_INVALIDATED; +#else +class JS_PUBLIC_API AutoCheckCannotGC : public AutoRequireNoGC { + public: + explicit AutoCheckCannotGC(JSContext* cx = nullptr) {} +} JS_HAZ_GC_INVALIDATED; +#endif extern JS_PUBLIC_API void SetLowMemoryState(JSContext* cx, bool newState); diff --git a/js/public/UbiNode.h b/js/public/UbiNode.h index 51faeb526972..54f565d2a170 100644 --- a/js/public/UbiNode.h +++ b/js/public/UbiNode.h @@ -967,51 +967,50 @@ class PreComputedEdgeRange : public EdgeRange { // // RootList::init itself causes a minor collection, but once the list of roots // has been created, GC must not occur, as the referent ubi::Nodes are not -// stable across GC. It returns a [[nodiscard]] AutoCheckCannotGC token in order -// to enforce this. The token's lifetime must extend at least as long as the -// RootList itself. Note that the RootList does not itself contain a nogc field, -// which means that it is possible to store it somewhere that it can escape -// the init()'s nogc scope. Don't do that. (Or you could call some function -// and pass in the RootList and GC, but that would be caught.) +// stable across GC. The init calls emplace on |noGC|'s AutoCheckCannotGC, whose +// lifetime must extend at least as long as the RootList itself. // // Example usage: // // { -// JS::ubi::RootList rootList(cx); -// auto [ok, nogc] = rootList.init(); -// if (!ok()) { +// mozilla::Maybe maybeNoGC; +// JS::ubi::RootList rootList(cx, maybeNoGC); +// if (!rootList.init()) { // return false; // } // +// // The AutoCheckCannotGC is guaranteed to exist if init returned true. +// MOZ_ASSERT(maybeNoGC.isSome()); +// // JS::ubi::Node root(&rootList); // // ... // } class MOZ_STACK_CLASS JS_PUBLIC_API RootList { + Maybe& noGC; + public: JSContext* cx; EdgeVector edges; bool wantNames; - bool inited; - explicit RootList(JSContext* cx, bool wantNames = false); + RootList(JSContext* cx, Maybe& noGC, + bool wantNames = false); // Find all GC roots. - [[nodiscard]] std::pair init(); + [[nodiscard]] bool init(); // Find only GC roots in the provided set of |JS::Compartment|s. Note: it's // important to take a CompartmentSet and not a RealmSet: objects in // same-compartment realms can reference each other directly, without going // through CCWs, so if we used a RealmSet here we would miss edges. - [[nodiscard]] std::pair init( - CompartmentSet& debuggees); + [[nodiscard]] bool init(CompartmentSet& debuggees); // Find only GC roots in the given Debugger object's set of debuggee // compartments. - [[nodiscard]] std::pair init( - HandleObject debuggees); + [[nodiscard]] bool init(HandleObject debuggees); // Returns true if the RootList has been initialized successfully, false // otherwise. - bool initialized() { return inited; } + bool initialized() { return noGC.isSome(); } // Explicitly add the given Node as a root in this RootList. If wantNames is // true, you must pass an edgeName. The RootList does not take ownership of diff --git a/js/src/builtin/Object.cpp b/js/src/builtin/Object.cpp index bc951abc6ce6..41987e0d48ff 100644 --- a/js/src/builtin/Object.cpp +++ b/js/src/builtin/Object.cpp @@ -488,7 +488,7 @@ JSString* js::ObjectToSource(JSContext* cx, HandleObject obj) { val.set(desc->value()); - JSFunction* fun = nullptr; + JSFunction* fun; if (IsFunctionObject(val, &fun) && fun->isMethod()) { if (!AddProperty(id, val, PropertyKind::Method)) { return nullptr; diff --git a/js/src/builtin/TestingFunctions.cpp b/js/src/builtin/TestingFunctions.cpp index dc3ab82d2b8c..55aafacde381 100644 --- a/js/src/builtin/TestingFunctions.cpp +++ b/js/src/builtin/TestingFunctions.cpp @@ -5712,20 +5712,21 @@ static bool ShortestPaths(JSContext* cx, unsigned argc, Value* vp) { Vector>> names(cx); { + mozilla::Maybe maybeNoGC; JS::ubi::Node root; - JS::ubi::RootList rootList(cx, true); + JS::ubi::RootList rootList(cx, maybeNoGC, true); if (start.isNull()) { - auto [ok, nogc] = rootList.init(); - if (!ok) { + if (!rootList.init()) { ReportOutOfMemory(cx); return false; } root = JS::ubi::Node(&rootList); } else { + maybeNoGC.emplace(cx); root = JS::ubi::Node(start); } - JS::AutoCheckCannotGC noGC(cx); + JS::AutoCheckCannotGC& noGC = maybeNoGC.ref(); JS::ubi::NodeSet targets; diff --git a/js/src/debugger/Debugger.cpp b/js/src/debugger/Debugger.cpp index 2a83ca4c6db5..89524c7d62b0 100644 --- a/js/src/debugger/Debugger.cpp +++ b/js/src/debugger/Debugger.cpp @@ -5779,15 +5779,15 @@ class MOZ_STACK_CLASS Debugger::ObjectQuery { { // We can't tolerate the GC moving things around while we're // searching the heap. Check that nothing we do causes a GC. + Maybe maybeNoGC; RootedObject dbgObj(cx, dbg->object); - JS::ubi::RootList rootList(cx); - auto [ok, nogc] = rootList.init(dbgObj); - if (!ok) { + JS::ubi::RootList rootList(cx, maybeNoGC); + if (!rootList.init(dbgObj)) { ReportOutOfMemory(cx); return false; } - Traversal traversal(cx, *this, nogc); + Traversal traversal(cx, *this, maybeNoGC.ref()); traversal.wantNames = false; return traversal.addStart(JS::ubi::Node(&rootList)) && diff --git a/js/src/debugger/DebuggerMemory.cpp b/js/src/debugger/DebuggerMemory.cpp index 617331252782..f2c9d0e1168a 100644 --- a/js/src/debugger/DebuggerMemory.cpp +++ b/js/src/debugger/DebuggerMemory.cpp @@ -413,14 +413,14 @@ bool DebuggerMemory::CallData::takeCensus() { } { - JS::ubi::RootList rootList(cx); - auto [ok, nogc] = rootList.init(dbgObj); - if (!ok) { + Maybe maybeNoGC; + JS::ubi::RootList rootList(cx, maybeNoGC); + if (!rootList.init(dbgObj)) { ReportOutOfMemory(cx); return false; } - JS::ubi::CensusTraversal traversal(cx, handler, nogc); + JS::ubi::CensusTraversal traversal(cx, handler, maybeNoGC.ref()); traversal.wantNames = false; if (!traversal.addStart(JS::ubi::Node(&rootList)) || diff --git a/js/src/devtools/rootAnalysis/CFG.js b/js/src/devtools/rootAnalysis/CFG.js index f8e71fc81824..c1beafe08227 100644 --- a/js/src/devtools/rootAnalysis/CFG.js +++ b/js/src/devtools/rootAnalysis/CFG.js @@ -8,8 +8,6 @@ "use strict"; -var TRACING = false; - // Find all points (positions within the code) of the body given by the list of // bodies and the blockId to match (which will specify an outer function or a // loop within it), recursing into loops if needed. @@ -37,154 +35,6 @@ function findAllPoints(bodies, blockId, bits) return points; } -// Visitor of a graph of vertexes and sixgill-generated edges, -// where the edges represent the actual computation happening. -// -// Uses the syntax `var Visitor = class { ... }` rather than `class Visitor` -// to allow reloading this file with the JS debugger. -var Visitor = class { - constructor(bodies) { - this.visited_bodies = new Map(); - for (const body of bodies) { - this.visited_bodies.set(body, new Map()); - } - } - - // Returns whether we should keep going after seeing this - // pair. Also records it as visited. - visit(body, ppoint, info) { - const visited = this.visited_bodies.get(body); - const existing = visited.get(ppoint); - const action = this.next_action(existing, info); - const merged = this.merge_info(existing, info); - visited.set(ppoint, merged); - return [action, merged]; - } - - // Default implementation does a basic "only visit nodes once" search. - // (Whether this is BFS/DFS/other is determined by the caller.) - - // Override if you need to revisit nodes. Valid actions are "continue", - // "prune", and "done". "continue" means continue with the search. "prune" - // means stop at this node, only continue on other edges. "done" means the - // whole search is complete even if unvisited nodes remain. - next_action(prev, current) { return prev ? "prune" : "continue"; } - - // Update the info at a node. If this is the first time the node has been - // seen, `prev` will be undefined. `current` will be the info computed by - // `extend_path`. The node will be updated with the return value. - merge_info(prev, current) { return true; } - - // Prepend `edge` to the info stored at the successor node, returning - // the updated info value. This should be overridden by pretty much any - // subclass, as a traversal's semantics are largely determined by this method. - extend_path(edge, body, ppoint, successor_path) { return true; } -}; - -function findMatchingBlock(bodies, blockId) { - for (const body of bodies) { - if (sameBlockId(body.BlockId, blockId)) { - return body; - } - } - assert(false); -} - -// Perform a mostly breadth-first search through the graph of . -// This is only mostly breadth-first because the visitor decides whether to -// stop searching when it sees an already-visited node. It can choose to -// re-visit a node in order to find "better" paths that include a node more -// than once. -// -// The return value depends on how the search finishes. If a 'done' action -// is returned by visitor.visit(), use the information returned by -// that call. If the search completes without reaching the entry point of -// the function (the "root"), return null. If the search manages to reach -// the root, return the value of the `result_if_reached_root` parameter. -// -// This allows this function to be used in different ways. If the visitor -// associates a value with each node that chains onto its successors -// (or predecessors in the "upwards" search order), then this will return -// a complete path through the graph. But this can also be used to test -// whether a condition holds (eg "the exit point is reachable after -// calling SomethingImportant()"), in which case no path is needed and the -// visitor will cause the return value to be a simple boolean (or null -// if it terminates the search before reaching the root.) -// -// The information returned by the visitor for a node is often called -// `path` in the code below, even though it may not represent a path. -// -function BFS_upwards(start_body, start_ppoint, bodies, visitor, - initial_successor_info={}, - result_if_reached_root=null) -{ - const work = [[start_body, start_ppoint, null, initial_successor_info]]; - if (TRACING) { - printErr(`BFS start at ${blockIdentifier(start_body)}:${start_ppoint}`); - } - - let reached_root = false; - while (work.length > 0) { - const [body, ppoint, edgeToAdd, successor_path] = work.shift(); - if (TRACING) { - printErr(`prepending edge from ${ppoint} to state '${successor_path}'`); - } - let path = visitor.extend_path(edgeToAdd, body, ppoint, successor_path); - - const [action, merged_path] = visitor.visit(body, ppoint, path); - if (action === "done") { - return merged_path; - } - if (action === "prune") { - // Do not push anything else to the work queue, but continue processing - // other branches. - continue; - } - assert(action == "continue"); - path = merged_path; - - const predecessors = getPredecessors(body); - for (const edge of (predecessors[ppoint] || [])) { - if (edge.Kind == "Loop") { - // Propagate the search into the exit point of the loop body. - const loopBody = findMatchingBlock(bodies, edge.BlockId); - const loopEnd = loopBody.Index[1]; - work.push([loopBody, loopEnd, null, path]); - // Don't continue to predecessors here without going through - // the loop. (The points in this body that enter the loop will - // be traversed when we reach the entry point of the loop.) - } else { - work.push([body, edge.Index[0], edge, path]); - } - } - - // Check for hitting the entry point of a loop body. - if (ppoint == body.Index[0] && body.BlockId.Kind == "Loop") { - // Propagate to outer body parents that enter the loop body. - for (const parent of (body.BlockPPoint || [])) { - const parentBody = findMatchingBlock(bodies, parent.BlockId); - work.push([parentBody, parent.Index, null, path]); - } - - // This point is also preceded by the *end* of this loop, for the - // previous iteration. - work.push([body, body.Index[1], null, path]); - } - - // Check for reaching the root of the function. - if (body === start_body && ppoint == body.Index[0]) { - reached_root = true; - } - } - - // The search space was exhausted without finding a 'done' state. That - // might be because all search paths were pruned before reaching the entry - // point of the function, in which case reached_root will be false. (If - // reached_root is true, then we may still not have visited the entire - // graph, if some paths were pruned but at least one made it to the root.) - return reached_root ? result_if_reached_root : null; -} - // Given the CFG for the constructor call of some RAII, return whether the // given edge is the matching destructor call. function isMatchingDestructor(constructor, edge) @@ -328,42 +178,6 @@ function pointsInRAIIScope(bodies, body, constructorEdge, bits) { return points; } -function isImmobileValue(exp) { - if (exp.Kind == "Int" && exp.String == "0") { - return true; - } - return false; -} - -function expressionIsVariableAddress(exp, variable) -{ - while (exp.Kind == "Fld") - exp = exp.Exp[0]; - return exp.Kind == "Var" && sameVariable(exp.Variable, variable); -} - -function edgeTakesVariableAddress(edge, variable, body) -{ - if (ignoreEdgeUse(edge, variable, body)) - return false; - if (ignoreEdgeAddressTaken(edge)) - return false; - switch (edge.Kind) { - case "Assign": - return expressionIsVariableAddress(edge.Exp[1], variable); - case "Call": - if ("PEdgeCallArguments" in edge) { - for (var exp of edge.PEdgeCallArguments.Exp) { - if (expressionIsVariableAddress(exp, variable)) - return true; - } - } - return false; - default: - return false; - } -} - // Look at an invocation of a virtual method or function pointer contained in a // field, and return the static type of the invocant (or the containing struct, // for a function pointer field.) @@ -402,495 +216,6 @@ function getFieldCallInstanceCSU(edge, field) } } -function expressionUsesVariable(exp, variable) -{ - if (exp.Kind == "Var" && sameVariable(exp.Variable, variable)) - return true; - if (!("Exp" in exp)) - return false; - for (var childExp of exp.Exp) { - if (expressionUsesVariable(childExp, variable)) - return true; - } - return false; -} - -function expressionUsesVariableContents(exp, variable) -{ - if (!("Exp" in exp)) - return false; - for (var childExp of exp.Exp) { - if (childExp.Kind == 'Drf') { - if (expressionUsesVariable(childExp, variable)) - return true; - } else if (expressionUsesVariableContents(childExp, variable)) { - return true; - } - } - return false; -} - -// Detect simple |return nullptr;| statements. -function isReturningImmobileValue(edge, variable) -{ - if (variable.Kind == "Return") { - if (edge.Exp[0].Kind == "Var" && sameVariable(edge.Exp[0].Variable, variable)) { - if (isImmobileValue(edge.Exp[1])) - return true; - } - } - return false; -} - -// If the edge uses the given variable's value, return the earliest point at -// which the use is definite. Usually, that means the source of the edge -// (anything that reaches that source point will end up using the variable, but -// there may be other ways to reach the destination of the edge.) -// -// Return values are implicitly used at the very last point in the function. -// This makes a difference: if an RAII class GCs in its destructor, we need to -// start looking at the final point in the function, not one point back from -// that, since that would skip over the GCing call. -// -// Note that this returns true only if the variable's incoming value is used. -// So this would return false for 'obj': -// -// obj = someFunction(); -// -// but these would return true: -// -// obj = someFunction(obj); -// obj->foo = someFunction(); -// -function edgeUsesVariable(edge, variable, body) -{ - if (ignoreEdgeUse(edge, variable, body)) - return 0; - - if (variable.Kind == "Return" && body.Index[1] == edge.Index[1] && body.BlockId.Kind == "Function") { - // The last point in the function body is treated as using the return - // value. This is the only time the destination point is returned - // rather than the source point. - return edge.Index[1]; - } - - var src = edge.Index[0]; - - switch (edge.Kind) { - - case "Assign": { - // Detect `Return := nullptr`. - if (isReturningImmobileValue(edge, variable)) - return 0; - const [lhs, rhs] = edge.Exp; - // Detect `lhs := ...variable...` - if (expressionUsesVariable(rhs, variable)) - return src; - // Detect `...variable... := rhs` but not `variable := rhs`. The latter - // overwrites the previous value of `variable` without using it. - if (expressionUsesVariable(lhs, variable) && !expressionIsVariable(lhs, variable)) - return src; - return 0; - } - - case "Assume": - return expressionUsesVariableContents(edge.Exp[0], variable) ? src : 0; - - case "Call": { - const callee = edge.Exp[0]; - if (expressionUsesVariable(callee, variable)) - return src; - if ("PEdgeCallInstance" in edge) { - if (expressionUsesVariable(edge.PEdgeCallInstance.Exp, variable)) { - if (edgeStartsValueLiveRange(edge, variable)) { - // If the variable is being constructed, then the incoming - // value is not used here; it didn't exist before - // construction. (The analysis doesn't get told where - // variables are defined, so must infer it from - // construction. If the variable does not have a - // constructor, its live range may be larger than it really - // ought to be if it is defined within a loop body, but - // that is conservative.) - } else { - return src; - } - } - } - if ("PEdgeCallArguments" in edge) { - for (var exp of edge.PEdgeCallArguments.Exp) { - if (expressionUsesVariable(exp, variable)) - return src; - } - } - if (edge.Exp.length == 1) - return 0; - - // Assigning call result to a variable. - const lhs = edge.Exp[1]; - if (expressionUsesVariable(lhs, variable) && !expressionIsVariable(lhs, variable)) - return src; - return 0; - } - - case "Loop": - return 0; - - case "Assembly": - return 0; - - default: - assert(false); - } -} - -function expressionIsVariable(exp, variable) -{ - return exp.Kind == "Var" && sameVariable(exp.Variable, variable); -} - -function expressionIsMethodOnVariable(exp, variable) -{ - // This might be calling a method on a base class, in which case exp will - // be an unnamed field of the variable instead of the variable itself. - while (exp.Kind == "Fld" && exp.Field.Name[0].startsWith("field:")) - exp = exp.Exp[0]; - - return exp.Kind == "Var" && sameVariable(exp.Variable, variable); -} - -// Return whether the edge starts the live range of a variable's value, by setting -// it to some new value. Examples of starting obj's live range: -// -// obj = foo; -// obj = foo(); -// obj = foo(obj); // uses previous value but then sets to new value -// SomeClass obj(true, 1); // constructor -// -function edgeStartsValueLiveRange(edge, variable) -{ - // Direct assignments start live range of lhs: var = value - if (edge.Kind == "Assign") { - const [lhs, rhs] = edge.Exp; - return (expressionIsVariable(lhs, variable) && - !isReturningImmobileValue(edge, variable)); - } - - if (edge.Kind != "Call") - return false; - - // Assignments of call results start live range: var = foo() - if (1 in edge.Exp) { - var lhs = edge.Exp[1]; - if (expressionIsVariable(lhs, variable)) - return true; - } - - // Constructor calls start live range of instance: SomeClass var(...) - if ("PEdgeCallInstance" in edge) { - var instance = edge.PEdgeCallInstance.Exp; - - // Kludge around incorrect dereference on some constructor calls. - if (instance.Kind == "Drf") - instance = instance.Exp[0]; - - if (!expressionIsVariable(instance, variable)) - return false; - - var callee = edge.Exp[0]; - if (callee.Kind != "Var") - return false; - - assert(callee.Variable.Kind == "Func"); - var calleeName = readable(callee.Variable.Name[0]); - - // Constructor calls include the text 'Name::Name(' or 'Name<...>::Name('. - var openParen = calleeName.indexOf('('); - if (openParen < 0) - return false; - calleeName = calleeName.substring(0, openParen); - - var lastColon = calleeName.lastIndexOf('::'); - if (lastColon < 0) - return false; - var constructorName = calleeName.substr(lastColon + 2); - calleeName = calleeName.substr(0, lastColon); - - var lastTemplateOpen = calleeName.lastIndexOf('<'); - if (lastTemplateOpen >= 0) - calleeName = calleeName.substr(0, lastTemplateOpen); - - if (calleeName.endsWith(constructorName)) - return true; - } - - return false; -} - -// Return whether an edge "clears out" a variable's value. A simple example -// would be -// -// var = nullptr; -// -// for analyses for which nullptr is a "safe" value (eg GC rooting hazards; you -// can't get in trouble by holding a nullptr live across a GC.) A more complex -// example is a Maybe that gets reset: -// -// Maybe nogc; -// nogc.emplace(cx); -// nogc.reset(); -// gc(); // <-- not a problem; nogc is invalidated by prev line -// nogc.emplace(cx); -// foo(nogc); -// -// Yet another example is a UniquePtr being passed by value, which means the -// receiver takes ownership: -// -// UniquePtr uobj(obj); -// foo(uobj); -// gc(); -// -function edgeEndsValueLiveRange(edge, variable, body) -{ - // var = nullptr; - if (edge.Kind == "Assign") { - const [lhs, rhs] = edge.Exp; - return expressionIsVariable(lhs, variable) && isImmobileValue(rhs); - } - - if (edge.Kind != "Call") - return false; - - var callee = edge.Exp[0]; - - if (edge.Type.Kind == 'Function' && - edge.Exp[0].Kind == 'Var' && - edge.Exp[0].Variable.Kind == 'Func' && - edge.Exp[0].Variable.Name[1] == 'MarkVariableAsGCSafe' && - edge.Exp[0].Variable.Name[0].includes("JS::detail::MarkVariableAsGCSafe") && - expressionIsVariable(edge.PEdgeCallArguments.Exp[0], variable)) - { - // explicit JS_HAZ_VARIABLE_IS_GC_SAFE annotation - return true; - } - - if (edge.Type.Kind == 'Function' && - edge.Exp[0].Kind == 'Var' && - edge.Exp[0].Variable.Kind == 'Func' && - edge.Exp[0].Variable.Name[1] == 'move' && - edge.Exp[0].Variable.Name[0].includes('std::move(') && - expressionIsVariable(edge.PEdgeCallArguments.Exp[0], variable) && - edge.Exp[1].Kind == 'Var' && - edge.Exp[1].Variable.Kind == 'Temp') - { - // temp = std::move(var) - // - // If var is a UniquePtr, and we pass it into something that takes - // ownership, then it should be considered to be invalid. Example: - // - // consume(std::move(var)); - // - // where consume takes a UniquePtr. This will compile to something like - // - // UniquePtr* __temp_1 = &std::move(var); - // UniquePtr&& __temp_2(*temp_1); // move constructor - // consume(__temp_2); - // ~UniquePtr(__temp_2); - // - // The line commented with "// move constructor" is a result of passing - // a UniquePtr as a parameter. If consume() took a UniquePtr&& - // directly, this would just be: - // - // UniquePtr* __temp_1 = &std::move(var); - // consume(__temp_1); - // - // which is not guaranteed to move from the reference. It might just - // ignore the parameter. We can't predict what consume(UniquePtr&&) - // will do. We do know that UniquePtr(UniquePtr&& other) moves out of - // `other`. - // - // The std::move() technically is irrelevant, but because we only care - // about bare variables, it has to be used, which is fortunate because - // the UniquePtr&& constructor operates on a temporary, not the - // variable we care about. - - const lhs = edge.Exp[1].Variable; - if (basicBlockEatsVariable(lhs, body, edge.Index[1])) - return true; - } - - if (edge.Type.Kind == 'Function' && - edge.Type.TypeFunctionCSU && - edge.PEdgeCallInstance && - expressionIsMethodOnVariable(edge.PEdgeCallInstance.Exp, variable)) - { - const typeName = edge.Type.TypeFunctionCSU.Type.Name; - const m = typeName.match(/^(((\w|::)+?)(\w+)). - const ctorName = `${namespace}${classname}::${classname}()`; - if (callee.Kind == 'Var' && - typesWithSafeConstructors.has(type) && - callee.Variable.Name[0].includes(ctorName)) - { - return true; - } - - // special-case: UniquePtr::reset() and similar. - if (callee.Kind == 'Var' && - type in resetterMethods && - resetterMethods[type].has(callee.Variable.Name[1])) - { - return true; - } - } - } - - // special-case: passing UniquePtr by value. - if (edge.Type.Kind == 'Function' && - edge.Type.TypeFunctionArgument && - edge.PEdgeCallArguments) - { - for (const i in edge.Type.TypeFunctionArgument) { - const param = edge.Type.TypeFunctionArgument[i]; - if (param.Type.Kind != 'CSU') - continue; - if (!param.Type.Name.startsWith("mozilla::UniquePtr<")) - continue; - const arg = edge.PEdgeCallArguments.Exp[i]; - if (expressionIsVariable(arg, variable)) { - return true; - } - } - } - - return false; -} - -function edgeMovesVariable(edge, variable) -{ - if (edge.Kind != 'Call') - return false; - const callee = edge.Exp[0]; - if (callee.Kind == 'Var' && - callee.Variable.Kind == 'Func') - { - const { Variable: { Name: [ fullname, shortname ] } } = callee; - const [ mangled, unmangled ] = splitFunction(fullname); - // Match a UniquePtr move constructor. - if (unmangled.match(/::UniquePtr<[^>]*>::UniquePtr\((\w+::)*UniquePtr<[^>]*>&&/)) - return true; - } - - return false; -} - -// Scan forward through the basic block in 'body' starting at 'startpoint', -// looking for a call that passes 'variable' to a move constructor that -// "consumes" it (eg UniquePtr::UniquePtr(UniquePtr&&)). -function basicBlockEatsVariable(variable, body, startpoint) -{ - const successors = getSuccessors(body); - let point = startpoint; - while (point in successors) { - // Only handle a single basic block. If it forks, stop looking. - const edges = successors[point]; - if (edges.length != 1) { - return false; - } - const edge = edges[0]; - - if (edgeMovesVariable(edge, variable)) { - return true; - } - - // edgeStartsValueLiveRange will find places where 'variable' is given - // a new value. Never observed in practice, since this function is only - // called with a temporary resulting from std::move(), which is used - // immediately for a call. But just to be robust to future uses: - if (edgeStartsValueLiveRange(edge, variable)) { - return false; - } - - point = edge.Index[1]; - } - - return false; -} - -function edgeIsNonReleasingDtor(body, edge, calleeName, functionBodies) { - if (edge.Kind !== "Call") { - return false; - } - if (!isRefcountedDtor(calleeName)) { - return false; - } - - let callee = edge.Exp[0]; - while (callee.Kind === "Drf") { - callee = callee.Exp[0]; - } - - const instance = edge.PEdgeCallInstance.Exp; - if (instance.Kind !== "Var") { - // TODO: handle field destructors - return false; - } - - // Test whether the dtor call is dominated by operations on the variable - // that mean it will not go to a zero refcount in the dtor: either because - // it's already dead (eg r.forget() was called) or because it can be proven - // to have a ref count of greater than 1. This is implemented by looking - // for the reverse: find a path scanning backwards from the dtor call where - // the variable is used in any way that does *not* ensure that it is - // trivially destructible. - - const variable = instance.Variable; - - const visitor = new class extends Visitor { - // Do not revisit nodes. For new nodes, relay the decision made by - // extend_path. - next_action(seen, current) { return seen ? "prune" : current; } - - // We don't revisit, so always use the new. - merge_info(seen, current) { return current; } - - // Return the action to take from this node. - extend_path(edge, body, ppoint, successor_path) { - if (!edge) { - // Dummy edge to join two points. - return "continue"; - } - - if (!edgeUsesVariable(edge, variable, body)) { - // Nothing of interest on this edge, keep searching. - return "continue"; - } - - if (edgeEndsValueLiveRange(edge, variable, body)) { - // This path is safe! - return "prune"; - } - - // Unsafe. Found a use that might set the variable to a - // nonzero refcount. - return "done"; - } - }(functionBodies); - - // Searching upwards from a destructor call, return the opposite of: is - // there a path to a use or the start of the function that does NOT hit a - // safe assignment like refptr.forget() first? - // - // In graph terms: return whether the destructor call is dominated by forget() calls (or similar). - return !BFS_upwards( - body, edge.Index[0], functionBodies, visitor, "start", - true // Return value if we reach the root without finding a non-forget() use. - ); -} - // gcc uses something like "__dt_del " for virtual destructors that it // generates. function isSyntheticVirtualDestructor(funcName) { diff --git a/js/src/devtools/rootAnalysis/analyze.py b/js/src/devtools/rootAnalysis/analyze.py index 896ab022a63c..30e727e42c71 100755 --- a/js/src/devtools/rootAnalysis/analyze.py +++ b/js/src/devtools/rootAnalysis/analyze.py @@ -120,21 +120,11 @@ JOBS = { "{analysis_scriptdir}/computeCallgraph.js", "{typeInfo}", Output("rawcalls"), - Output("rawEdges"), "{i}", "{n}", ], "multi-output": True, - "outputs": ["rawcalls.{i}.of.{n}", "gcEdges.{i}.of.{n}"], - }, - "mergeJSON": { - "command": [ - "{js}", - "{analysis_scriptdir}/mergeJSON.js", - MultiInput("{rawEdges}"), - Output("gcEdges"), - ], - "outputs": ["gcEdges.json"], + "outputs": ["rawcalls.{i}.of.{n}"], }, "gcFunctions": { "command": [ @@ -145,12 +135,14 @@ JOBS = { Output("callgraph"), Output("gcFunctions"), Output("gcFunctions_list"), + Output("gcEdges"), Output("limitedFunctions_list"), ], "outputs": [ "callgraph.txt", "gcFunctions.txt", "gcFunctions.lst", + "gcEdges.txt", "limitedFunctions.lst", ], }, @@ -440,7 +432,6 @@ steps = [ "gcTypes", "rawcalls", "gcFunctions", - "mergeJSON", "allFunctions", "hazards", "gather-hazards", diff --git a/js/src/devtools/rootAnalysis/analyzeRoots.js b/js/src/devtools/rootAnalysis/analyzeRoots.js index b66a66cec353..8565f1b84384 100644 --- a/js/src/devtools/rootAnalysis/analyzeRoots.js +++ b/js/src/devtools/rootAnalysis/analyzeRoots.js @@ -16,75 +16,51 @@ var sourceRoot = (os.getenv('SOURCE') || '') + '/'; var functionName; var functionBodies; -try { - var options = parse_options([ - { - name: "--function", - type: 'string', - }, - { - name: "-f", - type: "string", - dest: "function", - }, - { - name: "gcFunctions", - default: "gcFunctions.lst" - }, - { - name: "gcEdges", - default: "gcEdges.json" - }, - { - name: "limitedFunctions", - default: "limitedFunctions.lst" - }, - { - name: "gcTypes", - default: "gcTypes.txt" - }, - { - name: "typeInfo", - default: "typeInfo.txt" - }, - { - name: "batch", - type: "number", - default: 1 - }, - { - name: "numBatches", - type: "number", - default: 1 - }, - { - name: "tmpfile", - default: "tmp.txt" - }, - ]); -} catch (e) { - printErr(e); - printErr("Usage: analyzeRoots.js [-f function_name] [start end [tmpfile]]"); - quit(1); +if (typeof scriptArgs[0] != 'string' || typeof scriptArgs[1] != 'string') + throw "Usage: analyzeRoots.js [-f function_name] [start end [tmpfile]]"; + +var theFunctionNameToFind; +if (scriptArgs[0] == '--function' || scriptArgs[0] == '-f') { + theFunctionNameToFind = scriptArgs[1]; + scriptArgs = scriptArgs.slice(2); } + +var gcFunctionsFile = scriptArgs[0] || "gcFunctions.lst"; +var gcEdgesFile = scriptArgs[1] || "gcEdges.txt"; +var limitedFunctionsFile = scriptArgs[2] || "limitedFunctions.lst"; +var gcTypesFile = scriptArgs[3] || "gcTypes.txt"; +var typeInfoFile = scriptArgs[4] || "typeInfo.txt"; +var batch = (scriptArgs[5]|0) || 1; +var numBatches = (scriptArgs[6]|0) || 1; +var tmpfile = scriptArgs[7] || "tmp.txt"; + var gcFunctions = {}; -var text = snarf(options.gcFunctions).split("\n"); +var text = snarf("gcFunctions.lst").split("\n"); assert(text.pop().length == 0); for (const line of text) gcFunctions[mangled(line)] = readable(line); -var limitedFunctions = JSON.parse(snarf(options.limitedFunctions)); +var limitedFunctions = JSON.parse(snarf(limitedFunctionsFile)); text = null; -var typeInfo = loadTypeInfo(options.typeInfo); +var typeInfo = loadTypeInfo(typeInfoFile); -var gcEdges = JSON.parse(os.file.readFile(options.gcEdges)); +var gcEdges = {}; +text = snarf(gcEdgesFile).split('\n'); +assert(text.pop().length == 0); +for (const line of text) { + var [ block, edge, func ] = line.split(" || "); + if (!(block in gcEdges)) + gcEdges[block] = {} + gcEdges[block][edge] = func; +} +text = null; var match; var gcThings = {}; var gcPointers = {}; -text = snarf(options.gcTypes).split("\n"); +text = snarf(gcTypesFile).split("\n"); for (var line of text) { if (match = /^GCThing: (.*)/.exec(line)) gcThings[match[1]] = true; @@ -119,6 +95,425 @@ function isUnrootedType(type) return false; } +function expressionUsesVariable(exp, variable) +{ + if (exp.Kind == "Var" && sameVariable(exp.Variable, variable)) + return true; + if (!("Exp" in exp)) + return false; + for (var childExp of exp.Exp) { + if (expressionUsesVariable(childExp, variable)) + return true; + } + return false; +} + +function expressionUsesVariableContents(exp, variable) +{ + if (!("Exp" in exp)) + return false; + for (var childExp of exp.Exp) { + if (childExp.Kind == 'Drf') { + if (expressionUsesVariable(childExp, variable)) + return true; + } else if (expressionUsesVariableContents(childExp, variable)) { + return true; + } + } + return false; +} + +function isImmobileValue(exp) { + if (exp.Kind == "Int" && exp.String == "0") { + return true; + } + return false; +} + +// Detect simple |return nullptr;| statements. +function isReturningImmobileValue(edge, variable) +{ + if (variable.Kind == "Return") { + if (edge.Exp[0].Kind == "Var" && sameVariable(edge.Exp[0].Variable, variable)) { + if (isImmobileValue(edge.Exp[1])) + return true; + } + } + return false; +} + +// If the edge uses the given variable's value, return the earliest point at +// which the use is definite. Usually, that means the source of the edge +// (anything that reaches that source point will end up using the variable, but +// there may be other ways to reach the destination of the edge.) +// +// Return values are implicitly used at the very last point in the function. +// This makes a difference: if an RAII class GCs in its destructor, we need to +// start looking at the final point in the function, not one point back from +// that, since that would skip over the GCing call. +// +// Note that this returns true only if the variable's incoming value is used. +// So this would return false for 'obj': +// +// obj = someFunction(); +// +// but these would return true: +// +// obj = someFunction(obj); +// obj->foo = someFunction(); +// +function edgeUsesVariable(edge, variable, body) +{ + if (ignoreEdgeUse(edge, variable, body)) + return 0; + + if (variable.Kind == "Return" && body.Index[1] == edge.Index[1] && body.BlockId.Kind == "Function") + return edge.Index[1]; // Last point in function body uses the return value. + + var src = edge.Index[0]; + + switch (edge.Kind) { + + case "Assign": { + // Detect `Return := nullptr`. + if (isReturningImmobileValue(edge, variable)) + return 0; + const [lhs, rhs] = edge.Exp; + // Detect `lhs := ...variable...` + if (expressionUsesVariable(rhs, variable)) + return src; + // Detect `...variable... := rhs` but not `variable := rhs`. The latter + // overwrites the previous value of `variable` without using it. + if (expressionUsesVariable(lhs, variable) && !expressionIsVariable(lhs, variable)) + return src; + return 0; + } + + case "Assume": + return expressionUsesVariableContents(edge.Exp[0], variable) ? src : 0; + + case "Call": { + const callee = edge.Exp[0]; + if (expressionUsesVariable(callee, variable)) + return src; + if ("PEdgeCallInstance" in edge) { + if (expressionUsesVariable(edge.PEdgeCallInstance.Exp, variable)) { + if (edgeStartsValueLiveRange(edge, variable)) { + // If the variable is being constructed, then the incoming + // value is not used here; it didn't exist before + // construction. (The analysis doesn't get told where + // variables are defined, so must infer it from + // construction. If the variable does not have a + // constructor, its live range may be larger than it really + // ought to be if it is defined within a loop body, but + // that is conservative.) + } else { + return src; + } + } + } + if ("PEdgeCallArguments" in edge) { + for (var exp of edge.PEdgeCallArguments.Exp) { + if (expressionUsesVariable(exp, variable)) + return src; + } + } + if (edge.Exp.length == 1) + return 0; + + // Assigning call result to a variable. + const lhs = edge.Exp[1]; + if (expressionUsesVariable(lhs, variable) && !expressionIsVariable(lhs, variable)) + return src; + return 0; + } + + case "Loop": + return 0; + + case "Assembly": + return 0; + + default: + assert(false); + } +} + +function expressionIsVariableAddress(exp, variable) +{ + while (exp.Kind == "Fld") + exp = exp.Exp[0]; + return exp.Kind == "Var" && sameVariable(exp.Variable, variable); +} + +function edgeTakesVariableAddress(edge, variable, body) +{ + if (ignoreEdgeUse(edge, variable, body)) + return false; + if (ignoreEdgeAddressTaken(edge)) + return false; + switch (edge.Kind) { + case "Assign": + return expressionIsVariableAddress(edge.Exp[1], variable); + case "Call": + if ("PEdgeCallArguments" in edge) { + for (var exp of edge.PEdgeCallArguments.Exp) { + if (expressionIsVariableAddress(exp, variable)) + return true; + } + } + return false; + default: + return false; + } +} + +function expressionIsVariable(exp, variable) +{ + return exp.Kind == "Var" && sameVariable(exp.Variable, variable); +} + +function expressionIsMethodOnVariable(exp, variable) +{ + // This might be calling a method on a base class, in which case exp will + // be an unnamed field of the variable instead of the variable itself. + while (exp.Kind == "Fld" && exp.Field.Name[0].startsWith("field:")) + exp = exp.Exp[0]; + + return exp.Kind == "Var" && sameVariable(exp.Variable, variable); +} + +// Return whether the edge starts the live range of a variable's value, by setting +// it to some new value. Examples of starting obj's live range: +// +// obj = foo; +// obj = foo(); +// obj = foo(obj); // uses previous value but then sets to new value +// SomeClass obj(true, 1); // constructor +// +function edgeStartsValueLiveRange(edge, variable) +{ + // Direct assignments start live range of lhs: var = value + if (edge.Kind == "Assign") { + const [lhs, rhs] = edge.Exp; + return (expressionIsVariable(lhs, variable) && + !isReturningImmobileValue(edge, variable)); + } + + if (edge.Kind != "Call") + return false; + + // Assignments of call results start live range: var = foo() + if (1 in edge.Exp) { + var lhs = edge.Exp[1]; + if (expressionIsVariable(lhs, variable)) + return true; + } + + // Constructor calls start live range of instance: SomeClass var(...) + if ("PEdgeCallInstance" in edge) { + var instance = edge.PEdgeCallInstance.Exp; + + // Kludge around incorrect dereference on some constructor calls. + if (instance.Kind == "Drf") + instance = instance.Exp[0]; + + if (!expressionIsVariable(instance, variable)) + return false; + + var callee = edge.Exp[0]; + if (callee.Kind != "Var") + return false; + + assert(callee.Variable.Kind == "Func"); + var calleeName = readable(callee.Variable.Name[0]); + + // Constructor calls include the text 'Name::Name(' or 'Name<...>::Name('. + var openParen = calleeName.indexOf('('); + if (openParen < 0) + return false; + calleeName = calleeName.substring(0, openParen); + + var lastColon = calleeName.lastIndexOf('::'); + if (lastColon < 0) + return false; + var constructorName = calleeName.substr(lastColon + 2); + calleeName = calleeName.substr(0, lastColon); + + var lastTemplateOpen = calleeName.lastIndexOf('<'); + if (lastTemplateOpen >= 0) + calleeName = calleeName.substr(0, lastTemplateOpen); + + if (calleeName.endsWith(constructorName)) + return true; + } + + return false; +} + +function edgeMovesVariable(edge, variable) +{ + if (edge.Kind != 'Call') + return false; + const callee = edge.Exp[0]; + if (callee.Kind == 'Var' && + callee.Variable.Kind == 'Func') + { + const { Variable: { Name: [ fullname, shortname ] } } = callee; + const [ mangled, unmangled ] = splitFunction(fullname); + // Match a UniquePtr move constructor. + if (unmangled.match(/::UniquePtr<[^>]*>::UniquePtr\((\w+::)*UniquePtr<[^>]*>&&/)) + return true; + } + + return false; +} + +// Scan forward through the given 'body', starting at 'startpoint', looking for +// a call that passes 'variable' to a move constructor that "consumes" it (eg +// UniquePtr::UniquePtr(UniquePtr&&)). +function bodyEatsVariable(variable, body, startpoint) +{ + const successors = getSuccessors(body); + const work = [startpoint]; + while (work.length > 0) { + const point = work.shift(); + if (!(point in successors)) + continue; + for (const edge of successors[point]) { + if (edgeMovesVariable(edge, variable)) + return true; + // edgeStartsValueLiveRange will find places where 'variable' is given a + // new value. Never observed in practice, since this function is + // only called with a temporary resulting from std::move(), which + // is used immediately for a call. But just to be robust to future + // uses: + if (!edgeStartsValueLiveRange(edge, variable)) + work.push(edge.Index[1]); + } + } + return false; +} + +// Return whether an edge "clears out" a variable's value. A simple example +// would be +// +// var = nullptr; +// +// for analyses for which nullptr is a "safe" value (eg GC rooting hazards; you +// can't get in trouble by holding a nullptr live across a GC.) A more complex +// example is a Maybe that gets reset: +// +// Maybe nogc; +// nogc.emplace(cx); +// nogc.reset(); +// gc(); // <-- not a problem; nogc is invalidated by prev line +// nogc.emplace(cx); +// foo(nogc); +// +// Yet another example is a UniquePtr being passed by value, which means the +// receiver takes ownership: +// +// UniquePtr uobj(obj); +// foo(uobj); +// gc(); +// +function edgeEndsValueLiveRange(edge, variable, body) +{ + // var = nullptr; + if (edge.Kind == "Assign") { + const [lhs, rhs] = edge.Exp; + return expressionIsVariable(lhs, variable) && isImmobileValue(rhs); + } + + if (edge.Kind != "Call") + return false; + + var callee = edge.Exp[0]; + + if (edge.Type.Kind == 'Function' && + edge.Exp[0].Kind == 'Var' && + edge.Exp[0].Variable.Kind == 'Func' && + edge.Exp[0].Variable.Name[1] == 'MarkVariableAsGCSafe' && + edge.Exp[0].Variable.Name[0].includes("JS::detail::MarkVariableAsGCSafe") && + expressionIsVariable(edge.PEdgeCallArguments.Exp[0], variable)) + { + // explicit JS_HAZ_VARIABLE_IS_GC_SAFE annotation + return true; + } + + if (edge.Type.Kind == 'Function' && + edge.Exp[0].Kind == 'Var' && + edge.Exp[0].Variable.Kind == 'Func' && + edge.Exp[0].Variable.Name[1] == 'move' && + edge.Exp[0].Variable.Name[0].includes('std::move(') && + expressionIsVariable(edge.PEdgeCallArguments.Exp[0], variable) && + edge.Exp[1].Kind == 'Var' && + edge.Exp[1].Variable.Kind == 'Temp') + { + // temp = std::move(var) + // + // If var is a UniquePtr, and we pass it into something that takes + // ownership, then it should be considered to be invalid. It really + // ought to be invalidated at the point of the function call that calls + // the move constructor, but given that we're creating a temporary here + // just for the purpose of passing it in, this edge is good enough. + const lhs = edge.Exp[1].Variable; + if (bodyEatsVariable(lhs, body, edge.Index[1])) + return true; + } + + if (edge.Type.Kind == 'Function' && + edge.Type.TypeFunctionCSU && + edge.PEdgeCallInstance && + expressionIsMethodOnVariable(edge.PEdgeCallInstance.Exp, variable)) + { + const typeName = edge.Type.TypeFunctionCSU.Type.Name; + const m = typeName.match(/^(((\w|::)+?)(\w+)). + const ctorName = `${namespace}${classname}::${classname}()`; + if (callee.Kind == 'Var' && + typesWithSafeConstructors.has(type) && + callee.Variable.Name[0].includes(ctorName)) + { + return true; + } + + // special-case: UniquePtr::reset() and similar. + if (callee.Kind == 'Var' && + type in resetterMethods && + resetterMethods[type].has(callee.Variable.Name[1])) + { + return true; + } + } + } + + // special-case: passing UniquePtr by value. + if (edge.Type.Kind == 'Function' && + edge.Type.TypeFunctionArgument && + edge.PEdgeCallArguments) + { + for (const i in edge.Type.TypeFunctionArgument) { + const param = edge.Type.TypeFunctionArgument[i]; + if (param.Type.Kind != 'CSU') + continue; + if (!param.Type.Name.startsWith("mozilla::UniquePtr<")) + continue; + const arg = edge.PEdgeCallArguments.Exp[i]; + if (expressionIsVariable(arg, variable)) { + return true; + } + } + } + + return false; +} + function edgeCanGC(edge) { if (edge.Kind != "Call") @@ -155,57 +550,10 @@ function edgeCanGC(edge) return false; } -// Search upwards through a function's control flow graph (CFG) to find a path containing: -// -// - a use of a variable, preceded by -// -// - a function call that can GC, preceded by -// -// - a use of the variable that shows that the live range starts at least that -// far back, preceded by -// -// - an informative use of the variable (which might be the same use), one that -// assigns to it a value that might contain a GC pointer (or is the start of -// the function for parameters or 'this'.) This is not necessary for -// correctness, it just makes it easier to understand why something might be -// a hazard. The output of the analysis will include the whole path from the -// informative use to the post-GC use, to make the problem as understandable -// as possible. -// -// A canonical example might be: -// -// void foo() { -// JS::Value* val = lookupValue(); <-- informative use -// if (!val.isUndefined()) { <-- any use -// GC(); <-- GC call -// } -// putValue(val); <-- a use after a GC -// } -// -// The search is performed on an underlying CFG that we traverse in -// breadth-first order (to find the shortest path). We build a path starting -// from an empty path and conditionally lengthening and improving it according -// to the computation occurring on each incoming edge. (If that path so far -// does not have a GC call and we traverse an edge with a GC call, then we -// lengthen the path by that edge and record it as including a GC call.) The -// resulting path may include a point or edge more than once! For example, in: -// -// void foo(JS::Value val) { -// for (int i = 0; i < N; i++) { -// GC(); -// val = processValue(val); -// } -// } -// -// the path would start at the point after processValue(), go through the GC(), -// then back to the processValue() (for the call in the previous loop -// iteration). -// -// While searching, each point is annotated with a path node corresponding to -// the best path found to that node so far. When a later search ends up at the -// same point, the best path node is kept. (But the path that it heads may -// include an earlier path node for the same point, as in the case above.) -// +// Search recursively through predecessors from the use of a variable's value, +// returning whether a GC call is reachable (in the reverse direction; this +// means that the variable use is reachable from the GC call, and therefore the +// variable is live after the GC call), along with some additional information. // What info we want depends on whether the variable turns out to be live // across a GC call. We are looking for both hazards (unrooted variables live // across GC calls) and unnecessary roots (rooted variables that have no GC @@ -218,286 +566,200 @@ function edgeCanGC(edge) // // If so: // -// - 'successor': a path from the GC call to a use of the variable after the GC -// call, chained through 'successor' field in the returned edge descriptor +// - 'why': a path from the GC call to a use of the variable after the GC +// call, chained through a 'why' field in the returned edge descriptor // // - 'gcInfo': a direct pointer to the GC call edge // -function findGCBeforeValueUse(start_body, start_point, suppressed_bits, variable) +function findGCBeforeValueUse(start_body, start_point, suppressed, variable) { - const isGCSuppressed = Boolean(suppressed_bits & ATTR_GC_SUPPRESSED); - // Scan through all edges preceding an unrooted variable use, using an - // explicit worklist, looking for a GC call and a preceding point where the - // variable is known to be live. A worklist contains an incoming edge - // together with a description of where it or one of its successors GC'd - // (if any). + // explicit worklist, looking for a GC call. A worklist contains an + // incoming edge together with a description of where it or one of its + // successors GC'd (if any). - class Path { - get ProgressProperties() { return ["informativeUse", "anyUse", "gcInfo"]; } + var bodies_visited = new Map(); - constructor(successor_path, body, ppoint) { - Object.assign(this, {body, ppoint}); - if (successor_path !== undefined) { - this.successor = successor_path; - for (const prop of this.ProgressProperties) { - if (prop in successor_path) { - this[prop] = successor_path[prop]; + let worklist = [{body: start_body, ppoint: start_point, preGCLive: false, gcInfo: null, why: null}]; + while (worklist.length) { + // Grab an entry off of the worklist, representing a point within the + // CFG identified by . If this point has a descendant + // later in the CFG that can GC, gcInfo will be set to the information + // about that GC call. + + var entry = worklist.pop(); + var { body, ppoint, gcInfo, preGCLive } = entry; + + // Handle the case where there are multiple ways to reach this point + // (traversing backwards). + var visited = bodies_visited.get(body); + if (!visited) + bodies_visited.set(body, visited = new Map()); + if (visited.has(ppoint)) { + var seenEntry = visited.get(ppoint); + + // This point already knows how to GC through some other path, so + // we have nothing new to learn. (The other path will consider the + // predecessors.) + if (seenEntry.gcInfo) + continue; + + // If this worklist's entry doesn't know of any way to GC, then + // there's no point in continuing the traversal through it. Perhaps + // another edge will be found that *can* GC; otherwise, the first + // route to the point will traverse through predecessors. + // + // Note that this means we may visit a point more than once, if the + // first time we visit we don't have a known reachable GC call and + // the second time we do. + if (!gcInfo) + continue; + } + visited.set(ppoint, {body: body, gcInfo: gcInfo}); + + // Check for hitting the entry point of the current body (which may be + // the outer function or a loop within it.) + if (ppoint == body.Index[0]) { + if (body.BlockId.Kind == "Loop") { + // Propagate to outer body parents that enter the loop body. + if ("BlockPPoint" in body) { + for (var parent of body.BlockPPoint) { + var found = false; + for (var xbody of functionBodies) { + if (sameBlockId(xbody.BlockId, parent.BlockId)) { + assert(!found); + found = true; + worklist.push({body: xbody, ppoint: parent.Index, + gcInfo: gcInfo, why: entry}); + } + } + assert(found); } } + + // Also propagate to the *end* of this loop, for the previous + // iteration. + worklist.push({body: body, ppoint: body.Index[1], + gcInfo: gcInfo, why: entry}); + } else if ((variable.Kind == "Arg" || variable.Kind == "This") && gcInfo) { + // The scope of arguments starts at the beginning of the + // function + return entry; + } else if (entry.preGCLive) { + // We didn't find a "good" explanation beginning of the live + // range, but we do know the variable was live across the GC. + // This can happen if the live range started when a variable is + // used as a retparam. + return entry; } } - toString() { - const trail = []; - for (let path = this; path.ppoint; path = path.successor) { - trail.push(path.ppoint); - } - return trail.join(); - } + var predecessors = getPredecessors(body); + if (!(ppoint in predecessors)) + continue; - // Return -1, 0, or 1 to indicate how complete this Path is compared - // to another one. - compare(other) { - for (const prop of this.ProgressProperties) { - const a = this.hasOwnProperty(prop); - const b = other.hasOwnProperty(prop); - if (a != b) { - return a - b; - } - } - return 0; - } - }; - - // In case we never find an informative use, keep track of the best path - // found with any use. - let bestPathWithAnyUse = null; - - const visitor = new class extends Visitor { - constructor() { - super(functionBodies); - } - - // Do a BFS upwards through the CFG, starting from a use of the - // variable and searching for a path containing a GC followed by an - // initializing use of the variable (or, in forward direction, a start - // of the variable's live range, a GC within that live range, and then - // a use showing that the live range extends past the GC call.) - // Actually, possibly two uses: any use at all, and then if available - // an "informative" use that is more convincing (they may be the same). - // - // The CFG is a graph (a 'body' here is acyclic, but they can contain - // loop nodes that bridge to additional bodies for the loop, so the - // overall graph can by cyclic.) That means there may be multiple paths - // from point A to point B, and we want paths with a GC on them. This - // can be thought of as searching for a "maximal GCing" path from a use - // A to an initialization B. - // - // This is implemented as a BFS search that when it reaches a point - // that has been visited before, stops if and only if the current path - // being advanced is a less GC-ful path. The traversal pushes a - // `gcInfo` token, initially empty, up through the graph and stores the - // maximal one visited so far at every point. - // - // Note that this means we may traverse through the same point more - // than once, and so in theory this scan is superlinear -- if you visit - // every point twice, once for a non GC path and once for a GC path, it - // would be 2^n. But that is unlikely to matter, since you'd need lots - // of split/join pairs that GC on one side and not the other, and you'd - // have to visit them in an unlucky order. This could be fixed by - // updating the gcInfo for past points in a path when a GC is found, - // but it hasn't been found to matter in practice yet. - - next_action(prev, current) { - // Continue if first visit, or the new path is more complete than the old path. This - // could be enhanced at some point to choose paths with 'better' - // examples of GC (eg a call that invokes GC through concrete functions rather than going through a function pointer that is conservatively assumed to GC.) - - if (!current) { - // This search path has been terminated. - return "prune"; - } - - if (current.informativeUse) { - // We have a path with an informative use leading to a GC - // leading to the starting point. - assert(current.gcInfo); - return "done"; - } - - if (prev === undefined) { - // first visit - return "continue"; - } - - if (!prev.gcInfo && current.gcInfo) { - // More GC. - return "continue"; - } else { - return "prune"; - } - } - - merge_info(prev, current) { - // Keep the most complete path. - - if (!prev || !current) { - return prev || current; - } - - // Tie goes to the first found, since it will be shorter when doing a BFS-like search. - return prev.compare(current) >= 0 ? prev : current; - } - - extend_path(edge, body, ppoint, successor_path) { - // Clone the successor path node and then tack on the new point. Other values - // will be updated during the rest of this function, according to what is - // happening on the edge. - const path = new Path(successor_path, body, ppoint); - if (edge === null) { - // Artificial edge to connect loops to their surrounding nodes in the outer body. - // Does not influence "completeness" of path. - return path; - } - - assert(ppoint == edge.Index[0]); + for (var edge of predecessors[ppoint]) { + var source = edge.Index[0]; if (edgeEndsValueLiveRange(edge, variable, body)) { - // Terminate the search through this point. - return null; + // Terminate the search through this point; we thought we were + // within the live range, but it turns out that the variable + // was set to a value that we don't care about. + continue; } - const edge_starts = edgeStartsValueLiveRange(edge, variable); - const edge_uses = edgeUsesVariable(edge, variable, body); + var edge_kills = edgeStartsValueLiveRange(edge, variable); + var edge_uses = edgeUsesVariable(edge, variable, body); - if (edge_starts || edge_uses) { - if (!body.minimumUse || ppoint < body.minimumUse) - body.minimumUse = ppoint; + if (edge_kills || edge_uses) { + if (!body.minimumUse || source < body.minimumUse) + body.minimumUse = source; } - if (edge_starts) { + if (edge_kills) { // This is a beginning of the variable's live range. If we can // reach a GC call from here, then we're done -- we have a path - // from the beginning of the live range, through the GC call, to a - // use after the GC call that proves its live range extends at - // least that far. - if (path.gcInfo) { - path.anyUse = path.anyUse || edge; - path.informativeUse = path.informativeUse || edge; - return path; - } + // from the beginning of the live range, through the GC call, + // to a use after the GC call that proves its live range + // extends at least that far. + if (gcInfo) + return {body: body, ppoint: source, gcInfo: gcInfo, why: entry }; - // Otherwise, truncate this particular branch of the search at this - // edge -- there is no GC after this use, and traversing the edge - // would lead to a different live range. - return null; + // Otherwise, keep searching through the graph, but truncate + // this particular branch of the search at this edge. + continue; } - // The value is live across this edge. Check whether this edge can GC (if we don't have a GC yet on this path.) - const had_gcInfo = Boolean(path.gcInfo); - if (!path.gcInfo && !(body.attrs[ppoint] & ATTR_GC_SUPPRESSED) && !isGCSuppressed) { + var src_gcInfo = gcInfo; + var src_preGCLive = preGCLive; + if (!gcInfo && !(body.attrs[source] & ATTR_GC_SUPPRESSED) && !(suppressed & ATTR_GC_SUPPRESSED)) { var gcName = edgeCanGC(edge, body); - if (gcName) { - path.gcInfo = {name:gcName, body, ppoint}; + if (gcName) + src_gcInfo = {name:gcName, body:body, ppoint:source}; + } + + if (edge_uses) { + // The live range starts at least this far back, so we're done + // for the same reason as with edge_kills. The only difference + // is that a GC on this edge indicates a hazard, whereas if + // we're killing a live range in the GC call then it's not live + // *across* the call. + // + // However, we may want to generate a longer usage chain for + // the variable than is minimally necessary. For example, + // consider: + // + // Value v = f(); + // if (v.isUndefined()) + // return false; + // gc(); + // return v; + // + // The call to .isUndefined() is considered to be a use and + // therefore indicates that v must be live at that point. But + // it's more helpful to the user to continue the 'why' path to + // include the ancestor where the value was generated. So we + // will only return here if edge.Kind is Assign; otherwise, + // we'll pass a "preGCLive" value up through the worklist to + // remember that the variable *is* alive before the GC and so + // this function should be returning a true value even if we + // don't find an assignment. + + if (src_gcInfo) { + src_preGCLive = true; + if (edge.Kind == 'Assign') + return {body:body, ppoint:source, gcInfo:src_gcInfo, why:entry}; } } - // Beginning of function? - if (ppoint == body.Index[0] && body.BlockId.Kind != "Loop") { - if (path.gcInfo && (variable.Kind == "Arg" || variable.Kind == "This")) { - // The scope of arguments starts at the beginning of the - // function. - path.anyUse = path.informativeUse = true; - } - - if (path.anyUse) { - // We know the variable was live across the GC. We may or - // may not have found an "informative" explanation - // beginning of the live range. (This can happen if the - // live range started when a variable is used as a - // retparam.) - return path; + if (edge.Kind == "Loop") { + // Additionally propagate the search into a loop body, starting + // with the exit point. + var found = false; + for (var xbody of functionBodies) { + if (sameBlockId(xbody.BlockId, edge.BlockId)) { + assert(!found); + found = true; + worklist.push({body:xbody, ppoint:xbody.Index[1], + preGCLive: src_preGCLive, gcInfo:src_gcInfo, + why:entry}); + } } + assert(found); + // Don't continue to predecessors here without going through + // the loop. (The points in this body that enter the loop will + // be traversed when we reach the entry point of the loop.) + break; } - if (!path.gcInfo) { - // We haven't reached a GC yet, so don't start looking for uses. - return path; - } + // Propagate the search to the predecessors of this edge. + worklist.push({body:body, ppoint:source, + preGCLive: src_preGCLive, gcInfo:src_gcInfo, + why:entry}); + } + } - if (!edge_uses) { - // We have a GC. If this edge doesn't use the value, then there - // is no change to the completeness of the path. - return path; - } - - // The live range starts at least this far back, so we're done for - // the same reason as with edge_starts. The only difference is that - // a GC on this edge indicates a hazard, whereas if we're killing a - // live range in the GC call then it's not live *across* the call. - // - // However, we may want to generate a longer usage chain for the - // variable than is minimally necessary. For example, consider: - // - // Value v = f(); - // if (v.isUndefined()) - // return false; - // gc(); - // return v; - // - // The call to .isUndefined() is considered to be a use and - // therefore indicates that v must be live at that point. But it's - // more helpful to the user to continue the 'successor' path to - // include the ancestor where the value was generated. So we will - // only stop here if edge.Kind is Assign; otherwise, we'll pass a - // "preGCLive" value up through the worklist to remember that the - // variable *is* alive before the GC and so this function should be - // returning a true value even if we don't find an assignment. - - // One special case: if the use of the variable is on the - // destination part of the edge (which currently only happens for - // the return value and a terminal edge in the body), and this edge - // is also GCing, then that usage happens *after* the GC and so - // should not be used for anyUse or informativeUse. This matters - // for a hazard involving a destructor GC'ing after an immobile - // return value has been assigned: - // - // GCInDestructor guard(cx); - // if (cond()) { - // return nullptr; - // } - // - // which boils down to - // - // p1 --(construct guard)--> - // p2 --(call cond)--> - // p3 --(returnval := nullptr) --> - // p4 --(destruct guard, possibly GCing)--> - // p5 - // - // The return value is considered to be live at p5. The live range - // of the return value would ordinarily be from p3->p4->p5, except - // that the nullptr assignment means it needn't be considered live - // back that far, and so the live range is *just* p5. The GC on the - // 4->5 edge happens just before that range, so the value was not - // live across the GC. - // - if (!had_gcInfo && edge_uses == edge.Index[1]) { - return path; // New GC does not cross this variable use. - } - - path.anyUse = path.anyUse || edge; - bestPathWithAnyUse = bestPathWithAnyUse || path; - if (edge.Kind == 'Assign') { - path.informativeUse = edge; // Done! Setting this terminates the search. - } - - return path; - }; - }; - - return BFS_upwards(start_body, start_point, functionBodies, visitor, new Path()) || bestPathWithAnyUse; + return null; } function variableLiveAcrossGC(suppressed, variable) @@ -580,8 +842,8 @@ function unsafeVariableAddressTaken(suppressed, variable) // given function and store it. function loadPrintedLines(functionName) { - assert(!os.system("xdbfind src_body.xdb '" + functionName + "' > " + options.tmpfile)); - var lines = snarf(options.tmpfile).split('\n'); + assert(!os.system("xdbfind src_body.xdb '" + functionName + "' > " + tmpfile)); + var lines = snarf(tmpfile).split('\n'); for (var body of functionBodies) body.lines = []; @@ -644,15 +906,14 @@ function printEntryTrace(functionName, entry) if (!functionBodies[0].lines) loadPrintedLines(functionName); - while (entry.successor) { + while (entry) { var ppoint = entry.ppoint; var lineText = findLocation(entry.body, ppoint, {"brief": true}); var edgeText = ""; - if (entry.successor && entry.successor.body == entry.body) { - // If the next point in the trace is in the same block, look for an - // edge between them. - var next = entry.successor.ppoint; + if (entry.why && entry.why.body == entry.body) { + // If the next point in the trace is in the same block, look for an edge between them. + var next = entry.why.ppoint; if (!entry.body.edgeTable) { var table = {}; @@ -686,7 +947,7 @@ function printEntryTrace(functionName, entry) } print(" " + lineText + (edgeText.length ? ": " + edgeText : "")); - entry = entry.successor; + entry = entry.why; } } @@ -756,8 +1017,7 @@ function processBodies(functionName, wholeBodyAttrs) // Hopefully these will be simplified at some point (see bug 1517829), but // for now we special-case functions in the mozilla::dom namespace that // contain locals with types ending in "Argument". Or - // Maybe. Or Maybe>. It's - // a harsh world. + // Maybe. It's a harsh world. const ignoreVars = new Set(); if (functionName.match(/mozilla::dom::/)) { const vars = functionBodies[0].DefineVariable.filter( @@ -766,11 +1026,9 @@ function processBodies(functionName, wholeBodyAttrs) v => [ v.Variable.Name[0], v.Type.Name ] ); - const holders = vars.filter( - ([n, t]) => n.match(/^arg\d+_holder$/) && - (t.includes("Argument") || t.includes("Rooter"))); + const holders = vars.filter(([n, t]) => n.match(/^arg\d+_holder$/) && t.match(/Argument\b/)); for (const [holder,] of holders) { - ignoreVars.add(holder); // Ignore the holder. + ignoreVars.add(holder); // Ignore the older. ignoreVars.add(holder.replace("_holder", "")); // Ignore the "managed" arg. } } @@ -804,7 +1062,6 @@ function processBodies(functionName, wholeBodyAttrs) } else if (isUnrootedType(variable.Type)) { var result = variableLiveAcrossGC(suppressed, variable.Variable); if (result) { - assert(result.gcInfo); var lineText = findLocation(result.gcInfo.body, result.gcInfo.ppoint); if (annotations.has('Expect Hazards')) { print("\nThis is expected, but '" + functionName + "'" + @@ -848,7 +1105,7 @@ function processBodies(functionName, wholeBodyAttrs) } } -if (options.batch == 1) +if (batch == 1) print("Time: " + new Date); var xdb = xdbLibrary(); @@ -857,8 +1114,8 @@ xdb.open("src_body.xdb"); var minStream = xdb.min_data_stream()|0; var maxStream = xdb.max_data_stream()|0; -var start = batchStart(options.batch, options.numBatches, minStream, maxStream); -var end = batchLast(options.batch, options.numBatches, minStream, maxStream); +var start = batchStart(batch, numBatches, minStream, maxStream); +var end = batchLast(batch, numBatches, minStream, maxStream); function process(name, json) { functionName = name; @@ -877,9 +1134,6 @@ function process(name, json) { if (attrs) pbody.attrs[id] = attrs; } - for (const edgeAttr of gcEdges[blockIdentifier(body)] || []) { - body.attrs[edgeAttr.Index[0]] |= edgeAttr.attrs; - } } // Special case: std::swap of two refcounted values thinks it can drop the @@ -894,11 +1148,11 @@ function process(name, json) { processBodies(functionName, wholeBodyAttrs); } -if (options.function) { - var data = xdb.read_entry(options.function); +if (theFunctionNameToFind) { + var data = xdb.read_entry(theFunctionNameToFind); var json = data.readString(); debugger; - process(options.function, json); + process(theFunctionNameToFind, json); xdb.free_string(data); quit(0); } diff --git a/js/src/devtools/rootAnalysis/annotations.js b/js/src/devtools/rootAnalysis/annotations.js index 314958566e98..3198020a5464 100644 --- a/js/src/devtools/rootAnalysis/annotations.js +++ b/js/src/devtools/rootAnalysis/annotations.js @@ -17,7 +17,7 @@ var ignoreIndirectCalls = { }; // Types that when constructed with no arguments, are "safe" values (they do -// not contain GC pointers, or values with nontrivial destructors.) +// not contain GC pointers). var typesWithSafeConstructors = new Set([ "mozilla::Maybe", "mozilla::dom::Nullable", @@ -32,14 +32,8 @@ var resetterMethods = { 'js::UniquePtr': new Set(["reset"]), 'mozilla::dom::Nullable': new Set(["SetNull"]), 'mozilla::dom::TypedArray_base': new Set(["Reset"]), - 'RefPtr': new Set(["forget"]), - 'nsCOMPtr': new Set(["forget"]), }; -function isRefcountedDtor(name) { - return name.includes("::~RefPtr(") || name.includes("::~nsCOMPtr("); -} - function indirectCallCannotGC(fullCaller, fullVariable) { var caller = readable(fullCaller); diff --git a/js/src/devtools/rootAnalysis/computeCallgraph.js b/js/src/devtools/rootAnalysis/computeCallgraph.js index 3d7d95a4be46..c4737cdc7594 100644 --- a/js/src/devtools/rootAnalysis/computeCallgraph.js +++ b/js/src/devtools/rootAnalysis/computeCallgraph.js @@ -8,46 +8,23 @@ loadRelativeToScript('callgraph.js'); -var options = parse_options([ - { - name: '--function', - type: 'string' - }, - { - name: 'typeInfo_filename', - type: 'string', - default: "typeInfo.txt" - }, - { - name: 'callgraphOut_filename', - type: 'string', - default: "rawcalls.txt" - }, - { - name: 'gcEdgesOut_filename', - type: 'string', - default: "gcEdges.json" - }, - { - name: 'batch', - default: 1, - type: 'number' - }, - { - name: 'numBatches', - default: 1, - type: 'number' - }, -]); +var theFunctionNameToFind; +if (scriptArgs[0] == '--function' || scriptArgs[0] == '-f') { + theFunctionNameToFind = scriptArgs[1]; + scriptArgs = scriptArgs.slice(2); +} -var origOut = os.file.redirect(options.callgraphOut_filename); +var typeInfo_filename = scriptArgs[0] || "typeInfo.txt"; +var callgraphOut_filename = scriptArgs[1] || "rawcalls.txt"; +var batch = (scriptArgs[2]|0) || 1; +var numBatches = (scriptArgs[3]|0) || 1; + +var origOut = os.file.redirect(callgraphOut_filename); var memoized = new Map(); var unmangled2id = new Set(); -var gcEdges = {}; - // Insert a string into the name table and return the ID. Do not use for // functions, which must be handled specially. function getId(name) @@ -128,7 +105,7 @@ function getAnnotations(functionName, body) { // Scan through a function body, pulling out all annotations and calls and // recording them in callgraph.txt. -function processBody(functionName, body, functionBodies) +function processBody(functionName, body) { if (!('PEdge' in body)) return; @@ -161,13 +138,6 @@ function processBody(functionName, body, functionBodies) var edgeAttrs = body.attrs[edge.Index[0]] | 0; for (var callee of getCallees(edge)) { - // Special-case some calls when we can derive more information about them, eg - // that they are a destructor that won't do anything. - if (callee.kind === "direct" && edgeIsNonReleasingDtor(body, edge, callee.name, functionBodies)) { - const block = blockIdentifier(body); - addToKeyedList(gcEdges, block, { Index: edge.Index, attrs: ATTR_GC_SUPPRESSED | ATTR_NONRELEASING }); - } - // Individual callees may have additional attrs. The only such // bit currently is that nsISupports.{AddRef,Release} are assumed // to never GC. @@ -228,7 +198,7 @@ assert(ID.nogcfunc == functionId("(nogc-function)")); // garbage collection assert(ID.gc == functionId("(GC)")); -var typeInfo = loadTypeInfo(options.typeInfo_filename); +var typeInfo = loadTypeInfo(typeInfo_filename); loadTypes("src_comp.xdb"); @@ -286,8 +256,8 @@ printErr("Finished loading data structures"); var minStream = xdb.min_data_stream(); var maxStream = xdb.max_data_stream(); -if (options.function) { - var index = xdb.lookup_key(options.function); +if (theFunctionNameToFind) { + var index = xdb.lookup_key(theFunctionNameToFind); if (!index) { printErr("Function not found"); quit(1); @@ -307,7 +277,7 @@ function process(functionName, functionBodies) } for (var body of functionBodies) - processBody(functionName, body, functionBodies); + processBody(functionName, body); // Not strictly necessary, but add an edge from the synthetic "(js-code)" // to RunScript to allow better stacks than just randomly selecting a @@ -420,8 +390,8 @@ function process(functionName, functionBodies) printOnce(`D ${functionId("(js-code)")} ${functionId(functionName)}`); } -var start = batchStart(options.batch, options.numBatches, minStream, maxStream); -var end = batchLast(options.batch, options.numBatches, minStream, maxStream); +var start = batchStart(batch, numBatches, minStream, maxStream); +var end = batchLast(batch, numBatches, minStream, maxStream); for (var nameIndex = start; nameIndex <= end; nameIndex++) { var name = xdb.read_key(nameIndex); @@ -431,8 +401,4 @@ for (var nameIndex = start; nameIndex <= end; nameIndex++) { xdb.free_string(data); } -os.file.close(os.file.redirect(options.gcEdgesOut_filename)); - -print(JSON.stringify(gcEdges, null, 4)); - os.file.close(os.file.redirect(origOut)); diff --git a/js/src/devtools/rootAnalysis/computeGCFunctions.js b/js/src/devtools/rootAnalysis/computeGCFunctions.js index 90705051c911..02edc9399cc2 100644 --- a/js/src/devtools/rootAnalysis/computeGCFunctions.js +++ b/js/src/devtools/rootAnalysis/computeGCFunctions.js @@ -18,52 +18,31 @@ if (typeof scriptArgs[0] != 'string') var start = "Time: " + new Date; -try { - var options = parse_options([ - { - name: 'inputs', - dest: 'rawcalls_filenames', - nargs: '+' - }, - { - name: '--outputs', - type: 'bool' - }, - { - name: 'callgraph', - type: 'string', - default: 'callgraph.txt' - }, - { - name: 'gcFunctions', - type: 'string', - default: 'gcFunctions.txt' - }, - { - name: 'gcFunctionsList', - type: 'string', - default: 'gcFunctions.lst' - }, - { - name: 'limitedFunctions', - type: 'string', - default: 'limitedFunctions.lst' - }, - ]); -} catch { - printErr("Usage: computeGCFunctions.js ... --outputs "); - quit(1); -}; +var rawcalls_filenames = []; +while (scriptArgs.length) { + const arg = scriptArgs.shift(); + if (arg == '--outputs') + break; + rawcalls_filenames.push(arg); +} +if (scriptArgs.length == 0) + usage(); + +var callgraph_filename = scriptArgs[0] || "callgraph.txt"; +var gcFunctions_filename = scriptArgs[1] || "gcFunctions.txt"; +var gcFunctionsList_filename = scriptArgs[2] || "gcFunctions.lst"; +var gcEdges_filename = scriptArgs[3] || "gcEdges.txt"; +var limitedFunctionsList_filename = scriptArgs[4] || "limitedFunctions.lst"; var { gcFunctions, functions, calleesOf, limitedFunctions -} = loadCallgraph(options.rawcalls_filenames); +} = loadCallgraph(rawcalls_filenames); -printErr("Writing " + options.gcFunctions); -redirect(options.gcFunctions); +printErr("Writing " + gcFunctions_filename); +redirect(gcFunctions_filename); for (var name in gcFunctions) { for (let readable of (functions.readableName[name] || [name])) { @@ -83,8 +62,8 @@ for (var name in gcFunctions) { } } -printErr("Writing " + options.gcFunctionsList); -redirect(options.gcFunctionsList); +printErr("Writing " + gcFunctionsList_filename); +redirect(gcFunctionsList_filename); for (var name in gcFunctions) { if (name in functions.readableName) { for (var readable of functions.readableName[name]) @@ -94,10 +73,28 @@ for (var name in gcFunctions) { } } -printErr("Writing " + options.limitedFunctions); -redirect(options.limitedFunctions); +// gcEdges is a list of edges that can GC for more specific reasons than just +// calling a function that is in gcFunctions.txt. +// +// Right now, it is unused. It was meant for ~AutoRealm when it might +// wrap an exception, but anything held live across ~AC will have to be held +// live across the corresponding constructor (and hence the whole scope of the +// AC), and in that case it'll be held live across whatever could create an +// exception within the AC scope. So ~AC edges are redundant. I will leave the +// stub machinery here for now. +printErr("Writing " + gcEdges_filename); +redirect(gcEdges_filename); +for (var block in gcEdges) { + for (var edge in gcEdges[block]) { + var func = gcEdges[block][edge]; + print([ block, edge, func ].join(" || ")); + } +} + +printErr("Writing " + limitedFunctionsList_filename); +redirect(limitedFunctionsList_filename); print(JSON.stringify(limitedFunctions, null, 4)); -printErr("Writing " + options.callgraph); -redirect(options.callgraph); +printErr("Writing " + callgraph_filename); +redirect(callgraph_filename); saveCallgraph(functions, calleesOf); diff --git a/js/src/devtools/rootAnalysis/computeGCTypes.js b/js/src/devtools/rootAnalysis/computeGCTypes.js index 9fc72f6d4442..a52aed832af3 100644 --- a/js/src/devtools/rootAnalysis/computeGCTypes.js +++ b/js/src/devtools/rootAnalysis/computeGCTypes.js @@ -9,10 +9,8 @@ loadRelativeToScript('utility.js'); loadRelativeToScript('annotations.js'); -var options = parse_options([ - { name: "gcTypes", default: "gcTypes.txt" }, - { name: "typeInfo", default: "typeInfo.txt" } -]); +var gcTypes_filename = scriptArgs[0] || "gcTypes.txt"; +var typeInfo_filename = scriptArgs[1] || "typeInfo.txt"; var typeInfo = { 'GCPointers': [], @@ -490,7 +488,7 @@ function explain(csu, indent, seen) { } } -var origOut = os.file.redirect(options.gcTypes); +var origOut = os.file.redirect(gcTypes_filename); for (var csu in gcTypes) { print("GCThing: " + csu); @@ -502,7 +500,7 @@ for (var csu in gcPointers) { } // Redirect output to the typeInfo file and close the gcTypes file. -os.file.close(os.file.redirect(options.typeInfo)); +os.file.close(os.file.redirect(typeInfo_filename)); // Compute the set of types that suppress GC within their RAII scopes (eg // AutoSuppressGC, AutoSuppressGCForAnalysis). diff --git a/js/src/devtools/rootAnalysis/loadCallgraph.js b/js/src/devtools/rootAnalysis/loadCallgraph.js index eef76c24b356..d824dd01495c 100644 --- a/js/src/devtools/rootAnalysis/loadCallgraph.js +++ b/js/src/devtools/rootAnalysis/loadCallgraph.js @@ -419,11 +419,8 @@ function loadCallgraph(files) // set of mangled names (map from mangled name => {any,all,recursive_root:bool} var limitedFunctions = {}; - for (const [id, [any, all]] of Object.entries(functionAttrs)) { - if (all) { - limitedFunctions[functions.name[id]] = { attributes: all }; - } - } + for (const [id, [any, all]] of Object.entries(functionAttrs)) + limitedFunctions[functions.name[id]] = { attributes: all }; for (const [id, limits, label] of recursive_roots) { const name = functions.name[id]; diff --git a/js/src/devtools/rootAnalysis/mergeJSON.js b/js/src/devtools/rootAnalysis/mergeJSON.js deleted file mode 100644 index 56fb3a2ab1c3..000000000000 --- a/js/src/devtools/rootAnalysis/mergeJSON.js +++ /dev/null @@ -1,18 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this file, - * You can obtain one at http://mozilla.org/MPL/2.0/. */ - -/* -*- indent-tabs-mode: nil; js-indent-level: 4 -*- */ - -var infiles = [...scriptArgs]; -var outfile = infiles.pop(); - -const output = {}; -for (const filename of infiles) { - const data = JSON.parse(os.file.readFile(filename)); - Object.assign(output, data); -} - -var origOut = os.file.redirect(outfile); -print(JSON.stringify(output, null, 4)); -os.file.close(os.file.redirect(origOut)); diff --git a/js/src/devtools/rootAnalysis/run-test.py b/js/src/devtools/rootAnalysis/run-test.py index f0bafb2babf2..cd02d68939d4 100755 --- a/js/src/devtools/rootAnalysis/run-test.py +++ b/js/src/devtools/rootAnalysis/run-test.py @@ -108,7 +108,6 @@ make_dir(outroot) os.environ["HAZARD_RUN_INTERNAL_TESTS"] = "1" failed = set() -passed = set() for name in cfg.tests: name = os.path.basename(name) indir = os.path.join(testdir, name) @@ -135,9 +134,6 @@ for name in cfg.tests: raise else: print("TEST-PASSED: %s" % name) - passed.add(name) if failed: raise Exception("Failed tests: " + " ".join(failed)) - -print(f"All {len(passed)} tests passed.") diff --git a/js/src/devtools/rootAnalysis/t/hazards/source.cpp b/js/src/devtools/rootAnalysis/t/hazards/source.cpp index fc5ba8464a28..e866a789f7b4 100644 --- a/js/src/devtools/rootAnalysis/t/hazards/source.cpp +++ b/js/src/devtools/rootAnalysis/t/hazards/source.cpp @@ -367,46 +367,3 @@ class Subcell : public Cell { return f; // this->f } }; - -template -struct RefPtr { - ~RefPtr() { GC(); } - void forget() {} -}; - -Cell* refptr_test1() { - static Cell cell; - RefPtr v1; - Cell* ref_unsafe1 = &cell; - return ref_unsafe1; -} - -Cell* refptr_test2() { - static Cell cell; - RefPtr v2; - Cell* ref_safe2 = &cell; - v2.forget(); - return ref_safe2; -} - -Cell* refptr_test3() { - static Cell cell; - RefPtr v3; - Cell* ref_unsafe3 = &cell; - if (x) { - v3.forget(); - } - return ref_unsafe3; -} - -Cell* refptr_test4() { - static Cell cell; - RefPtr r; - return &cell; // hazard in return value -} - -Cell* refptr_test5() { - static Cell cell; - RefPtr r; - return nullptr; // returning immobile value, so no hazard -} diff --git a/js/src/devtools/rootAnalysis/t/hazards/test.py b/js/src/devtools/rootAnalysis/t/hazards/test.py index e6d992f05f25..aa39187c2001 100644 --- a/js/src/devtools/rootAnalysis/t/hazards/test.py +++ b/js/src/devtools/rootAnalysis/t/hazards/test.py @@ -7,6 +7,7 @@ test.run_analysis_script("gcTypes") # gcFunctions should be the inverse, but we get to rely on unmangled names here. gcFunctions = test.load_gcFunctions() +print(gcFunctions) assert "void GC()" in gcFunctions assert "void suppressedFunction()" not in gcFunctions assert "void halfSuppressedFunction()" in gcFunctions @@ -25,11 +26,10 @@ assert "cell6" not in hazmap assert "" in hazmap assert "this" in hazmap -# All hazards should be in f(), loopy(), safevals(), method(), and refptr_test{1,3,4}() +# All hazards should be in f(), loopy(), safevals(), and method() assert hazmap["cell2"].function == "Cell* f()" -haz_functions = set(haz.function for haz in hazards) -print(haz_functions) -assert len(haz_functions) == 7 +print(len(set(haz.function for haz in hazards))) +assert len(set(haz.function for haz in hazards)) == 4 # Check that the correct GC call is reported for each hazard. (cell3 has a # hazard from two different GC calls; it doesn't really matter which is @@ -39,16 +39,8 @@ assert hazmap["cell3"].GCFunction in ( "void halfSuppressedFunction()", "void unsuppressedFunction()", ) -returnval_hazards = set( - haz.function for haz in hazards if haz.variable == "" -) -assert returnval_hazards == set( - [ - "Cell* f()", - "Cell* refptr_test1()", - "Cell* refptr_test3()", - "Cell* refptr_test4()", - ] +assert hazmap[""].GCFunction.startswith( + "void GCInDestructor::~GCInDestructor()" ) assert "container1" in hazmap diff --git a/js/src/devtools/rootAnalysis/utility.js b/js/src/devtools/rootAnalysis/utility.js index 35156c6de4b1..8461b782b0e6 100644 --- a/js/src/devtools/rootAnalysis/utility.js +++ b/js/src/devtools/rootAnalysis/utility.js @@ -14,7 +14,6 @@ loadRelativeToScript('dumpCFG.js'); var ATTR_GC_SUPPRESSED = 1; var ATTR_CANSCRIPT_BOUNDED = 2; // Unimplemented var ATTR_DOM_ITERATING = 4; // Unimplemented -var ATTR_NONRELEASING = 8; // ~RefPtr of value whose refcount will not go to zero var ATTRS_NONE = 0; var ATTRS_ALL = 7; // All possible bits set @@ -75,131 +74,64 @@ function xprint(x, padding) } } -// Command-line argument parser. -// -// `parameters` is a dict of parameters specs, each of which is a dict with keys: -// -// - name: name of option, prefixed with "--" if it is named (otherwise, it -// is interpreted as a positional parameter.) -// - dest: key to store the result in, defaulting to the parameter name without -// any leading "--"" and with dashes replaced with underscores. -// - default: value of option if no value is given. Positional parameters with -// a default value are optional. If no default is given, the parameter's name -// is not included in the return value. -// - type: `bool` if it takes no argument, otherwise an argument is required. -// Named arguments default to 'bool', positional arguments to 'string'. -// - nargs: the only supported value is `+`, which means to grab all following -// arguments, up to the next named option, and store them as a list. -// -// The command line is parsed for `--foo=value` and `--bar` arguments. -// -// Return value is a dict of parameter values, keyed off of `dest` as determined -// above. An extra option named "rest" will be set to the list of all remaining -// arguments passed in. -// function parse_options(parameters, inArgs = scriptArgs) { const options = {}; - const named = {}; + const optional = {}; const positional = []; for (const param of parameters) { if (param.name.startsWith("-")) { - named[param.name] = param; - if (!param.dest) { - if (!param.name.startsWith("--")) { - throw new Error(`parameter '${param.name}' requires param.dest to be set`); - } - param.dest = param.name.substring(2).replace("-", "_"); - } + optional[param.name] = param; + param.dest = param.dest || param.name.substring(2).replace("-", "_"); } else { - if (!('default' in param) && positional.length > 0 && ('default' in positional.at(-1))) { - throw new Error(`required parameter '${param.name}' follows optional parameter`); - } - param.positional = true; positional.push(param); param.dest = param.dest || param.name.replace("-", "_"); } - if (!param.type) { - if (param.nargs === "+") { - param.type = "list"; - } else if (param.positional) { - param.type = "string"; - } else { - param.type = "bool"; - } - } - - if ('default' in param) { + param.type = param.type || 'bool'; + if ('default' in param) options[param.dest] = param.default; - } } options.rest = []; const args = [...inArgs]; - let grabbing_into = undefined; while (args.length > 0) { - let arg = args.shift(); let param; - if (arg.startsWith("-") && arg in named) { - param = named[arg]; - if (param.type !== 'bool') { - if (args.length == 0) { - throw(new Error(`${param.name} requires an argument`)); - } - arg = args.shift(); - } - } else { - const pos = arg.indexOf("="); + let pos = -1; + if (args[0] in optional) + param = optional[args[0]]; + else { + pos = args[0].indexOf("="); if (pos != -1) { - const name = arg.substring(0, pos); - param = named[name]; - if (!param) { - throw(new Error(`Unknown option '${name}'`)); - } else if (param.type === 'bool') { - throw(new Error(`--${param.name} does not take an argument`)); - } - arg = arg.substring(pos + 1); + param = optional[args[0].substring(0, pos)]; + pos++; } } - // If this isn't a --named param, and we're not accumulating into a nargs="+" param, then - // use the next positional. - if (!param && !grabbing_into && positional.length > 0) { - param = positional.shift(); - } - - // If a parameter was identified, then any old accumulator is done and we might start a new one. - if (param) { - if (param.type === 'list') { - grabbing_into = options[param.dest] = options[param.dest] || []; + if (!param) { + if (positional.length > 0) { + param = positional.shift(); + options[param.dest] = args.shift(); } else { - grabbing_into = undefined; + options.rest.push(args.shift()); } + continue; } - if (grabbing_into) { - grabbing_into.push(arg); - } else if (param) { - if (param.type === 'bool') { - options[param.dest] = true; + if (param.type != 'bool') { + if (pos != -1) { + options[param.dest] = args.shift().substring(pos); } else { - options[param.dest] = arg; + args.shift(); + if (args.length == 0) + throw(new Error(`--${param.name} requires an argument`)); + options[param.dest] = args.shift(); } } else { - options.rest.push(arg); - } - } - - for (const param of positional) { - if (!('default' in param)) { - throw(new Error(`'${param.name}' option is required`)); - } - } - - for (const param of parameters) { - if (param.nargs === '+' && options[param.dest].length == 0) { - throw(new Error(`at least one value required for option '${param.name}'`)); + if (pos != -1) + throw(new Error(`--${param.name} does not take an argument`)); + options[param.dest] = true; + args.shift(); } } @@ -254,8 +186,13 @@ function collectBodyEdges(body) function getPredecessors(body) { - if (!('predecessors' in body)) - collectBodyEdges(body); + try { + if (!('predecessors' in body)) + collectBodyEdges(body); + } catch (e) { + debugger; + printErr("body is " + body); + } return body.predecessors; } diff --git a/js/src/vm/UbiNode.cpp b/js/src/vm/UbiNode.cpp index 25e772906fef..91521dbfafbd 100644 --- a/js/src/vm/UbiNode.cpp +++ b/js/src/vm/UbiNode.cpp @@ -361,35 +361,38 @@ const char16_t Concrete::concreteTypeName[] = namespace JS { namespace ubi { -RootList::RootList(JSContext* cx, bool wantNames /* = false */) - : cx(cx), edges(), wantNames(wantNames), inited(false) {} +RootList::RootList(JSContext* cx, Maybe& noGC, + bool wantNames /* = false */) + : noGC(noGC), cx(cx), edges(), wantNames(wantNames) {} -std::pair RootList::init() { +bool RootList::init() { EdgeVectorTracer tracer(cx->runtime(), &edges, wantNames); js::TraceRuntime(&tracer); - inited = tracer.okay; - return {tracer.okay, JS::AutoCheckCannotGC(cx)}; + if (!tracer.okay) { + return false; + } + noGC.emplace(); + return true; } -std::pair RootList::init( - CompartmentSet& debuggees) { +bool RootList::init(CompartmentSet& debuggees) { EdgeVector allRootEdges; EdgeVectorTracer tracer(cx->runtime(), &allRootEdges, wantNames); ZoneSet debuggeeZones; for (auto range = debuggees.all(); !range.empty(); range.popFront()) { if (!debuggeeZones.put(range.front()->zone())) { - return {false, JS::AutoCheckCannotGC(cx)}; + return false; } } js::TraceRuntime(&tracer); if (!tracer.okay) { - return {false, JS::AutoCheckCannotGC(cx)}; + return false; } js::gc::TraceIncomingCCWs(&tracer, debuggees); if (!tracer.okay) { - return {false, JS::AutoCheckCannotGC(cx)}; + return false; } for (EdgeVector::Range r = allRootEdges.all(); !r.empty(); r.popFront()) { @@ -406,14 +409,15 @@ std::pair RootList::init( } if (!edges.append(std::move(edge))) { - return {false, JS::AutoCheckCannotGC(cx)}; + return false; } } - return {true, JS::AutoCheckCannotGC(cx)}; + noGC.emplace(); + return true; } -std::pair RootList::init(HandleObject debuggees) { +bool RootList::init(HandleObject debuggees) { MOZ_ASSERT(debuggees && JS::dbg::IsDebugger(*debuggees)); js::Debugger* dbg = js::Debugger::fromJSObject(debuggees.get()); @@ -422,13 +426,12 @@ std::pair RootList::init(HandleObject debuggees) { for (js::WeakGlobalObjectSet::Range r = dbg->allDebuggees(); !r.empty(); r.popFront()) { if (!debuggeeCompartments.put(r.front()->compartment())) { - return {false, JS::AutoCheckCannotGC(cx)}; + return false; } } - auto [ok, nogc] = init(debuggeeCompartments); - if (!ok) { - return {false, nogc}; + if (!init(debuggeeCompartments)) { + return false; } // Ensure that each of our debuggee globals are in the root list. @@ -436,14 +439,15 @@ std::pair RootList::init(HandleObject debuggees) { r.popFront()) { if (!addRoot(JS::ubi::Node(static_cast(r.front())), u"debuggee global")) { - return {false, nogc}; + return false; } } - return {true, nogc}; + return true; } bool RootList::addRoot(Node node, const char16_t* edgeName) { + MOZ_ASSERT(noGC.isSome()); MOZ_ASSERT_IF(wantNames, edgeName); UniqueTwoByteChars name; diff --git a/js/src/vm/UbiNodeShortestPaths.cpp b/js/src/vm/UbiNodeShortestPaths.cpp index 25dc28590962..b0f2cdb58123 100644 --- a/js/src/vm/UbiNodeShortestPaths.cpp +++ b/js/src/vm/UbiNodeShortestPaths.cpp @@ -46,15 +46,16 @@ static void dumpNode(const JS::ubi::Node& node) { JS_PUBLIC_API void dumpPaths(JSContext* cx, Node node, uint32_t maxNumPaths /* = 10 */) { - JS::ubi::RootList rootList(cx, true); - auto [ok, nogc] = rootList.init(); - MOZ_ASSERT(ok); + mozilla::Maybe nogc; + + JS::ubi::RootList rootList(cx, nogc, true); + MOZ_ASSERT(rootList.init()); NodeSet targets; - ok = targets.putNew(node); + bool ok = targets.putNew(node); MOZ_ASSERT(ok); - auto paths = ShortestPaths::Create(cx, nogc, maxNumPaths, &rootList, + auto paths = ShortestPaths::Create(cx, nogc.ref(), maxNumPaths, &rootList, std::move(targets)); MOZ_ASSERT(paths.isSome());