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 <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. */
