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 516866 fd3f61ce9366b8e616f6bf4d27f1e679b11d4749
parent 516865 a55571003fbde93cb82e5ff6eeadb954ea1b7a8f
child 516867 f0e97add682c71d36dbe895efad729307638f14f
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [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)