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.
> (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
