New patch with suggested changes incorporated. Warning now on by
default, asserts put back if ipa-sra not enabled, and style fixes.
-DeLesley
On Thu, Nov 3, 2011 at 11:06 AM, Diego Novillo <dnovi...@google.com> wrote:
> On Thu, Nov 3, 2011 at 12:41, Delesley Hutchins <deles...@google.com> wrote:
>>> I wonder... how about disabling IPA-SRA when annotalysis is enabled?
>>
>> That would certainly make my life a lot easier -- IPA-SRA is not
>> really compatible with annotalysis. The only problem I can see is
>> that people may be unhappy if turning on annotalysis makes their code
>> runs slower. We need a good way to communicate the issue to users.
>> Is there a way to print a note that will be reported by the build
>> system, e.g. "Thread safety analysis enabled, some optimizations are
>> disabled?"
>
> Certainly, you could call inform() with a diagnostic. Though, it's
> true that analysis should not affect optimization levels. I think I
> prefer making the analysis tolerate the optimizers than affecting code
> generation.
>
>>
>>> Also, why not turn OPT_Wthread_safety on by default?
>>
>> We certainly could. However, it generates extra warnings, which may
>> break the build for some users. Which would users prefer: knowing
>> that annotalysis is suppressing warnings, or having to update their
>> build files to pass in an extra flag to turn off the warnings?
>
> What is the effect of your patch on annotalysis warnings? Does it
> produce false negatives? If so, I think we should turn this warning
> by default. I'd rather break the build than let a false negative
> through.
>
>
> Diego.
>
--
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,5 +1,5 @@
-// 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" }
@@ -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(1) 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,15 @@ 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)
+ {
+ if (flag_ipa_sra)
+ return NULL_TREE; /* Attempt to recover gracefully. */
+ else
+ gcc_unreachable ();
+ }
/* 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 +1742,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 +1918,19 @@ 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 +1948,18 @@ 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 (!flag_ipa_sra)
+ gcc_unreachable ();
+ else 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 +2150,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. */
