From c16fe33b0ec71f184d4b49ca285d09888151503a Mon Sep 17 00:00:00 2001 From: Steve Fink Date: Mon, 30 Sep 2019 03:57:52 +0000 Subject: [PATCH] Bug 1531951 - [hazards] Comments and refactoring r=jonco Differential Revision: https://phabricator.services.mozilla.com/D46543 --HG-- extra : moz-landing-system : lando --- js/src/devtools/rootAnalysis/CFG.js | 18 ++++++++---- js/src/devtools/rootAnalysis/analyzeRoots.js | 4 +-- js/src/devtools/rootAnalysis/annotations.js | 17 +++++++---- js/src/devtools/rootAnalysis/callgraph.js | 29 ++++++++++++------- .../devtools/rootAnalysis/computeCallgraph.js | 2 +- js/src/devtools/rootAnalysis/explain.py | 8 ++--- js/src/devtools/rootAnalysis/loadCallgraph.js | 9 +++--- js/src/devtools/rootAnalysis/utility.js | 1 + .../scripts/builder/hazard-analysis.sh | 2 +- 9 files changed, 55 insertions(+), 35 deletions(-) diff --git a/js/src/devtools/rootAnalysis/CFG.js b/js/src/devtools/rootAnalysis/CFG.js index a0c98bcf1a46..2ee1e73ddf6f 100644 --- a/js/src/devtools/rootAnalysis/CFG.js +++ b/js/src/devtools/rootAnalysis/CFG.js @@ -4,10 +4,13 @@ /* -*- indent-tabs-mode: nil; js-indent-level: 4 -*- */ +// Utility code for traversing the JSON data structures produced by sixgill. + "use strict"; -var functionBodies; - +// 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. function findAllPoints(bodies, blockId, limits) { var points = []; @@ -32,6 +35,8 @@ function findAllPoints(bodies, blockId, limits) return points; } +// Given the CFG for the constructor call of some RAII, return whether the +// given edge is the matching destructor call. function isMatchingDestructor(constructor, edge) { if (edge.Kind != "Call") @@ -97,7 +102,7 @@ function allRAIIGuardedCallPoints(typeInfo, bodies, body, isConstructor) } // Test whether the given edge is the constructor corresponding to the given -// destructor edge +// destructor edge. function isMatchingConstructor(destructor, edge) { if (edge.Kind != "Call") @@ -129,7 +134,7 @@ function isMatchingConstructor(destructor, edge) return sameVariable(constructExp.Variable, destructExp.Variable); } -function findMatchingConstructor(destructorEdge, body) +function findMatchingConstructor(destructorEdge, body, warnIfNotFound=true) { var worklist = [destructorEdge]; var predecessors = getPredecessors(body); @@ -142,8 +147,9 @@ function findMatchingConstructor(destructorEdge, body) worklist.push(e); } } - printErr("Could not find matching constructor!"); - debugger; + if (warnIfNotFound) + printErr("Could not find matching constructor!"); + return undefined; } function pointsInRAIIScope(bodies, body, constructorEdge, limits) { diff --git a/js/src/devtools/rootAnalysis/analyzeRoots.js b/js/src/devtools/rootAnalysis/analyzeRoots.js index a3608faceef6..b417f709784d 100644 --- a/js/src/devtools/rootAnalysis/analyzeRoots.js +++ b/js/src/devtools/rootAnalysis/analyzeRoots.js @@ -516,8 +516,8 @@ function edgeCanGC(edge) var variable = callee.Variable; if (variable.Kind == "Func") { - var callee = mangled(variable.Name[0]); - if ((callee in gcFunctions) || ((callee + internalMarker) in gcFunctions)) + var func = mangled(variable.Name[0]); + if ((func in gcFunctions) || ((func + internalMarker) in gcFunctions)) return "'" + variable.Name[0] + "'"; return null; } diff --git a/js/src/devtools/rootAnalysis/annotations.js b/js/src/devtools/rootAnalysis/annotations.js index 7e9ccda1cd67..9b5b5950f4aa 100644 --- a/js/src/devtools/rootAnalysis/annotations.js +++ b/js/src/devtools/rootAnalysis/annotations.js @@ -255,6 +255,10 @@ var ignoreFunctions = { "uint8 nsContentUtils::IsExpandedPrincipal(nsIPrincipal*)" : true, "void mozilla::AutoProfilerLabel::~AutoProfilerLabel(int32)" : true, + + // Stores a function pointer in an AutoProfilerLabelData struct and calls it. + // And it's in mozglue, which doesn't have access to the attributes yet. + "void mozilla::ProfilerLabelEnd(mozilla::Tuple*)" : true, }; function extraGCFunctions() { @@ -319,6 +323,7 @@ function ignoreGCFunction(mangled) // tryNewTenuredThing, tryNewNurseryObject, and others. if (/refillFreeList|tryNew/.test(fun) && /\(js::AllowGC\)0u/.test(fun)) return true; + return false; } @@ -396,7 +401,7 @@ function isLimitConstructor(typeInfo, edgeType, varName) // nsISupports subclasses' methods may be scriptable (or overridden // via binary XPCOM), and so may GC. But some fields just aren't going // to get overridden with something that can GC. -function isOverridableField(initialCSU, csu, field) +function isOverridableField(staticCSU, csu, field) { if (csu != 'nsISupports') return false; @@ -421,19 +426,19 @@ function isOverridableField(initialCSU, csu, field) return false; if (field == "ConstructUbiNode") return false; - if (initialCSU == 'nsIXPCScriptable' && field == "GetScriptableFlags") + if (staticCSU == 'nsIXPCScriptable' && field == "GetScriptableFlags") return false; - if (initialCSU == 'nsIXPConnectJSObjectHolder' && field == 'GetJSObject') + if (staticCSU == 'nsIXPConnectJSObjectHolder' && field == 'GetJSObject') return false; - if (initialCSU == 'nsIXPConnect' && field == 'GetSafeJSContext') + if (staticCSU == 'nsIXPConnect' && field == 'GetSafeJSContext') return false; // nsIScriptSecurityManager is not [builtinclass], but smaug says "the // interface definitely should be builtinclass", which is good enough. - if (initialCSU == 'nsIScriptSecurityManager' && field == 'IsSystemPrincipal') + if (staticCSU == 'nsIScriptSecurityManager' && field == 'IsSystemPrincipal') return false; - if (initialCSU == 'nsIScriptContext') { + if (staticCSU == 'nsIScriptContext') { if (field == 'GetWindowProxy' || field == 'GetWindowProxyPreserveColor') return false; } diff --git a/js/src/devtools/rootAnalysis/callgraph.js b/js/src/devtools/rootAnalysis/callgraph.js index 721ff319afed..9b198791e0fb 100644 --- a/js/src/devtools/rootAnalysis/callgraph.js +++ b/js/src/devtools/rootAnalysis/callgraph.js @@ -6,10 +6,18 @@ loadRelativeToScript('utility.js'); loadRelativeToScript('annotations.js'); loadRelativeToScript('CFG.js'); -var subclasses = new Map(); // Map from csu => set of immediate subclasses -var superclasses = new Map(); // Map from csu => set of immediate superclasses -var classFunctions = new Map(); // Map from "csu:name" => set of full method name +// Map from csu => set of immediate subclasses +var subclasses = new Map(); +// Map from csu => set of immediate superclasses +var superclasses = new Map(); + +// Map from "csu.name:nargs" => set of full method name +var virtualDefinitions = new Map(); + +// Every virtual method declaration, anywhere. +// +// field : CFG of the field var virtualResolutionsSeen = new Set(); // map is a map from names to sets of entries. @@ -44,10 +52,11 @@ function processCSU(csuName, csu) addToNamedSet(subclasses, superclass, subclass); addToNamedSet(superclasses, subclass, superclass); } + if ("Variable" in field) { // Note: not dealing with overloading correctly. const name = field.Variable.Name[0]; - addToNamedSet(classFunctions, fieldKey(csuName, field.Field[0]), name); + addToNamedSet(virtualDefinitions, fieldKey(csuName, field.Field[0]), name); } } } @@ -58,8 +67,8 @@ function nearestAncestorMethods(csu, field) { const key = fieldKey(csu, field); - if (classFunctions.has(key)) - return new Set(classFunctions.get(key)); + if (virtualDefinitions.has(key)) + return new Set(virtualDefinitions.get(key)); const functions = new Set(); if (superclasses.has(csu)) { @@ -120,8 +129,8 @@ function findVirtualFunctions(initialCSU, field) const csu = worklist.pop(); const key = fieldKey(csu, field); - if (classFunctions.has(key)) - functions.update(classFunctions.get(key)); + if (virtualDefinitions.has(key)) + functions.update(virtualDefinitions.get(key)); if (subclasses.has(csu)) worklist.push(...subclasses.get(csu)); @@ -228,10 +237,10 @@ function loadTypesWithCache(type_xdb_filename, cache_filename) { const cacheData = deserialize(cb); subclasses = cacheData.subclasses; superclasses = cacheData.superclasses; - classFunctions = cacheData.classFunctions; + virtualDefinitions = cacheData.virtualDefinitions; } catch (e) { loadTypes(type_xdb_filename); - const cb = serialize({subclasses, superclasses, classFunctions}); + const cb = serialize({subclasses, superclasses, virtualDefinitions}); os.file.writeTypedArrayToFile(cache_filename, new Uint8Array(cb.arraybuffer)); } diff --git a/js/src/devtools/rootAnalysis/computeCallgraph.js b/js/src/devtools/rootAnalysis/computeCallgraph.js index 687391288f6f..a622d38e1a21 100644 --- a/js/src/devtools/rootAnalysis/computeCallgraph.js +++ b/js/src/devtools/rootAnalysis/computeCallgraph.js @@ -9,7 +9,7 @@ loadRelativeToScript('callgraph.js'); var theFunctionNameToFind; -if (scriptArgs[0] == '--function') { +if (scriptArgs[0] == '--function' || scriptArgs[0] == '-f') { theFunctionNameToFind = scriptArgs[1]; scriptArgs = scriptArgs.slice(2); } diff --git a/js/src/devtools/rootAnalysis/explain.py b/js/src/devtools/rootAnalysis/explain.py index b999ea2db5f1..373475e69d68 100755 --- a/js/src/devtools/rootAnalysis/explain.py +++ b/js/src/devtools/rootAnalysis/explain.py @@ -62,16 +62,14 @@ try: continue m = re.match( - r"^Function.*has unrooted.*of type.*live across GC call ('?)(.*?)('?) at (\S+):\d+$", line) # NOQA: E501 + r"^Function.*has unrooted.*of type.*live across GC call '(.*?)' at (\S+):\d+$", line) # NOQA: E501 if m: - # Function names are surrounded by single quotes. Field calls - # are unquoted. - current_gcFunction = m.group(2) + current_gcFunction = m.group(1) hazardousGCFunctions[current_gcFunction].append(line) hazardOrder.append((current_gcFunction, len(hazardousGCFunctions[current_gcFunction]) - 1)) num_hazards += 1 - fileOfFunction[current_gcFunction] = m.group(4) + fileOfFunction[current_gcFunction] = m.group(2) continue m = re.match(r'Function.*expected hazard.*but none were found', line) diff --git a/js/src/devtools/rootAnalysis/loadCallgraph.js b/js/src/devtools/rootAnalysis/loadCallgraph.js index ea584a731b67..cfe5ab6c58bd 100644 --- a/js/src/devtools/rootAnalysis/loadCallgraph.js +++ b/js/src/devtools/rootAnalysis/loadCallgraph.js @@ -8,7 +8,7 @@ loadRelativeToScript('utility.js'); -// Functions come out of sixgill in the form "mangled|readable". The mangled +// Functions come out of sixgill in the form "mangled$readable". The mangled // name is Truth. One mangled name might correspond to multiple readable names, // for multiple reasons, including (1) sixgill/gcc doesn't always qualify types // the same way or de-typedef the same amount; (2) sixgill's output treats @@ -22,8 +22,9 @@ loadRelativeToScript('utility.js'); // the beginning for these. // // The strategy used is to separate out the pieces whenever they are read in, -// create a table mapping mangled names to (one of the) readable names, and -// use the mangled names in all computation. +// create a table mapping mangled names to all readable names, and use the +// mangled names in all computation -- except for limited circumstances when +// the readable name is used in annotations. // // Note that callgraph.txt uses a compressed representation -- each name is // mapped to an integer, and those integers are what is recorded in the edges. @@ -279,7 +280,7 @@ function loadCallgraph(file) // context, and add them to the set of gcFunctions. while (worklist.length) { name = worklist.shift(); - assert(name in gcFunctions); + assert(name in gcFunctions, "gcFunctions does not contain " + name); if (!(name in callersOf)) continue; for (const {caller, limits} of callersOf[name]) { diff --git a/js/src/devtools/rootAnalysis/utility.js b/js/src/devtools/rootAnalysis/utility.js index 72aa413c855a..8df860facbfe 100644 --- a/js/src/devtools/rootAnalysis/utility.js +++ b/js/src/devtools/rootAnalysis/utility.js @@ -283,6 +283,7 @@ function addToKeyedList(collection, key, entry) if (!(key in collection)) collection[key] = []; collection[key].push(entry); + return collection[key]; } function loadTypeInfo(filename) diff --git a/taskcluster/scripts/builder/hazard-analysis.sh b/taskcluster/scripts/builder/hazard-analysis.sh index 5624600a4fa7..078a530e56aa 100755 --- a/taskcluster/scripts/builder/hazard-analysis.sh +++ b/taskcluster/scripts/builder/hazard-analysis.sh @@ -126,7 +126,7 @@ function grab_artifacts () { # Bundle up the less important but still useful intermediate outputs, # just to cut down on the clutter in treeherder's Job Details pane. - tar -acvf "${artifacts}/hazardIntermediates.tar.xz" --exclude-from <(for f in "${important[@]}"; do echo $f; done) *.txt *.lst build_xgill.log + tar -acvf "${artifacts}/hazardIntermediates.tar.xz" --exclude-from <(IFS=$'\n'; echo "${important[*]}") *.txt *.lst build_xgill.log # Upload the important outputs individually, so that they will be # visible in Job Details and accessible to automated jobs.