author | Steve Fink <sfink@mozilla.com> |
Fri, 28 Dec 2018 21:16:48 -0800 | |
changeset 458983 | a55571003fbd |
parent 458982 | 89fc662c01ee |
child 458984 | fd3f61ce9366 |
push id | 35553 |
push user | shindli@mozilla.com |
push date | Thu, 14 Feb 2019 04:41:18 +0000 |
treeherder | mozilla-central@f0ea53f47215 [default view] [failures only] |
perfherder | [talos] [build metrics] [platform microbench] (compared to previous push) |
reviewers | jonco |
bugs | 1500247 |
milestone | 67.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
|
--- 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)