* Re: [google] Enhance Annotalysis to support cloned functions/methods (issue4591066)
@ 2011-06-13 20:12 lcwu
2011-06-13 20:39 ` Diego Novillo
0 siblings, 1 reply; 5+ messages in thread
From: lcwu @ 2011-06-13 20:12 UTC (permalink / raw)
To: dnovillo, jyasskin; +Cc: aaw, delesley, gcc-patches, reply
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#newcode1159
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#newcode2151
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.
http://codereview.appspot.com/4591066/
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [google] Enhance Annotalysis to support cloned functions/methods (issue4591066)
2011-06-13 20:12 [google] Enhance Annotalysis to support cloned functions/methods (issue4591066) lcwu
@ 2011-06-13 20:39 ` Diego Novillo
0 siblings, 0 replies; 5+ messages in thread
From: Diego Novillo @ 2011-06-13 20:39 UTC (permalink / raw)
To: Le-Chun Wu, Diego Novillo, Jeffrey Yasskin, Ollie Wild, delesley,
gcc-patches, reply
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [google] Enhance Annotalysis to support cloned functions/methods (issue4591066)
@ 2011-06-11 18:53 dnovillo
0 siblings, 0 replies; 5+ messages in thread
From: dnovillo @ 2011-06-11 18:53 UTC (permalink / raw)
To: lcwu, jyasskin; +Cc: aaw, delesley, gcc-patches, reply
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#newcode1159
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#newcode2151
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.
http://codereview.appspot.com/4591066/
^ permalink raw reply [flat|nested] 5+ messages in thread
* [google] Enhance Annotalysis to support cloned functions/methods (issue4591066)
@ 2011-06-11 6:22 Le-Chun Wu
0 siblings, 0 replies; 5+ messages in thread
From: Le-Chun Wu @ 2011-06-11 6:22 UTC (permalink / raw)
To: reply, dnovillo, jyasskin, aaw, delesley, gcc-patches
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [google] Enhance Annotalysis to support cloned functions/methods (issue4591066)
@ 2011-06-10 18:31 Le-Chun Wu
0 siblings, 0 replies; 5+ messages in thread
From: Le-Chun Wu @ 2011-06-10 18:31 UTC (permalink / raw)
To: reply, gcc-patches
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-06-13 19:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-06-13 20:12 [google] Enhance Annotalysis to support cloned functions/methods (issue4591066) lcwu
2011-06-13 20:39 ` Diego Novillo
-- strict thread matches above, loose matches on Subject: below --
2011-06-11 18:53 dnovillo
2011-06-11 6:22 Le-Chun Wu
2011-06-10 18:31 Le-Chun Wu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).