Bug 425454 - whitelist placement-new for stack classes. Also uses treehydra analysis so we don't have to require a constructor for stack classes any more. r=taras

config/config.mk

@@ -505,11 +505,12 @@ endif
 DEHYDRA_SCRIPT = $(topsrcdir)/xpcom/analysis/static-checking.js
 
 DEHYDRA_MODULES = \
-  $(topsrcdir)/xpcom/analysis/stack.js \
+  $(topsrcdir)/xpcom/analysis/final.js \
   $(NULL)
 
 TREEHYDRA_MODULES = \
   $(topsrcdir)/xpcom/analysis/outparams.js \
+  $(topsrcdir)/xpcom/analysis/stack.js \
   $(NULL)
 
 DEHYDRA_ARGS = \

xpcom/analysis/final.js

@@ -0,0 +1,16 @@
+function process_type(c)
+{
+  if ((c.kind == 'class' || c.kind == 'struct') && !c.isIncomplete) {
+    for each (let base in c.bases)
+      if (isFinal(base))
+	error("Class '" + c.name + "' derives from final class '" + base.name + "'.", c.loc);
+  }
+}
+
+function isFinal(c)
+{
+  if (c.isIncomplete)
+    throw Error("Can't get final property for incomplete type.");
+
+  return hasAttribute(c, 'NS_final');
+}

xpcom/analysis/stack.js

