Bug 1500247 - Add a mechanism to embed known hazards into test files and verify that they are caught, r=jonco
authorSteve Fink <sfink@mozilla.com>
Fri, 28 Dec 2018 21:13:32 -0800
changeset 458982 89fc662c01ee
parent 458981 3dd87564278e
child 458983 a55571003fbd
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 - Add a mechanism to embed known hazards into test files and verify that they are caught, r=jonco Use it to verify that MOZ_INHERIT_ATTRIBUTE_FROM_TEMPLATE_PARAM works.
js/public/GCAnnotations.h
js/src/devtools/rootAnalysis/analyzeRoots.js
js/src/devtools/rootAnalysis/explain.py
js/src/jsapi-tests/testGCExactRooting.cpp
js/src/jsapi-tests/tests.h
taskcluster/scripts/builder/hazard-analysis.sh
--- a/js/public/GCAnnotations.h
+++ b/js/public/GCAnnotations.h
@@ -6,16 +6,18 @@
 
 #ifndef js_GCAnnotations_h
 #define js_GCAnnotations_h
 
 // Set of annotations for the rooting hazard analysis, used to categorize types
 // and functions.
 #ifdef XGILL_PLUGIN
 
+#  define JS_EXPECT_HAZARDS __attribute__((annotate("Expect Hazards")))
+
 // Mark a type as being a GC thing (eg js::gc::Cell has this annotation).
 #  define JS_HAZ_GC_THING __attribute__((annotate("GC Thing")))
 
 // Mark a type as holding a pointer to a GC thing (eg JS::Value has this
 // annotation.) "Inherited" by templatized types with
 // MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS.
 #  define JS_HAZ_GC_POINTER __attribute__((annotate("GC Pointer")))
 
@@ -58,16 +60,17 @@
 
 // Mark a function as able to call JSNatives. Otherwise, JSNatives don't show
 // up in the callgraph. This doesn't matter for the can-GC analysis, but it is
 // very nice for other uses of the callgraph.
 #  define JS_HAZ_JSNATIVE_CALLER __attribute__((annotate("Calls JSNatives")))
 
 #else
 
+#  define JS_EXPECT_HAZARDS
 #  define JS_HAZ_GC_THING
 #  define JS_HAZ_GC_POINTER
 #  define JS_HAZ_ROOTED
 #  define JS_HAZ_GC_INVALIDATED
 #  define JS_HAZ_ROOTED_BASE
 #  define JS_HAZ_NON_GC_POINTER
 #  define JS_HAZ_GC_CALL
 #  define JS_HAZ_GC_SUPPRESSED
--- a/js/src/devtools/rootAnalysis/analyzeRoots.js
+++ b/js/src/devtools/rootAnalysis/analyzeRoots.js
@@ -785,16 +785,31 @@ function typeDesc(type)
     }
 }
 
 function processBodies(functionName)
 {
     if (!("DefineVariable" in functionBodies[0]))
         return;
     var suppressed = Boolean(limitedFunctions[mangled(functionName)] & LIMIT_CANNOT_GC);
+
+    // Look for the JS_EXPECT_HAZARDS annotation, and output a different
+    // message in that case that won't be counted as a hazard.
+    var annotations = new Set();
+    for (const variable of functionBodies[0].DefineVariable) {
+        if (variable.Variable.Kind == "Func" && variable.Variable.Name[0] == functionName) {
+            for (const { Name: [tag, value] } of (variable.Type.Annotation || [])) {
+                if (tag == 'annotate')
+                    annotations.add(value);
+            }
+        }
+    }
+
+    var missingExpectedHazard = annotations.has("Expect Hazards");
+
     for (var variable of functionBodies[0].DefineVariable) {
         var name;
         if (variable.Variable.Kind == "This")
             name = "this";
         else if (variable.Variable.Kind == "Return")
             name = "<returnvalue>";
         else
             name = variable.Variable.Name[0];
@@ -812,33 +827,56 @@ function processBodies(functionName)
                 }
                 print("\nFunction '" + functionName + "'" +
                       " has unnecessary root '" + name + "' at " + lineText);
             }
         } else if (isUnrootedType(variable.Type)) {
             var result = variableLiveAcrossGC(suppressed, variable.Variable);
             if (result) {
                 var lineText = findLocation(result.gcInfo.body, result.gcInfo.ppoint);
-                print("\nFunction '" + functionName + "'" +
-                      " has unrooted '" + name + "'" +
-                      " of type '" + typeDesc(variable.Type) + "'" +
-                      " live across GC call " + result.gcInfo.name +
-                      " at " + lineText);
+                if (annotations.has('Expect Hazards')) {
+                    print("\nThis is expected, but '" + functionName + "'" +
+                          " has unrooted '" + name + "'" +
+                          " of type '" + typeDesc(variable.Type) + "'" +
+                          " live across GC call " + result.gcInfo.name +
+                          " at " + lineText);
+                    missingExpectedHazard = false;
+                } else {
+                    print("\nFunction '" + functionName + "'" +
+                          " has unrooted '" + name + "'" +
+                          " of type '" + typeDesc(variable.Type) + "'" +
+                          " live across GC call " + result.gcInfo.name +
+                          " at " + lineText);
+                }
                 printEntryTrace(functionName, result);
             }
             result = unsafeVariableAddressTaken(suppressed, variable.Variable);
             if (result) {
                 var lineText = findLocation(result.body, result.ppoint);
                 print("\nFunction '" + functionName + "'" +
                       " takes unsafe address of unrooted '" + name + "'" +
                       " at " + lineText);
                 printEntryTrace(functionName, {body:result.body, ppoint:result.ppoint});
             }
         }
     }
