From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8050 invoked by alias); 4 Nov 2011 10:39:32 -0000 Received: (qmail 8042 invoked by uid 22791); 4 Nov 2011 10:39:30 -0000 X-SWARE-Spam-Status: No, hits=-3.3 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from cantor2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 04 Nov 2011 10:39:15 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.221.2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx2.suse.de (Postfix) with ESMTP id DA56E8B013; Fri, 4 Nov 2011 11:39:13 +0100 (CET) Date: Fri, 04 Nov 2011 10:56:00 -0000 From: Martin Jambor To: Delesley Hutchins Cc: gcc-patches , Ollie Wild , Diego Novillo Subject: Re: [PATCH] [Annotalysis] Fix ICE caused by ipa-sra optimization. Message-ID: <20111104103925.GA2508@alvy.suse.cz> Mail-Followup-To: Delesley Hutchins , gcc-patches , Ollie Wild , Diego Novillo References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) 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-11/txt/msg00523.txt.bz2 Hi, On Thu, Nov 03, 2011 at 09:09:24AM -0700, Delesley Hutchins wrote: > Let's try this again; perhaps I should learn to use e-mail. :-) > > This patch fixes an ICE caused when the ipa-sra optimization deletes > function arguments that are referenced from within a thread safety > attribute.  It will attempt to detect this situation and recover > gracefully.  Since a graceful recovery is not always possible, an > optional warning (-Wwarn-thread-optimization) can be turned on to > inform the user that certain attributes have been removed by > optimization. I know basically nothing about the analysis (what it does, at what point it runs) but from looking at the patch, it seems you're basically running into problems when you don't have the parameters you expect from annotations in the function declaration. Have you considered examining DECL_ABSTRACT_ORIGIN? If non-NULL, the function you are looking at is some sort of a clone (created by ipa-sra, ipa-cp or ipa-split and others may follow) and DECL_ABSTRACT_ORIGIN is the original declaration with the original PARM_DECLs and stuff that might help you not only recover but perhaps even perform what you want to? (Of course, ipa-sra removes scalar parameters only when they are not used in the first place and so there should be nothing to analyze.) Martin > > Bootstrapped and passed gcc regression testsuite on > x86_64-unknown-linux-gnu.  Okay for google/gcc-4_6? > >  -DeLesley > > Changelog.google-4_6: > 2011-11-02  DeLesley Hutchins   >    * tree-threadsafe-analyze.c: >      Ignores invalid attributes, issues a warning, recovers gracefully. > * common.opt: > Adds new thread safety warning. > > testsuite/Changelog.google-4_6: > 2011-11-02  DeLesley Hutchins >    * g++.dg/thread-ann/thread_annot_lock-82.C: > Expanded regression test > > -- > DeLesley Hutchins | Software Engineer | delesley@google.com | 505-206-0315 > Index: testsuite/g++.dg/thread-ann/thread_annot_lock-82.C > =================================================================== > --- testsuite/g++.dg/thread-ann/thread_annot_lock-82.C (revision 180716) > +++ testsuite/g++.dg/thread-ann/thread_annot_lock-82.C (working copy) > @@ -1,7 +1,7 @@ > -// Test template methods in the presence of cloned constructors. > -// Regression test for bugfix. > +// Regression tests: fix ICE issues when IPA-SRA deletes formal > +// function parameters. > // { dg-do compile } > -// { dg-options "-Wthread-safety -O3" } > +// { dg-options "-Wthread-safety -Wthread-warn-optimization -O3" } > > #include "thread_annot_common.h" > > @@ -10,6 +10,7 @@ void do_something(void* a); > > class Foo { > Mutex mu_; > + int a GUARDED_BY(mu_); > > // with optimization turned on, ipa-sra should eliminate the hidden > // "this" argument, thus invalidating EXCLUSIVE_LOCKS_REQUIRED. > @@ -18,6 +19,7 @@ class Foo { > } > > void foo(Foo* f); > + void bar(); > }; > > void Foo::foo(Foo* f) { > @@ -28,3 +30,17 @@ void Foo::foo(Foo* f) { > mu_.Unlock(); > } > > + > +class SCOPED_LOCKABLE DummyMutexLock { > +public: > + // IPA-SRA should kill the parameters to these functions > + explicit DummyMutexLock(Mutex* mutex) EXCLUSIVE_LOCK_FUNCTION(mutex) {} > + ~DummyMutexLock() UNLOCK_FUNCTION() {} > +}; > + > + > +void Foo::bar() { > + // Matches two warnings: > + DummyMutexLock dlock(&mu_); // { dg-warning "attribute has been removed by optimization." } > + a = 1; // warning here should be suppressed, due to errors handling dlock > +} > Index: common.opt > =================================================================== > --- common.opt (revision 180716) > +++ common.opt (working copy) > @@ -680,6 +680,10 @@ Wthread-attr-bind-param > Common Var(warn_thread_attr_bind_param) Init(1) Warning > Make the thread safety analysis try to bind the function parameters used in the attributes > > +Wthread-warn-optimization > +Common Var(warn_thread_optimization) Init(0) Warning > +Warn when optimizations invalidate the thread safety analysis. > + > Wtype-limits > Common Var(warn_type_limits) Init(-1) Warning > Warn if a comparison is always true or always false due to the limited range of the data type > Index: tree-threadsafe-analyze.c > =================================================================== > --- tree-threadsafe-analyze.c (revision 180716) > +++ tree-threadsafe-analyze.c (working copy) > @@ -1594,7 +1594,10 @@ get_actual_argument_from_position (gimple call, tr > > lock_pos = TREE_INT_CST_LOW (pos_arg); > > - gcc_assert (lock_pos >= 1 && lock_pos <= num_args); > + /* The ipa-sra optimization can occasionally delete arguments, thus > + invalidating the index. */ > + if (lock_pos < 1 || lock_pos > num_args) > + return NULL_TREE; > > /* The lock position specified in the attributes is 1-based, so we need to > subtract 1 from it when accessing the call arguments. */ > @@ -1734,7 +1737,27 @@ handle_lock_primitive_attrs (gimple call, tree fde > a formal parameter, we need to grab the corresponding actual argument > of the call. */ > else if (TREE_CODE (arg) == INTEGER_CST) > - arg = get_actual_argument_from_position (call, arg); > + { > + arg = get_actual_argument_from_position (call, arg); > + /* If ipa-sra has rewritten the call, then recover as gracefully as > + possible. */ > + if (!arg) > + { > + /* Add the universal lock to the lockset to suppress subsequent > + errors. */ > + if (is_exclusive_lock) > + pointer_set_insert(live_excl_locks, error_mark_node); > + else > + pointer_set_insert(live_shared_locks, error_mark_node); > + > + if (warn_thread_optimization) > + warning_at (*locus, OPT_Wthread_safety, > + G_("Lock attribute has been removed by " > + "optimization.")); > + > + return; > + } > + } > else if (TREE_CODE (get_leftmost_base_var (arg)) == PARM_DECL) > { > tree new_base > @@ -1890,7 +1913,18 @@ handle_unlock_primitive_attr (gimple call, tree fd > a formal parameter, we need to grab the corresponding actual argument > of the call. */ > if (TREE_CODE (arg) == INTEGER_CST) > - lockable = get_actual_argument_from_position (call, arg); > + { > + lockable = get_actual_argument_from_position (call, arg); > + /* If ipa-sra has rewritten the call, then fail as gracefully as > + possible -- just leave the lock in the lockset. */ > + if (!lockable) { > + if (warn_thread_optimization) > + warning_at (*locus, OPT_Wthread_safety, > + G_("Unlock attribute has been removed by " > + "optimization.")); > + return; > + } > + } > else if (TREE_CODE (get_leftmost_base_var (arg)) == PARM_DECL) > { > tree fdecl = gimple_call_fndecl (call); > @@ -1908,7 +1942,15 @@ handle_unlock_primitive_attr (gimple call, tree fd > } > else > { > - gcc_assert (base_obj); > + /* If ipa-sra has killed arguments, then base_obj may be NULL. > + Attempt to recover gracefully by leaving the lock in the lockset. */ > + if (!base_obj) { > + if (warn_thread_optimization) > + warning_at (*locus, OPT_Wthread_safety, > + G_("Unlock attribute has been removed by " > + "optimization.")); > + return; > + } > > /* Check if the primitive is an unlock routine (e.g. the destructor or > a release function) of a scoped_lock. If so, get the lock that is > @@ -2099,6 +2141,9 @@ handle_function_lock_requirement (gimple call, tre > else if (TREE_CODE (lock) == INTEGER_CST) > { > lock = get_actual_argument_from_position (call, lock); > + /* Ignore attribute if ipa-sra has killed the argument. */ > + if (!lock) > + return; > /* If the lock is a function argument, we don't want to > prepend the base object to the lock name. Set the > TMP_BASE_OBJ to NULL. */