Bug 431832: check outparams for PRBool or void return, r+a=bsmedberg
authorDavid Mandelin <dmandelin@mozilla.com>
Tue, 20 May 2008 11:26:03 -0700
changeset 15138 ce1413f5e3068d8a3f07d39b4fe6a17ed2de1e0c
parent 15137 819a2c29e295c93af2cb4d697ec91575794f6058
child 15139 979660ed509409e2787297780a5f5d46957e12c2
push id1
push userroot
push dateTue, 26 Apr 2011 22:38:44 +0000
treeherdermozilla-beta@bfdb6e623a36 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs431832
milestone1.9.1a1pre
Bug 431832: check outparams for PRBool or void return, r+a=bsmedberg
xpcom/analysis/outparams.js
xpcom/glue/nsInterfaceHashtable.h
xpcom/tests/static-checker/Makefile.in
xpcom/tests/static-checker/e12.cpp
xpcom/tests/static-checker/e13.cpp
xpcom/tests/static-checker/o12.cpp
xpcom/tests/static-checker/o13.cpp
--- a/xpcom/analysis/outparams.js
+++ b/xpcom/analysis/outparams.js
@@ -10,51 +10,68 @@ include('unstable/adts.js');
 include('unstable/analysis.js');
 include('unstable/esp.js');
 
 include('liveness.js');
 include('mayreturn.js');
 
 MapFactory.use_injective = true;
 
+// Print a trace for each function analyzed
 let TRACE_FUNCTIONS = 0;
+// Trace operation of the ESP analysis, use 2 or 3 for more detail
 let TRACE_ESP = 0;
+// Trace determination of function call parameter semantics, 2 for detail
+let TRACE_CALL_SEM = 0;
+// Print time-taken stats 
 let TRACE_PERF = 0;
+// Log analysis results in a special format
 let LOG_RESULTS = false;
 
 // Filter functions to process per CLI
 let func_filter;
 if (this.arg == undefined || this.arg == '') {
   func_filter = function(fd) true;
 } else {
   func_filter = function(fd) function_decl_name(fd) == this.arg;
 }
 
 function process_tree(func_decl) {
   if (!func_filter(func_decl)) return;
 
   // Determine outparams and return if function not relevant
+  if (is_constructor(func_decl)) return;
+  let psem = OutparamCheck.prototype.func_param_semantics(func_decl);
+  if (!psem.some(function(x) x == ps.OUT || x == ps.INOUT))
+    return;
   let decl = rectify_function_decl(func_decl);
-  if (decl.resultType != 'nsresult') return;
+  if (decl.resultType != 'nsresult' && decl.resultType != 'PRBool' &&
+      decl.resultType != 'void') {
+    warning("Cannot analyze outparam usage for function with return type '" +
+            decl.resultType + "'", location_of(func_decl));
+    return;
+  }
 
-  let psem = OutparamCheck.prototype.func_param_semantics(func_decl);
   let params = [ v for (v in flatten_chain(DECL_ARGUMENTS(func_decl))) ];
   let outparam_list = [];
   let psem_list = [];
   for (let i = 0; i < psem.length; ++i) {
     if (psem[i] == ps.OUT || psem[i] == ps.INOUT) {
       outparam_list.push(params[i]);
       psem_list.push(psem[i]);
     }
   }
   if (outparam_list.length == 0) return;
 
   // At this point we have a function we want to analyze
   let fstring = rfunc_string(decl);
-  if (TRACE_FUNCTIONS) print('* function ' + fstring);
+  if (TRACE_FUNCTIONS) {
+    print('* function ' + fstring);
+    print('    ' + loc_string(location_of(func_decl)));
+  }
   if (TRACE_PERF) timer_start(fstring);
   for (let i = 0; i < outparam_list.length; ++i) {
     let p = outparam_list[i];
     if (TRACE_FUNCTIONS) {
       print("  outparam " + expr_display(p) + " " + DECL_UID(p) + ' ' + 
             psem_list[i].label);
     }
   }
@@ -72,31 +89,36 @@ function process_tree(func_decl) {
   }
   
   let [retvar, retvars] = function() {
     let trace = 0;
     let a = new MayReturnAnalysis(cfg, trace);
     a.run();
     return [a.retvar, a.vbls];
   }();
-  if (retvar == undefined) throw new Error("assert");
+  if (retvar == undefined && decl.resultType != 'void') throw new Error("assert");
 
   {
     let trace = TRACE_ESP;
     let fts = link_switches(cfg);
     let a = new OutparamCheck(cfg, psem_list, outparam_list, retvar, retvars, fts, trace);
     // This is annoying, but this field is only used for logging anyway.
     a.fndecl = func_decl;
     a.run();
-    a.check();
+    a.check(decl.resultType == 'void', func_decl);
   }
   
   if (TRACE_PERF) timer_stop(fstring);
 }
 
