Issue 4591066: [google] Enhance Annotalysis to support cloned functions/methods (Closed)
|
Created:
12 years, 10 months ago by Le-Chun Wu Modified:
12 years, 10 months ago CC:
Ollie Wild, delesley, gcc-patches_gcc.gnu.org Visibility:
Public. |
DescriptionFix submitted to google/main at r175062.
Patch Set 1 #Patch Set 2 : [google] Enhance Annotalysis to support cloned functions/methods #Patch Set 3 : [google] Enhance Annotalysis to support cloned functions/methods #
Total comments: 4
MessagesTotal messages: 5
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
Sign in to reply to this message.
Just identified a bug in my previous patch after running the compiler on
google code base. Basically the difference from the previous patch is for the
compiler to handle the case where the parameters of a cloned method are
optimized away.
Bootstrapped OK. Testing is still on-going. OK for google/main after testing
is done (and passed)?
Thanks,
Le-chun
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..6456737 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,17 @@ 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. 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. */
--
This patch is available for review at http://codereview.appspot.com/4591066
Sign in to reply to this message.
OK with some minor nits. Diego. http://codereview.appspot.com/4591066/diff/3001/gcc/tree-threadsafe-analyze.c File gcc/tree-threadsafe-analyze.c (right): http://codereview.appspot.com/4591066/diff/3001/gcc/tree-threadsafe-analyze.c... gcc/tree-threadsafe-analyze.c:1159: gcc_assert (false); >+ else >+ gcc_assert (false); >+ Change to gcc_unreachable (); http://codereview.appspot.com/4591066/diff/3001/gcc/tree-threadsafe-analyze.c... gcc/tree-threadsafe-analyze.c:2151: && gimple_call_num_args(call) > 0) 2147 starting from GCC-4.5.). The clones could be created as early as when 2148 constructing SSA. Also note that the parameters of a cloned method could 2149 be optimized away. */ 2150 if (TREE_CODE (TREE_TYPE (DECL_ORIGIN (fdecl))) == METHOD_TYPE 2151 && gimple_call_num_args(call) > 0) Wouldn't it be easier to make fdecl == DECL_ORIGIN (fdecl) earlier in the function? It's OK either way, though.
Sign in to reply to this message.
http://codereview.appspot.com/4591066/diff/3001/gcc/tree-threadsafe-analyze.c File gcc/tree-threadsafe-analyze.c (right): http://codereview.appspot.com/4591066/diff/3001/gcc/tree-threadsafe-analyze.c... gcc/tree-threadsafe-analyze.c:1159: gcc_assert (false); On 2011/06/11 17:52:51, Diego Novillo wrote: > >+ else > >+ gcc_assert (false); > >+ > > Change to gcc_unreachable (); Done. http://codereview.appspot.com/4591066/diff/3001/gcc/tree-threadsafe-analyze.c... gcc/tree-threadsafe-analyze.c:2151: && gimple_call_num_args(call) > 0) On 2011/06/11 17:52:51, Diego Novillo wrote: > 2147 starting from GCC-4.5.). The clones could be created as early as when > 2148 constructing SSA. Also note that the parameters of a cloned method > could > 2149 be optimized away. */ > 2150 if (TREE_CODE (TREE_TYPE (DECL_ORIGIN (fdecl))) == METHOD_TYPE > 2151 && gimple_call_num_args(call) > 0) > > Wouldn't it be easier to make fdecl == DECL_ORIGIN (fdecl) earlier in the > function? > > It's OK either way, though. Yes, my original fix was to make fdecl = DECL_ORIGINAL (fdecl). But I later changed it to this way because I wanted to tolerate the case where the base object (i.e. "this" pointer) is an object instead of a pointer only when fdecl is a clone. (i.e. I don't want to arbitrarily relax it.) That's why I kept fdecl intact.
Sign in to reply to this message.
On Mon, Jun 13, 2011 at 12:44, <lcwu@google.com> wrote: >> could >> 2149 be optimized away. */ >> 2150 if (TREE_CODE (TREE_TYPE (DECL_ORIGIN (fdecl))) == METHOD_TYPE >> 2151 && gimple_call_num_args(call) > 0) > >> Wouldn't it be easier to make fdecl == DECL_ORIGIN (fdecl) earlier in > > the >> >> function? > >> It's OK either way, though. > > Yes, my original fix was to make fdecl = DECL_ORIGINAL (fdecl). But I > later changed it to this way because I wanted to tolerate the case where > the base object (i.e. "this" pointer) is an object instead of a pointer > only when fdecl is a clone. (i.e. I don't want to arbitrarily relax it.) > That's why I kept fdecl intact. Ah, makes sense. Thanks. Diego.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||
