Bug 1500247 - [hazards] Do not report hazards when a variable is known to hold a safe value, r=jonco
authorSteve Fink <sfink@mozilla.com>
Fri, 28 Dec 2018 21:16:48 -0800
changeset 458983 a55571003fbd
parent 458982 89fc662c01ee
child 458984 fd3f61ce9366
push id35553
push usershindli@mozilla.com
push dateThu, 14 Feb 2019 04:41:18 +0000
treeherdermozilla-central@f0ea53f47215 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjonco
bugs1500247
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1500247 - [hazards] Do not report hazards when a variable is known to hold a safe value, r=jonco
js/src/devtools/rootAnalysis/analyzeRoots.js
js/src/devtools/rootAnalysis/run-test.py
js/src/devtools/rootAnalysis/t/exceptions/source.cpp
js/src/devtools/rootAnalysis/t/hazards/source.cpp
js/src/devtools/rootAnalysis/t/hazards/test.py
--- a/js/src/devtools/rootAnalysis/analyzeRoots.js
+++ b/js/src/devtools/rootAnalysis/analyzeRoots.js
@@ -121,24 +121,30 @@ function expressionUsesVariableContents(
                 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 (edge.Exp[1].Kind == "Int" && edge.Exp[1].String == "0") {
+            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
@@ -261,29 +267,30 @@ function edgeTakesVariableAddress(edge, 
     }
 }
 
 function expressionIsVariable(exp, variable)
 {
     return exp.Kind == "Var" && sameVariable(exp.Variable, variable);
 }
 
-// Return whether the edge kills (overwrites) the variable's incoming value.
-// Examples of killing 'obj':
+// Return whether the edge terminates the live range of a variable's value when
+// searching in reverse through the CFG, by setting it to some new value.
+// Examples of killing 'obj's live range:
 //
 //     obj = foo;
 //     obj = foo();
-//     obj = foo(obj);         // uses previous value but then kills it
+//     obj = foo(obj);         // uses previous value but then sets to new value
 //     SomeClass obj(true, 1); // constructor
 //
 function edgeKillsVariable(edge, variable)
 {
     // Direct assignments kill their lhs: var = value
     if (edge.Kind == "Assign") {
-        const [lhs] = edge.Exp;
+        const [lhs, rhs] = edge.Exp;
         return (expressionIsVariable(lhs, variable) &&
                 !isReturningImmobileValue(edge, variable));
     }
 
     if (edge.Kind != "Call")
         return false;
 
     // Assignments of call results kill their lhs.
@@ -329,16 +336,80 @@ function edgeKillsVariable(edge, variabl
 
         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<T> that gets reset:
+//
+//     Maybe<AutoCheckCannotGC> nogc;
+//     nogc.emplace(cx);
+//     nogc.reset();
+//     gc();             // <-- not a problem; nogc is invalidated by prev line
+//     nogc.emplace(cx);
+//     foo(nogc);
+//
+// Compare to edgeKillsVariable: killing (in backwards direction) means the
+// variable's value was live and is no longer. Invalidating means it wasn't
+// actually live after all.
+function edgeInvalidatesVariable(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.Type.TypeFunctionCSU &&
+        edge.PEdgeCallInstance &&
+        edge.PEdgeCallInstance.Exp.Kind == 'Var' &&
+        expressionIsVariable(edge.PEdgeCallInstance.Exp, variable))
+    do {
+        const typeName = edge.Type.TypeFunctionCSU.Type.Name;
+        const m = typeName.match(/^mozilla::(\w+)</);
+        if (!m)
+            break;
+        const type = m[1];
+        if (!["Maybe", "UniquePtr"].includes(type))
+            break;
+
+        // special-case: (type)::reset()
+        if (callee.Kind == 'Var' &&
+            callee.Variable.Name[1] == 'reset')
+        {
+            return true;
+        }
+
+        // special-case: the initial constructor that doesn't provide a value.
+        if (callee.Kind == 'Var' &&
+            callee.Variable.Name[0].includes(`mozilla::${type}<T>::${type}()`))
+        {
+            return true;
+        }
+    } while(0);
+
+    return false;
+}
+
 function edgeCanGC(edge)
 {
     if (edge.Kind != "Call")
         return false;
 
     var callee = edge.Exp[0];
 
     while (callee.Kind == "Drf")
@@ -478,16 +549,23 @@ function findGCBeforeValueUse(start_body
 
         var predecessors = getPredecessors(body);
         if (!(ppoint in predecessors))
             continue;
 
         for (var edge of predecessors[ppoint]) {
             var source = edge.Index[0];
 
+            if (edgeInvalidatesVariable(edge, variable)) {
+                // 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;
+            }
+
             var edge_kills = edgeKillsVariable(edge, variable);
             var edge_uses = edgeUsesVariable(edge, variable, body);
 
             if (edge_kills || edge_uses) {
                 if (!body.minimumUse || source < body.minimumUse)
                     body.minimumUse = source;
             }
 
@@ -609,16 +687,20 @@ function variableLiveAcrossGC(suppressed
             //   obj = CopyObject(obj);
             //
             // This is not a hazard, because even though CopyObject can GC, obj
             // is not live across it. (obj is live before CopyObject, and
             // probably after, but not across.) There may be a hazard within
             // CopyObject, of course.
             //
 
+            // Ignore uses that are just invalidating the previous value.
+            if (edgeInvalidatesVariable(edge, variable))
+                continue;
+
             var usePoint = edgeUsesVariable(edge, variable, body);
             if (usePoint) {
                 var call = findGCBeforeValueUse(body, usePoint, suppressed, variable);
                 if (!call)
                     continue;
 
                 call.afterGCUse = usePoint;
                 return call;
old mode 100644
new mode 100755
--- a/js/src/devtools/rootAnalysis/run-test.py
+++ b/js/src/devtools/rootAnalysis/run-test.py
@@ -104,13 +104,13 @@ for name in cfg.tests:
         print("Running test %s" % name)
     testpath = os.path.join(indir, "test.py")
     testscript = open(testpath).read()
     testcode = compile(testscript, testpath, 'exec')
     try:
         exec(testcode, {'test': test, 'equal': equal})
     except subprocess.CalledProcessError:
         print("TEST-FAILED: %s" % name)
-    except StandardError:
+    except AssertionError:
         print("TEST-FAILED: %s" % name)
         raise
     else:
         print("TEST-PASSED: %s" % name)
--- a/js/src/devtools/rootAnalysis/t/exceptions/source.cpp
+++ b/js/src/devtools/rootAnalysis/t/exceptions/source.cpp
@@ -27,20 +27,22 @@ class AutoSomething {
 
  public:
   AutoSomething() : gc() {
     asm("");  // Ooh, scary, this might throw an exception
   }
   ~AutoSomething() { asm(""); }
 };
 
+extern Cell* getcell();
+
 extern void usevar(Cell* cell);
 
 void f() {
-  Cell* thing = nullptr;  // Live range starts here
+  Cell* thing = getcell();  // Live range starts here
 
   // When compiling with -fexceptions, there should be a hazard below. With
   // -fno-exceptions, there should not be one. We will check both.
   {
     AutoSomething smth;  // Constructor can GC only if exceptions are enabled
     usevar(thing);       // Live range ends here
   }  // In particular, 'thing' is dead at the destructor, so no hazard
 }
--- a/js/src/devtools/rootAnalysis/t/hazards/source.cpp
+++ b/js/src/devtools/rootAnalysis/t/hazards/source.cpp
@@ -202,8 +202,73 @@ void loopy() {
   // CellContainer;
   CellContainer haz8;
   for (int i6 = 0; i6 < 10; i6++) {
     GC();
     use(haz8.cell);
     haz8.cell = &cell;
   }
 }
+
+namespace mozilla {
+template<typename T>
+class UniquePtr
+{
+  T* val;
+ public:
+  UniquePtr() : val(nullptr) { asm(""); }
+  UniquePtr(T* p) : val(p) {}
+  UniquePtr(UniquePtr<T>&& u) : val(u.val) { u.val = nullptr; }
+  ~UniquePtr() { use(val); }
+  T* get() { return val; }
+  void reset() { val = nullptr; }
+} ANNOTATE("moz_inherit_type_annotations_from_template_args");
+} // namespace mozilla
+
+void safevals() {
+  Cell cell;
+
+  // Simple hazard.
+  Cell* unsafe1 = &cell;
+  GC();
+  use(unsafe1);
+
+  // Safe because it's known to be nullptr.
+  Cell* safe2 = &cell;
+  safe2 = nullptr;
+  GC();
+  use(safe2);
+
+  // Unsafe because it may not be nullptr.
+  Cell* unsafe3 = &cell;
+  if (reinterpret_cast<long>(&cell) & 0x100) {
+    unsafe3 = nullptr;
+  }
+  GC();
+  use(unsafe3);
+
+  // Hazard involving UniquePtr.
+  {
+    mozilla::UniquePtr<Cell> unsafe4(&cell);
+    GC();
+    // Destructor uses unsafe4.
+  }
+
+  // reset() to safe value before the GC.
+  {
+    mozilla::UniquePtr<Cell> safe5(&cell);
+    safe5.reset();
+    GC();
+  }
+
+  // reset() to safe value after the GC.
+  {
+    mozilla::UniquePtr<Cell> safe6(&cell);
+    GC();
+    safe6.reset();
+  }
+
+  // initialized to safe value.
+  {
+    mozilla::UniquePtr<Cell> safe7;
+    GC();
+  }
+}
--- a/js/src/devtools/rootAnalysis/t/hazards/test.py
+++ b/js/src/devtools/rootAnalysis/t/hazards/test.py
@@ -17,20 +17,20 @@ hazmap = {haz.variable: haz for haz in h
 assert('cell1' not in hazmap)
 assert('cell2' in hazmap)
 assert('cell3' in hazmap)
 assert('cell4' not in hazmap)
 assert('cell5' not in hazmap)
 assert('cell6' not in hazmap)
 assert('<returnvalue>' in hazmap)
 
-# All hazards should be in f() and loopy()
+# All hazards should be in f(), loopy(), and safevals()
 assert(hazmap['cell2'].function == 'Cell* f()')
 print(len(set(haz.function for haz in hazards)))
-assert(len(set(haz.function for haz in hazards)) == 2)
+assert(len(set(haz.function for haz in hazards)) == 3)
 
 # 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
 # reported.)
 assert(hazmap['cell2'].GCFunction == 'void halfSuppressedFunction()')
 assert(hazmap['cell3'].GCFunction in (
     'void halfSuppressedFunction()', 'void unsuppressedFunction()'))
 assert(hazmap['<returnvalue>'].GCFunction == 'void GCInDestructor::~GCInDestructor()')
@@ -46,8 +46,17 @@ assert(hazmap['<returnvalue>'].type == '
 assert('haz1' not in hazmap)
 assert('haz2' not in hazmap)
 assert('haz3' in hazmap)
 assert('haz4' in hazmap)
 assert('haz5' in hazmap)
 assert('haz6' not in hazmap)
 assert('haz7' not in hazmap)
 assert('haz8' in hazmap)
+
+# safevals hazards. See comments in source.
+assert('unsafe1' in hazmap)
+assert('safe2' not in hazmap)
+assert('unsafe3' in hazmap)
+assert('unsafe4' in hazmap)
+assert('safe5' not in hazmap)
+assert('safe6' not in hazmap)
+assert('safe7' not in hazmap)