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

@@ -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) {

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;
         }

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<void*, unsigned int>*)" : 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;
     }

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));
     }

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);
 }

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)

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]) {

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)

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.