[google] Enhance Annotalysis to support cloned functions/methods (issue4591066)
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
* 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 000..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
[google] Enhance Annotalysis to support cloned functions/methods (issue4591066)
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 000..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)
[google] Port self-assign warning to google/main branch (issue4442075)
2011-04-22 Le-Chun Wu
gcc/c-family/ChangeLog:
* c-common.c (check_for_self_assign): New function.
* c-common.h: New function declaration.
* c.opt: New option.
gcc/ChangeLog:
* c-parser.c (c_parser_declaration_or_fndef): Check for self-assign.
(c_parser_expr_no_commas): Check for self-assign.
* common.opt: New option.
* doc/invoke.texi: Documentation for new options.
* fold-const.c (operand_equal_p): Allow operands without typres to
compare.
(fold_unary_loc_1): Renamed from fold_unary_loc.
(fold_unary_loc): New wrapper function.
(fold_binary_loc_1): Renamed from fold_binary_loc.
(fold_binary_loc): New wrapper function.
(fold_ternary_loc_1): Renamed from fold_ternary_loc.
(fold_ternary_loc): New wrapper function.
* tree.h (struct tree_base): New flag for folded expr.
(enum operand_equal_flag): New flags.
gcc/cp/ChangeLog:
* init.c (perform_member_init): Check for self-assign.
* parser.c (expr_is_pod): New function.
(cp_parser_assignment_expression): Check for self-assign.
(cp_parser_init_declarator): Check for self-assign.
gcc/testsuite/ChangeLog:
* testsuite/g++.dg/plugin/selfassign.c (check_self_assign): Renamed
from warn_self_assign.
(execute_warn_self_assign): Call a function by its new name.
* testsuite/g++.dg/warn/Wself-assign-1.C: New test case.
* testsuite/g++.dg/warn/Wself-assign-2.C: Likewise.
* testsuite/g++.dg/warn/Wself-assign-3.C: Likewise.
* testsuite/g++.dg/warn/Wself-assign-4.C: Likewise.
* testsuite/g++.dg/warn/Wself-assign-5.C: Likewise.
* testsuite/g++.dg/warn/Wself-assign-non-pod-1.C: Likewise.
* testsuite/g++.dg/warn/Wself-assign-non-pod-2.C: Likewise.
* testsuite/g++.dg/warn/Wself-assign-non-pod-3.C: Likewise.
* testsuite/g++.dg/warn/Wself-assign-non-pod-4.C: Likewise.
* testsuite/g++.dg/warn/Wself-assign-non-pod-5.C: Likewise.
* testsuite/gcc.dg/plugin/selfassign.c (check_self_assign): Renamed
from warn_self_assign.
(execute_warn_self_assign): Call a function by its new name.:
* testsuite/gcc.dg/wself-assign-1.c: New test case.
* testsuite/gcc.dg/wself-assign-2.c: Likewise.
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index ca17bd2..13f13a8 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -10495,4 +10495,21 @@ keyword_is_decl_specifier (enum rid keyword)
}
}
+/* Check for and warn about self-assignment or self-initialization.
+ LHS and RHS are the tree nodes for the left-hand side and right-hand side
+ of the assignment or initialization we are checking.
+ LOCATION is the source location for RHS. */
+
+void
+check_for_self_assign (location_t location, tree lhs, tree rhs)
+{
+ /* Only emit a warning if RHS is not a folded expression so that we don't
+ warn on something like x = x / 1. */
+ if (!EXPR_FOLDED (rhs)
+ && operand_equal_p (lhs, rhs,
+ OEP_PURE_SAME | OEP_ALLOW_NULL | OEP_ALLOW_NO_TYPE))
+warning_at (location, OPT_Wself_assign, G_("%qE is assigned to itself"),
+lhs);
+}
+
#include "gt-c-family-c-common.h"
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index 406def9..566aba7 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -957,6 +957,7 @@ extern VEC(tree,gc) *make_tree_vector (void);
extern void release_tree_vector (VEC(tree,gc) *);
extern VEC(tree,gc) *make_tree_vector_single (tree);
extern VEC(tree,gc) *make_tree_vector_copy (const VEC(tree,gc) *);
+extern void check_for_self_assign (location_t, tree, tree);
/* In c-gimplify.c */
extern void c_genericize (tree);
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index bb928fa..af38a1f 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -319,6 +319,10 @@ Wconversion-null
C++ ObjC++ Var(warn_conversion_null) Init(1) Warning
Warn for converting NULL from/to a non-pointer type
+Wself-assign-non-pod
+C++ ObjC++ Var(warn_self_assign_non_pod) Init(0) Warning
+Warn when a variable of a non-POD type is assigned to itself
+
Wsign-conversion
C ObjC C++ ObjC++ Var(warn_sign_conversion) Init(-1)
Warn for implicit type conversions between signed and unsigned integers
diff --git a/gcc/c-parser.c b/gcc/c-parser.c
index 25bd790..72a9f6c 100644
--- a/gcc/c-parser.c
+++ b/gcc/c-parser.c
@@ -1626,6 +1626,11 @@ c_parser_declaration_or_fndef (c_parser *parser, bool
fndef_ok,
maybe_warn_string_init (TREE_TYPE (d), init);
finish_decl (d, init_loc, init.value,
init.original_type, asm_name);
+ /* Check for and warn about self-initialization if
+ -Wself-assign is enabled. Don't warn if there is
+
Re: [google] Port self-assign warning to google/main branch (issue4442075)
This patch ports the implementation of -Wself-assign warning from a GCC-4.4.3 based tree to google/main branch. The warning checks for self-assignment and self-initialization. It is intended for detecting accidental self-assignment due to typos, and therefore does not warn on a statement that is semantically a self-assignment after constant folding (e.g. x = x + 0). The patch also includes another new (sub-)flag -Wself-assign-non-pod that controls whether or not to warn about self-assignment of non-POD variables. Bootstrapped and passed gcc regression testsuite on x86_64. Le-chun On Fri, Apr 22, 2011 at 1:08 PM, Le-Chun Wu wrote: > > 2011-04-22 Le-Chun Wu > > gcc/c-family/ChangeLog: > * c-common.c (check_for_self_assign): New function. > * c-common.h: New function declaration. > * c.opt: New option. > > gcc/ChangeLog: > * c-parser.c (c_parser_declaration_or_fndef): Check for self-assign. > (c_parser_expr_no_commas): Check for self-assign. > * common.opt: New option. > * doc/invoke.texi: Documentation for new options. > * fold-const.c (operand_equal_p): Allow operands without typres to > compare. > (fold_unary_loc_1): Renamed from fold_unary_loc. > (fold_unary_loc): New wrapper function. > (fold_binary_loc_1): Renamed from fold_binary_loc. > (fold_binary_loc): New wrapper function. > (fold_ternary_loc_1): Renamed from fold_ternary_loc. > (fold_ternary_loc): New wrapper function. > * tree.h (struct tree_base): New flag for folded expr. > (enum operand_equal_flag): New flags. > > gcc/cp/ChangeLog: > * init.c (perform_member_init): Check for self-assign. > * parser.c (expr_is_pod): New function. > (cp_parser_assignment_expression): Check for self-assign. > (cp_parser_init_declarator): Check for self-assign. > > gcc/testsuite/ChangeLog: > * testsuite/g++.dg/plugin/selfassign.c (check_self_assign): Renamed > from warn_self_assign. > (execute_warn_self_assign): Call a function by its new name. > * testsuite/g++.dg/warn/Wself-assign-1.C: New test case. > * testsuite/g++.dg/warn/Wself-assign-2.C: Likewise. > * testsuite/g++.dg/warn/Wself-assign-3.C: Likewise. > * testsuite/g++.dg/warn/Wself-assign-4.C: Likewise. > * testsuite/g++.dg/warn/Wself-assign-5.C: Likewise. > * testsuite/g++.dg/warn/Wself-assign-non-pod-1.C: Likewise. > * testsuite/g++.dg/warn/Wself-assign-non-pod-2.C: Likewise. > * testsuite/g++.dg/warn/Wself-assign-non-pod-3.C: Likewise. > * testsuite/g++.dg/warn/Wself-assign-non-pod-4.C: Likewise. > * testsuite/g++.dg/warn/Wself-assign-non-pod-5.C: Likewise. > * testsuite/gcc.dg/plugin/selfassign.c (check_self_assign): Renamed > from warn_self_assign. > (execute_warn_self_assign): Call a function by its new name.: > * testsuite/gcc.dg/wself-assign-1.c: New test case. > * testsuite/gcc.dg/wself-assign-2.c: Likewise. > > > -- > This patch is available for review at http://codereview.appspot.com/4442075