@@ -1,26 +1,178 @@
-function process_function(f, stmts)
-{
-  var stmt;
-  function getLocation()
-  {
-    if (stmt.loc)
-      return stmt.loc;
+include("gcc_util.js");
+include("unstable/lazy_types.js");
 
-    return f.loc;
+function process_type(c)
+{
+  if ((c.kind == 'class' || c.kind == 'struct') &&
+      !c.isIncomplete)
+    isStack(c);
+}
+
+function isStack(c)
+{
+  function calculate()
+  {
+    if (hasAttribute(c, 'NS_stack'))
+      return true;
+
+    for each (let base in c.bases)
+    if (isStack(base))
+      return true;
+
+    for each (let member in c.members) {
+      if (member.isFunction)
+	continue;
+
+      let type = member.type;
+      while (true) {
+	if (type === undefined)
+	  break;
+
+	if (type.isArray) {
+	  type = type.type;
+	  continue;
+	}
+
+	if (type.typedef) {
+	  if (hasAttribute(type, 'NS_stack'))
+	    return true;
+
+	  type = type.typedef;
+	  continue;
+	}
+	break;
+      }
+
+      if (type === undefined) {
+	warning("incomplete type for member " + member + ".", member.loc);
+	continue;
+      }
+
+      if (type.isPointer || type.isReference)
+	continue;
+
+      if (!type.kind || (type.kind != 'class' && type.kind != 'struct'))
+	continue;
+
+      if (isStack(type))
+	return true;
+    }
+    return false;
   }
 
-  function processVar(v)
-  {
-    if (v.isConstructor &&
-        v.fieldOf &&
-        get_class(v.memberOf, false).stack &&
-        v.fieldOf.type.isPointer) {
-      error(getLocation() + ": constructed object of type '" +
-            v.memberOf.name + "' not on the stack.");
+  if (c.isIncomplete)
+    throw Error("Can't get stack property for incomplete type.");
+
+  if (!c.hasOwnProperty('isStack'))
+    c.isStack = calculate();
+
+  return c.isStack;
+}
+
+function isVoidPtr(t)
+{
+  return t.isPointer && t.type.name == 'void';
+}
+
+/**
+ * Detect a call to operator new. If this is operator new, and not
+ * placement-new, return the VAR_DECL of the temporary variable that operator
+ * new is always assigned to.
+ */
+function operator_new_assign(stmt)
+{
+  try {
+    stmt.tree_check(GIMPLE_MODIFY_STMT);
+    let [varDecl, callExpr] = stmt.operands();
+    varDecl.tree_check(VAR_DECL);
+    callExpr.tree_check(CALL_EXPR);
+    
+    let fncall = callable_arg_function_decl(CALL_EXPR_FN(callExpr)).
+                   tree_check(FUNCTION_DECL);
+
+    let nameid = DECL_NAME(fncall);
+    if (IDENTIFIER_OPNAME_P(nameid)) {
+      let name = IDENTIFIER_POINTER(nameid);
+      if (name == "operator new" || name == "operator new []") {
+        // if this is placement-new, ignore it (should we issue a warning?)
+        let fncallobj = dehydra_convert(TREE_TYPE(fncall));
+        if (fncallobj.parameters.length == 2 &&
+            isVoidPtr(fncallobj.parameters[1]))
+	  return null;
+
+        return varDecl;
+      }
     }
   }
+  catch (e if e.TreeCheckError) { }
 
-  for each (stmt in stmts) {
-    iter(processVar, stmt.statements);
-  }
+  return null;
+}
+
+function process_tree(fndecl)
+{
+  function findconstructors(t, stack)
+  {
+    function getLocation(t) {
+      let loc;
+      if (t) {
+        loc = location_of(t);
+        if (loc !== undefined)
+          return loc;
+      }
+
+      for (let i = stack.length - 1; i >= 0; --i) {
+        let loc = location_of(stack[i]);
+        if (loc !== undefined)
+          return loc;
+      }
+      return location_of(DECL_SAVED_TREE(fndecl));
+    }
+    
+    function check_opnew_assignment(varDecl, stmt)
+    {
+      if (TREE_CODE(stmt) != GIMPLE_MODIFY_STMT) {
+        warning("operator new not followed by a GIMPLE_MODIFY_STMT: " + TREE_CODE(stmt), getLocation(stmt));
+        return;
+      }
+
+      let [destVar, assign] = stmt.operands();
+      if (TREE_CODE(assign) == NOP_EXPR)
+        assign = assign.operands()[0];
+
+      if (assign != varDecl) {
+        warning("operator new not followed by an known assignment pattern", getLocation(stmt));
+        return;
+      }
+
+      let destType = dehydra_convert(TREE_TYPE(destVar));
+      if (!destType.isPointer && !destType.isReference) {
+        error("operator new not assigned to pointer/ref?", getLocation(stmt));
+        return;
+      }
+      destType = destType.type;
+
+      if (isStack(destType))
+        error("constructed object of type '" + destType.name + "' not on the stack.", getLocation(stmt));
+    }
+
+    if (TREE_CODE(t) != STATEMENT_LIST)
+      return;
+
+    // if we find a tuple of "operator new" / GIMPLE_MODIFY_STMT casting
+    // the result of operator new to a pointer type
+    let opnew = null;
+    for (let stmt in iter_statement_list(t)) {
+      if (opnew != null)
+	check_opnew_assignment(opnew, stmt);
+
+      opnew = operator_new_assign(stmt);
+    }
+
+    if (opnew != null)
+      warning("operator new not followed by an assignment", getLocation());
+  }
+  
+  let tmap = new Map();
+  walk_tree(DECL_SAVED_TREE(fndecl), findconstructors, tmap);
 }

xpcom/analysis/static-checking.js

@@ -36,194 +36,26 @@ LoadModules(options['dehydra-modules']);
 if (treehydra_enabled())
   LoadModules(options['treehydra-modules']);
 
-/**
- * gClassMap maps class names to an object with the following properties:
- *
- * .final = true   if the class has been annotated as final, and may not be
- *                 subclassed
- * .stack = true   if the class has been annotated as a class which may only
- *                 be instantiated on the stack
- */
-var gClassMap = {};
-
-function ClassType(name)
-{
-  this.name = name;
-}
-
-ClassType.prototype = {
-  final: false,
-  stack: false,
-};
-
 function process_type(c)
 {
-  if (c.kind == 'class' || c.kind == 'struct')
-    get_class(c, true);
-
   for each (let module in modules)
     if (module.hasOwnProperty('process_type'))
       module.process_type(c);
 }
 
-/**
- * Get the ClassType for a type 'c'
- *
- * If allowIncomplete is true and the type is incomplete, this function
- * will return null.
- *
- * If allowIncomplete is false and the type is incomplete, this function will
- * throw.
- */
-function get_class(c, allowIncomplete)
+function hasAttribute(c, attrname)
 {
-  var classattr, base, member, type, realtype, foundConstructor;
-  var bases = [];
-
-  if (c.isIncomplete) {
-    if (allowIncomplete)
-      return null;
-
-    throw Error("Can't process incomplete type '" + c + "'.");
-  }
-
-  if (gClassMap.hasOwnProperty(c.name)) {
-    return gClassMap[c.name];
-  }
-
-  for each (base in c.bases) {
-      realtype = get_class(base, allowIncomplete);
-      if (realtype == null) {
-        error("Complete type " + c + " has incomplete base " + base);
-        return null;
-      }
-
-      bases.push(realtype);
-  }
-
-  function hasAttribute(attrname)
-  {
-    var attr;
-
-    if (c.attributes === undefined)
-      return false;
-
-    for each (attr in c.attributes) {
-      if (attr.name == 'user' && attr.value[0] == attrname) {
-        return true;
-      }
-    }
+  var attr;
 
+  if (c.attributes === undefined)
     return false;
-  }
 
-  classattr = new ClassType(c.name);
-  gClassMap[c.name] = classattr;
+  for each (attr in c.attributes)
+    if (attr.name == 'user' && attr.value[0] == attrname)
+      return true;
 
-  // check for .final
-
-  if (hasAttribute('NS_final')) {
-    classattr.final = true;
-  }
-
-  // check for .stack
-
-  if (hasAttribute('NS_stack')) {
-    classattr.stack = true;
-  }
-  else {
-    for each (base in bases) {
-      if (base.stack) {
-        classattr.stack = true;
-        break;
-      }
-    }
-  }
-
-  if (!classattr.stack) {
-    // Check members
-    for each (member in c.members) {
-      if (member.isFunction)
-        continue;
-
-      type = member.type;
-
-      /* recurse through arrays and typedefs */
-      while (true) {
-          if (type === undefined) {
-              break;
-          }
-              
-          if (type.isArray) {
-              type = type.type;
-              continue;
-          }
-          if (type.typedef) {
-              type = type.typedef;
-              continue;
-          }
-          break;
-      }
-
-      if (type === undefined) {
-          warning("incomplete type  for member " + member + ".");
-          continue;
-      }
-
-      if (type.isPointer || type.isReference) {
-          continue;
-      }
-
-      if (!type.kind || (type.kind != 'class' && type.kind != 'struct')) {
-          continue;
-      }
-
-      var membertype = get_class(type, false);
-      if (membertype.stack) {
-        classattr.stack = true;
-        break;
-      }
-    }
-  }
-
-  // Check for errors at declaration-time
-
-  for each (base in bases) {
-    if (base.final) {
-      error("class '" + c.name + "' inherits from final class '" + base.name + "'.");
-    }
-  }
-
-  // At the moment, any class that is .final has to have a constructor, or
-  // we can't detect callsites... this may change with treehydra.
-  if (classattr.stack) {
-    foundConstructor = false;
-    for each (member in c.members) {
-      if (member.isConstructor) {
-        foundConstructor = true;
-        break;
-      }
-    }
-
-    if (!foundConstructor) {
-      warning(c.loc + ": class " + c.name + " is marked stack-only but doesn't have a constructor. Static checking can't detect instantiations of this class properly.");
-    }
-  }
-
-  return classattr;
+  return false;
 }
-
-/**
- * Unwrap any array of types back to their base type.
- */
-function unwrapArray(t)
-{
-  while (t.isArray) {
-    t = t.type;
-  }
-  return t;
-}
-
 function process_function(f, stmts)
 {
   for each (let module in modules)

xpcom/tests/static-checker/Makefile.in

@@ -55,6 +55,12 @@ FINAL_FAILURE_TESTCASES = \
 STACK_FAILURE_TESTCASES = \
   TestStack.cpp \
   TestStackTemplate.cpp \
+  StackNoConstructor.cpp \
+  StackVector.cpp \
+  $(NULL)
+
+STACK_PASS_TESTCASES = \
+  StackPlacementNewOK.cpp \
   $(NULL)
 
 OUTPARAMS_WARNING_TESTCASES = \
@@ -100,6 +106,7 @@ STATIC_WARNING_TESTCASES = \
 
 STATIC_PASS_TESTCASES = \
   $(OUTPARAMS_PASS_TESTCASES) \
+  $(STACK_PASS_TESTCASES) \
   $(NULL)
 
 include $(topsrcdir)/config/rules.mk

xpcom/tests/static-checker/StackNoConstructor.cpp

@@ -0,0 +1,11 @@
+#include "nscore.h"
+
+struct NS_STACK_CLASS A
+{
+  int i;
+};
+
+void* Foo()
+{
+  return new A();
+}

xpcom/tests/static-checker/StackPlacementNewOK.cpp

@@ -0,0 +1,15 @@
+#include "nscore.h"
+#include NEW_H
+
+struct NS_STACK_CLASS A
+{
+  int i;
+};
+
+int Foo()
+{
+  char buf[sizeof(A)];
+
+  A *a = new(&buf) A;
+  return a->i;
+}

xpcom/tests/static-checker/StackVector.cpp

@@ -0,0 +1,12 @@
+#include "nscore.h"
+
+struct NS_STACK_CLASS A
+{
+  int i;
+};
+
+void* Foo()
+{
+  A *a = new A[8];
+  return a;
+}