[PATCH] [annotalysis] Support IPA-SRA cloned functions (issue 4591066)

Delesley Hutchins delesley@google.com
Wed Jun 22 20:24:00 GMT 2011


Hi,

This patch is merely a port of an earlier patch, made by Le-Chun Wu,
from google/main
to annotalysis.  It extends Annotalysis to support cloned
functions/methods (especially
created by IPA-SRA).

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

Okay for branches/annotalysis?

  -DeLesley


2011-06-22  Le-Chun Wu  <lcwu@google.com>,  DeLesley Hutchins
<delesley@google.com>
        * tree-threadsafe-analyze.c (build_fully_qualified_lock): Handle
        IPA-SRA cloned methods.
        (get_canonical_lock_expr): Fold expressions that are INDIRECT_REF on
        top of ADDR_EXPR.
        (check_lock_required): Handle IPA-SRA cloned methods.
        (check_func_lock_excluded): Likewise.
        (process_function_attrs): Likewise.


Index: gcc/testsuite/g++.dg/thread-ann/thread_annot_lock-78.C
===================================================================
--- gcc/testsuite/g++.dg/thread-ann/thread_annot_lock-78.C	(revision 0)
+++ gcc/testsuite/g++.dg/thread-ann/thread_annot_lock-78.C	(revision 175062)
@@ -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" }
+}
Index: gcc/tree-threadsafe-analyze.c
===================================================================
--- gcc/tree-threadsafe-analyze.c	(revision 175061)
+++ gcc/tree-threadsafe-analyze.c	(working copy)
@@ -696,9 +696,13 @@
      rid of the ADDR_EXPR operator before we form the new lock expression.  */
   if (TREE_CODE (canon_base) != ADDR_EXPR)
     {
-      gcc_assert (POINTER_TYPE_P (TREE_TYPE (canon_base)));
-      canon_base = build1 (INDIRECT_REF, TREE_TYPE (TREE_TYPE (canon_base)),
-                           canon_base);
+      /* Note that if the base object is neither an ADDR_EXPR, nor a pointer,
+         most likely we have done IPA-SRA optimization and the DECL is a
+         cloned method, where reference parameters are changed to be passed
+         by value. So in this case, we don't need to do anything.  */
+      if (POINTER_TYPE_P (TREE_TYPE (canon_base)))
+        canon_base = build1 (INDIRECT_REF, TREE_TYPE (TREE_TYPE (canon_base)),
+                             canon_base);
     }
   else
     canon_base = TREE_OPERAND (canon_base, 0);
@@ -956,8 +960,18 @@
                                                      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 +1149,18 @@
                                                      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_unreachable ();
             }
           else
             base_obj = TREE_OPERAND (canon_base, 0);
@@ -1154,9 +1176,9 @@

   /* 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 +1186,10 @@
                                           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)
     {
@@ -1963,10 +1989,18 @@
                                                  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 (fdecl))
+            base_obj = canon_base;
+          else
+            gcc_unreachable ();
         }
       else
         base_obj = TREE_OPERAND (canon_base, 0);
@@ -2116,8 +2150,17 @@
     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. Also note that the parameters of a cloned method could
+     be optimized away.  */
+  if (TREE_CODE (TREE_TYPE (DECL_ORIGIN (fdecl))) == METHOD_TYPE
+      && gimple_call_num_args(call) > 0)
     base_obj = gimple_call_arg (call, 0);

   /* Check whether this is a locking primitive of any kind.  */



More information about the Gcc-patches mailing list