+
+    if (missingExpectedHazard) {
+        const {
+            Location: [
+                { CacheString: startfile, Line: startline },
+                { CacheString: endfile, Line: endline }
+            ]
+        } = functionBodies[0];
+
+        const loc = (startfile == endfile) ? `${startfile}:${startline}-${endline}`
+              : `${startfile}:${startline}`;
+
+        print("\nFunction '" + functionName + "' expected hazard(s) but none were found at " + loc);
+    }
 }
 
 if (batch == 1)
     print("Time: " + new Date);
 
 var xdb = xdbLibrary();
 xdb.open("src_body.xdb");
 
--- a/js/src/devtools/rootAnalysis/explain.py
+++ b/js/src/devtools/rootAnalysis/explain.py
@@ -12,31 +12,36 @@ parser.add_argument('rootingHazards', na
 parser.add_argument('gcFunctions', nargs='?', default='gcFunctions.txt')
 parser.add_argument('hazards', nargs='?', default='hazards.txt')
 parser.add_argument('extra', nargs='?', default='unnecessary.txt')
 parser.add_argument('refs', nargs='?', default='refs.txt')
 args = parser.parse_args()
 
 num_hazards = 0
 num_refs = 0
+num_missing = 0
+
 try:
     with open(args.rootingHazards) as rootingHazards, \
             open(args.hazards, 'w') as hazards, \
             open(args.extra, 'w') as extra, \
             open(args.refs, 'w') as refs:
         current_gcFunction = None
 
         # Map from a GC function name to the list of hazards resulting from
         # that GC function
         hazardousGCFunctions = defaultdict(list)
 
         # List of tuples (gcFunction, index of hazard) used to maintain the
         # ordering of the hazards
         hazardOrder = []
 
+        # Map from a hazardous GC function to the filename containing it.
+        fileOfFunction = {}
+
         for line in rootingHazards:
             m = re.match(r'^Time: (.*)', line)
             mm = re.match(r'^Run on:', line)
             if m or mm:
                 print(line, file=hazards)
                 print(line, file=extra)
                 print(line, file=refs)
                 continue
@@ -48,25 +53,32 @@ try:
 
             m = re.match(r'^Function.*takes unsafe address of unrooted', line)
             if m:
                 num_refs += 1
                 print(line, file=refs)
                 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)
                 hazardousGCFunctions[current_gcFunction].append(line)
                 hazardOrder.append((current_gcFunction,
                                     len(hazardousGCFunctions[current_gcFunction]) - 1))
                 num_hazards += 1
