From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31737 invoked by alias); 10 Jun 2011 18:17:52 -0000 Received: (qmail 31724 invoked by uid 22791); 10 Jun 2011 18:17:51 -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 18:17:35 +0000 Received: from wpaz33.hot.corp.google.com (wpaz33.hot.corp.google.com [172.24.198.97]) by smtp-out.google.com with ESMTP id p5AIHXN5019904; Fri, 10 Jun 2011 11:17:33 -0700 Received: from lcwu.mtv.corp.google.com (lcwu.mtv.corp.google.com [172.18.120.183]) by wpaz33.hot.corp.google.com with ESMTP id p5AIHVvT022073; Fri, 10 Jun 2011 11:17:31 -0700 Received: by lcwu.mtv.corp.google.com (Postfix, from userid 56351) id 32D0B1842B1; Fri, 10 Jun 2011 11:17:30 -0700 (PDT) To: reply@codereview.appspotmail.com, gcc-patches@gcc.gnu.org Subject: [google] Enhance Annotalysis to support cloned functions/methods (issue4591066) Message-Id: <20110610181731.32D0B1842B1@lcwu.mtv.corp.google.com> Date: Fri, 10 Jun 2011 18:31: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/msg00871.txt.bz2 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 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