Hi, On Fri, Nov 04, 2011 at 08:01:41AM -0700, Delesley Hutchins wrote: > Thanks for the suggestion. Unfortunately, knowing the original > declaration doesn't help me; I also need to know the original > arguments that were passed at the call site, before those arguments > were removed by ipa-sra.
I see, that is tough. Once you re-base the analysis on 4.7, you might be able to use the new debugging stuff for this purpose: http://gcc.gnu.org/ml/gcc-patches/2011-06/msg00649.html Apart from that, I think that the information about the original actual arguments is indeed lost in the transformation. Martin > > > (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.) > > The problem is that the static analysis may be using the parameters, > even if those parameters are not used in the body of the function. > For example: > > void dummyLock (Mutex* mu) EXCLUSIVE_LOCK_FUNCTION(mu) { } > void dummyUnlock(Mutex* mu) UNLOCK_FUNCTION(mu) { } > > Mutex* mutex; > int a GUARDED_BY(mutex); > > void foo() { > // add mutex to set of held locks > dummyLock(mutex); // gets rewritten by ipa-sra to dummyLock(). Oops! > // okay to modify a, because we've "locked" mutex > a = 0; > // remove mutex from set of held locks > dummyUnlock(mutex); // gets rewritten by ipa-sra to dummyUnlock(). Oops! > } > > The annotations here tell the static analyzer to treat dummyLock and > dummyUnlock as valid lock functions, even though they don't > technically do anything. Such a pattern is not quite as deranged as > it may at first appear -- it is used, for example, when creating a > template class that may choose to either acquire a lock, or not, > depending on its template parameter. Ipa-sra kills the arguments, so > I no longer know which mutex was locked. > > -DeLesley > > > > 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 <deles...@google.com> > >> * 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 <deles...@google.com> > >> * g++.dg/thread-ann/thread_annot_lock-82.C: > >> Expanded regression test > >> > >> -- > >> DeLesley Hutchins | Software Engineer | deles...@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. */ > > > > > > > > -- > DeLesley Hutchins | Software Engineer | deles...@google.com | 505-206-0315
