Bug 1500247 - [hazards] End the live range of a variable passed to a move constructor via std::move(), r=jonco
authorSteve Fink <sfink@mozilla.com>
Wed, 02 Jan 2019 16:59:39 -0800
changeset 458984 fd3f61ce9366
parent 458983 a55571003fbd
child 458985 f0e97add682c
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] End the live range of a variable passed to a move constructor via std::move(), r=jonco { extern void foo(UniquePtr<T> u); UniquePtr<T> uptr = MakeUnique<T>(); foo(std::move(uptr)); gc(); } should NOT be a hazard; uptr goes through the move constructor and transfers ownership of the pointer. This is kind of complicated because what actually happens is that std::move() returns a temporary that gets passed to the move constructor, so we have to handle ending the live ranges of both the original UniquePtr as well as the temporary.
js/src/devtools/rootAnalysis/analyzeRoots.js
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
@@ -336,49 +336,122 @@ function edgeKillsVariable(edge, variabl
 
         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;
+            // edgeKillsVariable 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 (!edgeKillsVariable(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<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);
 //
+// Yet another example is a UniquePtr being passed by value, which means the
+// receiver takes ownership:
+//
+//     UniquePtr<JSObject*> uobj(obj);
+//     foo(uobj);
+//     gc();
+//
 // 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.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 &&
         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)
@@ -397,16 +470,34 @@ function edgeInvalidatesVariable(edge, v
         // 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);
 
+    // special-case: passing UniquePtr<T> 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")
         return false;
 
@@ -549,17 +640,17 @@ 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)) {
+            if (edgeInvalidatesVariable(edge, variable, body)) {
                 // 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);
@@ -688,17 +779,17 @@ function variableLiveAcrossGC(suppressed
             //
             // 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))
+            if (edgeInvalidatesVariable(edge, variable, body))
                 continue;
 
             var usePoint = edgeUsesVariable(edge, variable, body);
             if (usePoint) {
                 var call = findGCBeforeValueUse(body, usePoint, suppressed, variable);
                 if (!call)
                     continue;
 
--- a/js/src/devtools/rootAnalysis/t/hazards/source.cpp
+++ b/js/src/devtools/rootAnalysis/t/hazards/source.cpp
@@ -1,8 +1,10 @@
+#include <utility>
+
 #define ANNOTATE(property) __attribute__((annotate(property)))
 
 struct Cell {
   int f;
 } ANNOTATE("GC Thing");
 
 template <typename T, typename U>
 struct UntypedContainer {
@@ -218,16 +220,18 @@ class UniquePtr
   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
 
+extern void consume(mozilla::UniquePtr<Cell> uptr);
+
 void safevals() {
   Cell cell;
 
   // Simple hazard.
   Cell* unsafe1 = &cell;
   GC();
   use(unsafe1);
 
@@ -261,14 +265,37 @@ void safevals() {
 
   // reset() to safe value after the GC.
   {
     mozilla::UniquePtr<Cell> safe6(&cell);
     GC();
     safe6.reset();
   }
 
+  // reset() to safe value after the GC -- but we've already used it, so it's
+  // too late.
+  {
+    mozilla::UniquePtr<Cell> unsafe7(&cell);
+    GC();
+    use(unsafe7.get());
+    unsafe7.reset();
+  }
+
   // initialized to safe value.
   {
-    mozilla::UniquePtr<Cell> safe7;
+    mozilla::UniquePtr<Cell> safe8;
     GC();
   }
+
+  // passed to a function that takes ownership before GC.
+  {
+    mozilla::UniquePtr<Cell> safe9(&cell);
+    consume(std::move(safe9));
+    GC();
+  }
+
+  // passed to a function that takes ownership after GC.
+  {
+    mozilla::UniquePtr<Cell> unsafe10(&cell);
+    GC();
+    consume(std::move(unsafe10));
+  }
 }
--- a/js/src/devtools/rootAnalysis/t/hazards/test.py
+++ b/js/src/devtools/rootAnalysis/t/hazards/test.py
@@ -54,9 +54,12 @@ 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)
+assert('unsafe7' in hazmap)
+assert('safe8' not in hazmap)
+assert('safe9' not in hazmap)
+assert('safe10' not in hazmap)