+                fileOfFunction[current_gcFunction] = m.group(4)
+                continue
+
+            m = re.match(r'Function.*expected hazard.*but none were found', line)
+            if m:
+                num_missing += 1
+                print(line + "\n", file=hazards)
                 continue
 
             if current_gcFunction:
                 if not line.strip():
                     # Blank line => end of this hazard
                     current_gcFunction = None
                 else:
                     hazardousGCFunctions[current_gcFunction][-1] += line
@@ -99,9 +111,9 @@ try:
                 print(gcHazards[index], file=hazards)
 
 except IOError as e:
     print('Failed: %s' % str(e))
 
 print("Wrote %s" % args.hazards)
 print("Wrote %s" % args.extra)
 print("Wrote %s" % args.refs)
-print("Found %d hazards and %d unsafe references" % (num_hazards, num_refs))
+print("Found %d hazards %d unsafe references %d missing" % (num_hazards, num_refs, num_missing))
--- a/js/src/jsapi-tests/testGCExactRooting.cpp
+++ b/js/src/jsapi-tests/testGCExactRooting.cpp
@@ -159,16 +159,55 @@ BEGIN_TEST(testGCRootedHashMap) {
     RootedObject obj(cx, r.front().value());
     CHECK(obj->as<NativeObject>().lastProperty() == r.front().key());
   }
 
   return true;
 }
 END_TEST(testGCRootedHashMap)
 
+// Repeat of the test above, but without rooting. This is a rooting hazard. The
+// JS_EXPECT_HAZARDS annotation will cause the hazard taskcluster job to fail
+// if the hazard below is *not* detected.
+BEGIN_TEST_WITH_ATTRIBUTES(testUnrootedGCHashMap,JS_EXPECT_HAZARDS) {
+  MyHashMap map(cx, 15);
+
+  for (size_t i = 0; i < 10; ++i) {
+    RootedObject obj(cx, JS_NewObject(cx, nullptr));
+    RootedValue val(cx, UndefinedValue());
+    // Construct a unique property name to ensure that the object creates a
+    // new shape.
+    char buffer[2];
+    buffer[0] = 'a' + i;
+    buffer[1] = '\0';
+    CHECK(JS_SetProperty(cx, obj, buffer, val));
+    CHECK(map.putNew(obj->as<NativeObject>().lastProperty(), obj));
+  }
+
+  JS_GC(cx);
+
+  // Access map to keep it live across the GC.
+  CHECK(map.count() == 10);
+
+  return true;
+}
+END_TEST(testUnrootedGCHashMap)
+
+BEGIN_TEST(testSafelyUnrootedGCHashMap) {
+  // This is not rooted, but it doesn't use GC pointers as keys or values so
+  // it's ok.
+  js::GCHashMap<uint64_t, uint64_t> map(cx, 15);
+
+  JS_GC(cx);
+  CHECK(map.putNew(12, 13));
+
+  return true;
+}
+END_TEST(testSafelyUnrootedGCHashMap)
+
 static bool FillMyHashMap(JSContext* cx, MutableHandle<MyHashMap> map) {
   for (size_t i = 0; i < 10; ++i) {
     RootedObject obj(cx, JS_NewObject(cx, nullptr));
     RootedValue val(cx, UndefinedValue());
     // Construct a unique property name to ensure that the object creates a
     // new shape.
     char buffer[2];
     buffer[0] = 'a' + i;
--- a/js/src/jsapi-tests/tests.h
+++ b/js/src/jsapi-tests/tests.h
@@ -348,21 +348,23 @@ class JSAPITest {
             (unsigned int)report->lineno, report->message().c_str());
   }
 
   virtual const JSClass* getGlobalClass() { return basicGlobalClass(); }
 
   virtual JSObject* createGlobal(JSPrincipals* principals = nullptr);
 };
 