+function is_constructor(function_decl)
+{
+  return function_decl.decl_common.lang_specific.decl_flags.constructor_attr;
+}
+
 // Outparam check analysis
 function OutparamCheck(cfg, psem_list, outparam_list, retvar, retvar_set, finally_tmps, trace) {
   this.retvar = retvar;
   this.psem_list = psem_list;
   // We need both an ordered set and a lookup structure
   this.outparam_list = outparam_list
   this.outparams = create_decl_set(outparam_list);
   this.psvar_list = outparam_list.slice(0);
@@ -204,17 +226,18 @@ OutparamCheck.prototype.flowState = func
   case CALL_EXPR:
     this.processCall(undefined, isn, isn, state);
     break;
   case SWITCH_EXPR:
   case COND_EXPR:
     // This gets handled by flowStateCond instead, has no exec effect
     break;
   case RETURN_EXPR:
-    this.processAssign(isn.operands()[0], state);
+    let op = isn.operands()[0];
+    if (op) this.processAssign(isn.operands()[0], state);
     break;
   case LABEL_EXPR:
   case RESX_EXPR:
   case ASM_EXPR:
     // NOPs for us
     break;
   default:
     print(TREE_CODE(isn));
@@ -440,17 +463,19 @@ OutparamCheck.prototype.processTest = fu
 };
 
 // The big one: outparam semantics of function calls.
 OutparamCheck.prototype.processCall = function(dest, expr, blame, state) {
   let args = call_args(expr);
   let callable = callable_arg_function_decl(TREE_OPERAND(expr, 1));
   let psem = this.func_param_semantics(callable);
     
-  //print('PSEM ' + psem);
+  if (TRACE_CALL_SEM) {
+    print("param semantics:" + psem);
+  }
 
   if (args.length != psem.length) {
     let ct = TREE_TYPE(callable);
     if (TREE_CODE(ct) == POINTER_TYPE) ct = TREE_TYPE(ct);
     if (args.length < psem.length || !stdarg_p(ct)) {
       //print("TTT " + type_string(ct));
       let name = function_decl_name(callable);
       // TODO Can __builtin_memcpy write to an outparam? Probably not.
@@ -485,23 +510,25 @@ OutparamCheck.prototype.processCall = fu
     if (!DECL_P(arg) || !this.outparams.has(arg)) continue;
     // At this point, we may be writing to an outparam 
     updates.push([arg, sem]);
   }
   
   if (updates.length) {
     if (dest != undefined && DECL_P(dest)) {
       // Update & stored rv. Do updates predicated on success.
+      let [ succ_ret, fail_ret ] = ret_coding(callable);
+
       state.update(function(ss) {
         let [s1, s2] = [ss.copy(), ss]; // s1 success, s2 fail
         for each (let [vbl, sem] in updates) {
           s1.assignValue(vbl, sem.val, blame);
-          s1.assignValue(dest, av.ZERO, blame);
+          s1.assignValue(dest, succ_ret, blame);
         }
-        s2.assignValue(dest, av.NONZERO, blame);
+        s2.assignValue(dest, fail_ret, blame);
         return [s1,s2];
       });
     } else {
       // Discarded rv. Per spec in the bug, we assume that either success
       // or failure is possible (if not, callee should return void).
       // Exceptions: Methods that return void and string mutators are
       //     considered no-fail.
       state.update(function(ss) {
@@ -522,49 +549,74 @@ OutparamCheck.prototype.processCall = fu
   } else {
     // no updates, just kill any destination for the rv
     if (dest != undefined && DECL_P(dest)) {
       state.remove(dest, blame);
     }
   }
 };
 
+/** Return the return value coding of the given function. This is a pair
+ *  [ succ, fail ] giving the abstract values of the return value under
+ *  success and failure conditions. */
+function ret_coding(callable) {
+  let type = TREE_TYPE(callable);
+  if (TREE_CODE(type) == POINTER_TYPE) type = TREE_TYPE(type);
+
+  let rtname = TYPE_NAME(TREE_TYPE(type));
+  if (rtname && IDENTIFIER_POINTER(DECL_NAME(rtname)) == 'PRBool') {
+    return [ av.NONZERO, av.ZERO ];
+  } else {
+    return [ av.ZERO, av.NONZERO ];
+  }
+}
+
 function unwrap_outparam(arg, state) {
   if (!DECL_P(arg) || state.factory.outparams.has(arg)) return arg;
 
   let outparam;
   for (let ss in state.substates.getValues()) {
     let val = ss.get(arg);
     if (val != undefined && val.hasOwnProperty('outparam')) {
       outparam = val.outparam;
     }
   }
   if (outparam) return outparam;
   return arg;
 }
 
 // Check for errors. Must .run() analysis before calling this.
-OutparamCheck.prototype.check = function() {
+OutparamCheck.prototype.check = function(isvoid, fndecl) {
   let state = this.cfg.x_exit_block_ptr.stateOut;
   for (let substate in state.substates.getValues()) {
-    this.checkSubstate(substate);
+    this.checkSubstate(isvoid, fndecl, substate);
   }
 }
 
-OutparamCheck.prototype.checkSubstate = function(ss) {
-  let rv = ss.get(this.retvar);
-  switch (rv) {
-  case av.ZERO:
+OutparamCheck.prototype.checkSubstate = function(isvoid, fndecl, ss) {
+  if (isvoid) {
     this.checkSubstateSuccess(ss);
-    break;
-  case av.NONZERO:
-    this.checkSubstateFailure(ss);
-    break;
-  default:
-    throw new Error("ni " + rv);
+  } else {
+    let [succ, fail] = ret_coding(fndecl);
+    let rv = ss.get(this.retvar);
+    switch (rv) {
+    case succ:
+      this.checkSubstateSuccess(ss);
+      break;
+    case fail:
+      this.checkSubstateFailure(ss);
+      break;
+    default:
+      // This condition indicates a bug in outparams.js. We'll just
+      // warn so we don't break static analysis builds.
+      warning("Outparams checker cannot determine rv success/failure",
+              location_of(fndecl));
+      this.checkSubstateSuccess(ss);
+      this.checkSubstateFailure(ss);
+    }
   }
 }
 
 OutparamCheck.prototype.checkSubstateSuccess = function(ss) {
   for (let i = 0; i < this.psem_list.length; ++i) {
     let [v, psem] = [ this.outparam_list[i], this.psem_list[i] ];
     if (psem == ps.INOUT) continue;
     let val = ss.get(v);
@@ -601,27 +653,31 @@ OutparamCheck.prototype.checkSubstateFai
       this.logResult('fail', '', 'ok');
     }
   }    
 }
 
 OutparamCheck.prototype.warn = function() {
   let tag = arguments[0];
   let v = arguments[1];
-  let rest = Array.slice(arguments, 2);
+  // Filter out any undefined values.
+  let rest = [ x for each (x in Array.slice(arguments, 2)) ];
 
   let label = expr_display(v)
   let lines = [ tag + ': ' + label,
               'Outparam declared at: ' + loc_string(location_of(v)) ];
   lines = lines.concat(rest);
   let msg = lines.join('\n    ');
   warning(msg);
 }
 
 OutparamCheck.prototype.formatBlame = function(msg, ss, v) {
+  // If v is undefined, that means we don't have that variable, e.g., 
+  // a return variable in a void function, so just return undefined;
+  if (v == undefined) return undefined;
   let blame = ss.getBlame(v);
   let loc = blame ? loc_string(location_of(blame)) : '?';
   return(msg + ": " + loc);
 }
 
 OutparamCheck.prototype.logResult = function(rv, msg, kind) {
   if (LOG_RESULTS) {
     let s = [ '"' + x + '"' for each (x in [ loc_string(location_of(this.fndecl)), function_decl_name(this.fndecl), rv, msg, kind ]) ].join(', ');
@@ -635,22 +691,25 @@ let ps = {
   OUTNOFAIL: { label: 'out-no-fail', val: av.WRITTEN },
   OUT:       { label: 'out',   val: av.WRITTEN },
   INOUT:     { label: 'inout',   val: av.WRITTEN },
   MAYBE:     { label: 'maybe', val: av.MAYBE_WRITTEN},  // maybe out
   CONST:     { label: 'const' }   // i.e. not out
 };
 
 // Return the param semantics of a FUNCTION_DECL or VAR_DECL representing
-// a function pointer.
+// a function pointer. The result is a pair [ ann, sems ].
 OutparamCheck.prototype.func_param_semantics = function(callable) {
   let ftype = TREE_TYPE(callable);
   if (TREE_CODE(ftype) == POINTER_TYPE) ftype = TREE_TYPE(ftype);
   // What failure semantics to use for outparams
-  let nofail = TREE_TYPE(TREE_TYPE(ftype)) == VOID_TYPE;
+  let rtype = TREE_TYPE(ftype);
+  let nofail = rtype == VOID_TYPE;
+  // Whether to guess outparams by type
+  let guess = type_string(rtype) == 'nsresult';
 
   // Set up param lists for analysis
   let params;     // param decls, if available
   let types;      // param types
   let string_mutator = false;
   if (TREE_CODE(callable) == FUNCTION_DECL) {
     params = [ p for (p in function_decl_params(callable)) ];
     types = [ TREE_TYPE(p) for each (p in params) ];
@@ -664,24 +723,29 @@ OutparamCheck.prototype.func_param_seman
   let ans = [];
   for (let i = 0; i < types.length; ++i) {
     let sem;
     if (i == 0 && string_mutator) {
       // Special case: string mutator receiver is an no-fail outparams
       sem = ps.OUTNOFAIL;
     } else {
       if (params) sem = param_semantics(params[i]);
+      if (TRACE_CALL_SEM >= 2) print("param " + i + ": annotated " + sem);
       if (sem == undefined) {
-        sem = param_semantics_by_type(types[i]);
-        // Params other than last are guessed as MAYBE
-        if (i < types.length - 1 && sem == ps.OUT) sem = ps.MAYBE;
+        if (guess) {
+          sem = param_semantics_by_type(types[i]);
+          // Params other than last are guessed as MAYBE
+          if (i < types.length - 1 && sem == ps.OUT) sem = ps.MAYBE;
+        } else {
+          sem = ps.CONST;
+        }
       }
       if (sem == ps.OUT && nofail) sem = ps.OUTNOFAIL;
     }
-    if (ans == undefined) throw new Error("assert");
+    if (sem == undefined) throw new Error("assert");
     ans.push(sem);
   }
   return ans;
 }
 
 // Return the param semantics as indicated by the attributes, or
 // undefined if no param attribute is present.
 function param_semantics(decl) {
@@ -701,21 +765,23 @@ function param_semantics(decl) {
   return undefined;
 }
 
 // Return param semantics as guessed from types. Never returns undefined.
 function param_semantics_by_type(type) {
   switch (TREE_CODE(type)) {
   case POINTER_TYPE:
     let pt = TREE_TYPE(type);
+    if (TYPE_READONLY(pt)) return ps.CONST;
     switch (TREE_CODE(pt)) {
     case RECORD_TYPE:
       // TODO: should we consider field writes?
       return ps.CONST;
     case POINTER_TYPE:
+    case ARRAY_TYPE:
       // Outparam if nsIFoo** or void **
       let ppt = TREE_TYPE(pt);
       let tname = TYPE_NAME(ppt);
       if (tname == undefined) return ps.CONST;
       let name = decl_name_string(tname);
       return name == 'void' || name == 'char' || name == 'PRUnichar' ||
         name.substr(0, 3) == 'nsI' ?
         ps.OUT : ps.CONST;
@@ -734,21 +800,23 @@ function param_semantics_by_type(type) {
       print("Y " + TREE_CODE(pt));
       print('Y ' + type_string(pt));
       throw new Error("ni");
     }
     break;
   case REFERENCE_TYPE:
     let rt = TREE_TYPE(type);
     return !TYPE_READONLY(rt) && is_string_type(rt) ? ps.OUT : ps.CONST;
+  case BOOLEAN_TYPE:
   case INTEGER_TYPE:
   case REAL_TYPE:
   case ENUMERAL_TYPE:
   case RECORD_TYPE:
   case UNION_TYPE:    // unsafe, c/b pointer
+  case ARRAY_TYPE:
     return ps.CONST;
   default:
     print("Z " + TREE_CODE(type));
     print('Z ' + type_string(type));
     throw new Error("ni");
   }
 }
 
--- a/xpcom/glue/nsInterfaceHashtable.h
+++ b/xpcom/glue/nsInterfaceHashtable.h
@@ -58,17 +58,17 @@ public:
   typedef typename KeyClass::KeyType KeyType;
   typedef Interface* UserDataType;
 
   /**
    * @copydoc nsBaseHashtable::Get
    * @param pData This is an XPCOM getter, so pData is already_addrefed.
    *   If the key doesn't exist, pData will be set to nsnull.
    */
-  PRBool Get(KeyType aKey, UserDataType* pData) const;
+  PRBool Get(KeyType aKey, UserDataType* pData NS_OUTPARAM) const;
 
   /**
    * Gets a weak reference to the hashtable entry.
    * @param aFound If not nsnull, will be set to PR_TRUE if the entry is found,
    *               to PR_FALSE otherwise.
    * @return The entry, or nsnull if not found. Do not release this pointer!
    */
   Interface* GetWeak(KeyType aKey, PRBool* aFound = nsnull) const;
@@ -88,17 +88,17 @@ public:
   typedef typename KeyClass::KeyType KeyType;
   typedef Interface* UserDataType;
 
   /**
    * @copydoc nsBaseHashtable::Get
    * @param pData This is an XPCOM getter, so pData is already_addrefed.
    *   If the key doesn't exist, pData will be set to nsnull.
    */
-  PRBool Get(KeyType aKey, UserDataType* pData) const;
+  PRBool Get(KeyType aKey, UserDataType* pData NS_OUTPARAM) const;
 
   // GetWeak does not make sense on a multi-threaded hashtable, where another
   // thread may remove the entry (and hence release it) as soon as GetWeak
   // returns
 };
 
 
 //
--- a/xpcom/tests/static-checker/Makefile.in
+++ b/xpcom/tests/static-checker/Makefile.in
@@ -64,30 +64,34 @@ OUTPARAMS_WARNING_TESTCASES = \
   e4.cpp \
   e5.cpp \
   e6.cpp \
   e7.cpp \
   e8.cpp \
   e9.cpp \
   e10.cpp \
   e11.cpp \
+  e12.cpp \
+  e13.cpp \
   $(NULL)
 
 OUTPARAMS_PASS_TESTCASES = \
   o1.cpp \
   o2.cpp \
   o3.cpp \
   o4.cpp \
   o5.cpp \
   o6.cpp \
   o7.cpp \
   o8.cpp \
   o9.cpp \
   o10.cpp \
   o11.cpp \
+  o12.cpp \
+  o13.cpp \
   $(NULL)
 
 STATIC_FAILURE_TESTCASES = \
   $(FINAL_FAILURE_TESTCASES) \
   $(STACK_FAILURE_TESTCASES) \
   $(NULL)
 
 STATIC_WARNING_TESTCASES = \
new file mode 100644
--- /dev/null
+++ b/xpcom/tests/static-checker/e12.cpp
@@ -0,0 +1,10 @@
+typedef int PRBool;
+typedef int PRUint32;
+typedef int PRInt32;
+
+typedef PRUint32 nsresult;
+typedef short PRUnichar;
+
+PRBool bar(int *p __attribute__((user("NS_outparam")))) {
+  return 1;
+}
new file mode 100644
--- /dev/null
+++ b/xpcom/tests/static-checker/e13.cpp
@@ -0,0 +1,9 @@
+typedef int PRBool;
+typedef int PRUint32;
+typedef int PRInt32;
+
+typedef PRUint32 nsresult;
+typedef short PRUnichar;
+
+void bar(int *p __attribute__((user("NS_outparam")))) {
+}
new file mode 100644
--- /dev/null
+++ b/xpcom/tests/static-checker/o12.cpp
@@ -0,0 +1,22 @@
+typedef int PRBool;
+typedef int PRUint32;
+typedef int PRInt32;
+
+typedef PRUint32 nsresult;
+typedef short PRUnichar;
+
+#define NS_OUTPARAM __attribute__((user("NS_outparam")))
+
+PRBool baz(int *p NS_OUTPARAM);
+
+PRBool bar(int *p NS_OUTPARAM) {
+  return baz(p);
+}
+
+nsresult foo(int *p) {
+  if (bar(p)) {
+    return 0;
+  } else {
+    return 1;
+  }
+}
new file mode 100644
--- /dev/null
+++ b/xpcom/tests/static-checker/o13.cpp
@@ -0,0 +1,14 @@
+typedef int PRBool;
+typedef int PRUint32;
+typedef int PRInt32;
+
+typedef PRUint32 nsresult;
+typedef short PRUnichar;
+
+nsresult baz(int *p);
+
+void bar(int *p __attribute__((user("NS_outparam")))) {
+  if (baz(p) != 0) {
+    *p = 0;
+  }
+}