From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6838 invoked by alias); 10 Jun 2011 23:56:30 -0000 Received: (qmail 6827 invoked by uid 22791); 10 Jun 2011 23:56:29 -0000 X-SWARE-Spam-Status: No, hits=-1.5 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from smtp-out.google.com (HELO smtp-out.google.com) (74.125.121.67) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 10 Jun 2011 23:56:15 +0000 Received: from wpaz29.hot.corp.google.com (wpaz29.hot.corp.google.com [172.24.198.93]) by smtp-out.google.com with ESMTP id p5ANtncr007461; Fri, 10 Jun 2011 16:55:50 -0700 Received: from lcwu.mtv.corp.google.com (lcwu.mtv.corp.google.com [172.18.120.183]) by wpaz29.hot.corp.google.com with ESMTP id p5ANtlQa008796; Fri, 10 Jun 2011 16:55:48 -0700 Received: by lcwu.mtv.corp.google.com (Postfix, from userid 56351) id E974C1842B1; Fri, 10 Jun 2011 16:55:36 -0700 (PDT) To: reply@codereview.appspotmail.com, dnovillo@google.com, jyasskin@google.com, aaw@google.com, delesley@google.com, gcc-patches@gcc.gnu.org Subject: [google] Enhance Annotalysis to support cloned functions/methods (issue4591066) Message-Id: <20110610235536.E974C1842B1@lcwu.mtv.corp.google.com> Date: Sat, 11 Jun 2011 06:22:00 -0000 From: lcwu@google.com (Le-Chun Wu) X-System-Of-Record: true X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2011-06/txt/msg00891.txt.bz2 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