[google] Enhance Annotalysis to support cloned functions/methods (issue4591066)

[lcwu at google dot com (Le-Chun Wu)]

Enhance Annotalysis to support cloned functions/methods (especially created
by IPA-SRA). The patch basically does the following:

1. For a FUNCTION_DECL, check whether it's a clone, and if so, grab its
original DECL.
2. Deal with the situation where a reference/pointer parameter is converted
to a value parameter by IPA-SRA.

Bootstrapped and passed GCC regression testsuite on x86_64-unknown-linux-gnu.

OK for google/main?

Le-chun


2011-06-10   Le-Chun Wu  <lcwu@google.com>

	* gcc/testsuite/g++.dg/thread-ann/thread_annot_lock-78.C: New test.
	* gcc/tree-threadsafe-analyze.c (get_canonical_lock_expr): Fold
	expressions that are INDIRECT_REF on top of ADDR_EXPR.
	(check_lock_required): Handle cloned methods.
	(process_function_attrs): Likewise.

diff --git a/gcc/testsuite/g++.dg/thread-ann/thread_annot_lock-78.C b/gcc/testsuite/g++.dg/thread-ann/thread_annot_lock-78.C
new file mode 100644
index 0000000..5055a92
--- /dev/null
+++ b/gcc/testsuite/g++.dg/thread-ann/thread_annot_lock-78.C
@@ -0,0 +1,28 @@
+// Test the support to handle cloned methods.
+// { dg-do compile }
+// { dg-options "-O2 -Wthread-safety -fipa-sra" }
+
+#include "thread_annot_common.h"
+
+struct A {
+  int *data_ PT_GUARDED_BY(mu_);
+  mutable Mutex mu_;
+  void Reset(void) EXCLUSIVE_LOCKS_REQUIRED(mu_) {
+    delete data_;
+  }
+};
+
+struct B {
+  A a_;
+  void TestFunc1();
+  void TestFunc2();
+};
+
+void B::TestFunc1() {
+  MutexLock l(&a_.mu_);
+  a_.Reset();  // This call is an IPA-SRA clone candidate.
+}
+
+void B::TestFunc2() {
+  a_.Reset();  // { dg-warning "Calling function 'Reset' requires lock" }
+}
diff --git a/gcc/tree-threadsafe-analyze.c b/gcc/tree-threadsafe-analyze.c
index 3510219..d39666d 100644
--- a/gcc/tree-threadsafe-analyze.c
+++ b/gcc/tree-threadsafe-analyze.c
@@ -956,8 +956,18 @@ get_canonical_lock_expr (tree lock, tree base_obj, bool is_temp_expr,
                                                      true /* is_temp_expr */,
                                                      new_leftmost_base_var);
           if (base != canon_base)
-            lock = build1 (INDIRECT_REF, TREE_TYPE (TREE_TYPE (canon_base)),
-                           canon_base);
+            {
+              /* If CANON_BASE is an ADDR_EXPR (e.g. &a), doing an indirect or
+                 memory reference on top of it is equivalent to accessing the
+                 variable itself. That is, *(&a) == a. So if that's the case,
+                 simply return the variable. Otherwise, build an indirect ref
+                 expression.  */
+              if (TREE_CODE (canon_base) == ADDR_EXPR)
+                lock = TREE_OPERAND (canon_base, 0);
+              else
+                lock = build1 (INDIRECT_REF,
+                               TREE_TYPE (TREE_TYPE (canon_base)), canon_base);
+            }
           break;
         }
       default:
@@ -1135,10 +1145,18 @@ check_lock_required (tree lock, tree decl, tree base_obj, bool is_indirect_ref,
                                                      NULL_TREE);
           if (TREE_CODE (canon_base) != ADDR_EXPR)
             {
-              gcc_assert (POINTER_TYPE_P (TREE_TYPE (canon_base)));
-              base_obj = build1 (INDIRECT_REF,
-                                 TREE_TYPE (TREE_TYPE (canon_base)),
-                                 canon_base);
+              if (POINTER_TYPE_P (TREE_TYPE (canon_base)))
+                base_obj = build1 (INDIRECT_REF,
+                                   TREE_TYPE (TREE_TYPE (canon_base)),
+                                   canon_base);
+              /* If the base object is neither an ADDR_EXPR, nor a pointer,
+                 and DECL is a cloned method, most likely we have done IPA-SRA
+                 optimization, where reference parameters are changed to be
+                 passed by value. So in this case, just use the CANON_BASE.  */
+              else if (DECL_ABSTRACT_ORIGIN (decl))
+                base_obj = canon_base;
+              else
+                gcc_assert (false);
             }
           else
             base_obj = TREE_OPERAND (canon_base, 0);
@@ -1154,9 +1172,9 @@ check_lock_required (tree lock, tree decl, tree base_obj, bool is_indirect_ref,
 
   /* We want to use fully-qualified expressions (i.e. including base_obj
      if any) for DECL when emitting warning messages.  */
-  if (base_obj)
+  if (TREE_CODE (decl) != FUNCTION_DECL)
     {
-      if (TREE_CODE (decl) != FUNCTION_DECL)
+      if (base_obj)
         {
           tree full_decl = build3 (COMPONENT_REF, TREE_TYPE (decl),
                                    base_obj, decl, NULL_TREE);
@@ -1164,6 +1182,10 @@ check_lock_required (tree lock, tree decl, tree base_obj, bool is_indirect_ref,
                                           true /* is_temp_expr */, NULL_TREE);
         }
     }
+  else
+    /* If DECL is a function, call DECL_ORIGIN first in case it is a clone so
+       that we can avoid using the cloned name in the warning messages.  */
+    decl = DECL_ORIGIN (decl);
 
   if (!lock)
     {
@@ -2116,8 +2138,15 @@ process_function_attrs (gimple call, tree fdecl,
     current_bb_info->writes_ignored = false;
 
   /* If the function is a class member, the first argument of the function
-     (i.e. "this" pointer) would be the base object.  */
-  if (TREE_CODE (TREE_TYPE (fdecl)) == METHOD_TYPE)
+     (i.e. "this" pointer) would be the base object. Note that here we call
+     DECL_ORIGIN on fdecl first before we check whether it's a METHOD_TYPE
+     because if fdecl is a cloned method, the TREE_CODE of its type would be
+     FUNCTION_DECL instead of METHOD_DECL, which would lead us to not grab
+     its base object. One possible situation where fdecl could be a clone is
+     when -fipa-sra is enabled. (-fipa-sra is enabled by default at -O2
+     starting from GCC-4.5.). The clones could be created as early as when
+     constructing SSA.  */
+  if (TREE_CODE (TREE_TYPE (DECL_ORIGIN (fdecl))) == METHOD_TYPE)
     base_obj = gimple_call_arg (call, 0);
 
   /* Check whether this is a locking primitive of any kind.  */

--
This patch is available for review at http://codereview.appspot.com/4591066