-#define BEGIN_TEST(testname)                                  \
+#define BEGIN_TEST_WITH_ATTRIBUTES(testname, attrs)           \
   class cls_##testname : public JSAPITest {                   \
    public:                                                    \
     virtual const char* name() override { return #testname; } \
-    virtual bool run(JS::HandleObject global) override
+    virtual bool run(JS::HandleObject global) override attrs
+
+#define BEGIN_TEST(testname) BEGIN_TEST_WITH_ATTRIBUTES(testname, )
 
 #define END_TEST(testname) \
   }                        \
   ;                        \
   static cls_##testname cls_##testname##_instance;
 
 /*
  * A "fixture" is a subclass of JSAPITest that holds common definitions for a
--- a/taskcluster/scripts/builder/hazard-analysis.sh
+++ b/taskcluster/scripts/builder/hazard-analysis.sh
@@ -154,35 +154,42 @@ function grab_artifacts () {
 function check_hazards () {
     (
     set +e
     NUM_HAZARDS=$(grep -c 'Function.*has unrooted.*live across GC call' "$1"/rootingHazards.txt)
     NUM_UNSAFE=$(grep -c '^Function.*takes unsafe address of unrooted' "$1"/refs.txt)
     NUM_UNNECESSARY=$(grep -c '^Function.* has unnecessary root' "$1"/unnecessary.txt)
     NUM_DROPPED=$(grep -c '^Dropped CFG' "$1"/build_xgill.log)
     NUM_WRITE_HAZARDS=$(perl -lne 'print $1 if m!found (\d+)/\d+ allowed errors!' "$1"/heapWriteHazards.txt)
+    NUM_MISSING=$(grep -c '^Function.*expected hazard.*but none were found' "$1"/rootingHazards.txt)
 
     set +x
     echo "TinderboxPrint: rooting hazards<br/>$NUM_HAZARDS"
     echo "TinderboxPrint: (unsafe references to unrooted GC pointers)<br/>$NUM_UNSAFE"
     echo "TinderboxPrint: (unnecessary roots)<br/>$NUM_UNNECESSARY"
+    echo "TinderboxPrint: missing expected hazards<br/>$NUM_MISSING"
     echo "TinderboxPrint: heap write hazards<br/>$NUM_WRITE_HAZARDS"
 
     # Display errors in a way that will get picked up by the taskcluster scraper.
-    perl -le 'print "TEST-UNEXPECTED-FAIL | hazards | $ENV{NUM_HAZARDS} rooting hazards" if $ENV{NUM_HAZARDS}'
     perl -lne 'print "TEST-UNEXPECTED-FAIL | hazards | $1 $2" if /^Function.* has (unrooted .*live across GC call).* (at .*)$/' "$1"/hazards.txt
 
     exit_status=0
 
     if [ $NUM_HAZARDS -gt 0 ]; then
         echo "TEST-UNEXPECTED-FAIL | hazards | $NUM_HAZARDS rooting hazards detected" >&2
         echo "TinderboxPrint: documentation<br/><a href='https://wiki.mozilla.org/Javascript:Hazard_Builds#Diagnosing_a_rooting_hazards_failure'>static rooting hazard analysis failures</a>, visit \"Inspect Task\" link for hazard details"
         exit_status=1
     fi
 
+    if [ $NUM_MISSING -gt 0 ]; then
+        echo "TEST-UNEXPECTED-FAIL | hazards | $NUM_MISSING expected hazards went undetected" >&2
+        echo "TinderboxPrint: documentation<br/><a href='https://wiki.mozilla.org/Javascript:Hazard_Builds#Diagnosing_a_rooting_hazards_failure'>static rooting hazard analysis failures</a>, visit \"Inspect Task\" link for hazard details"
+        exit_status=1
+    fi
+
     NUM_ALLOWED_WRITE_HAZARDS=0
     if [ $NUM_WRITE_HAZARDS -gt $NUM_ALLOWED_WRITE_HAZARDS ]; then
         echo "TEST-UNEXPECTED-FAIL | heap-write-hazards | $NUM_WRITE_HAZARDS heap write hazards detected out of $NUM_ALLOWED_WRITE_HAZARDS allowed" >&2
         echo "TinderboxPrint: documentation<br/><a href='https://wiki.mozilla.org/Javascript:Hazard_Builds#Diagnosing_a_heap_write_hazard_failure'>heap write hazard analysis failures</a>, visit \"Inspect Task\" link for hazard details"
         exit_status = 1
     fi
 
     if [ $NUM_DROPPED -gt 0 ]; then