On 11-09-29 23:01 , Delesley Hutchins wrote:
This patch suppresses bogus warnings that are caused when annotalysis
is run with the IPA-SRA optimization (i.e. -O2 or higher). IPA-SRA
creates clones of functions in which some arguments have been eliminated,
even if those arguments are used in the thread safety annotations;
this renders the attributes on such functions invalid. Annotalysis will now
detect when such clones have been created, and suppress certain
warnings to avoid false positives.
Bootstrapped and passed gcc regression testsuite on
x86_64-unknown-linux-gnu. Okay for google/gcc-4_6?
-DeLesley
cp/Changelog.google-4_6:
2011-9-27 DeLesley Hutchins<deles...@google.com>
The date should be formatted as YYYY-MM-DD
* cp/tree-threadsafe-analyze.c (process_function_attrs)
detect clones and suppress warnings
You need ':' after the closing ')' and write the entry as a proper
statement (start with capital and end in '.').
Also, no need to add 'cp/', since this entry will go in
cp/ChangeLog.google-4_6.
testsuite/Changelog.google-4_6:
2011-9-27 DeLesley Hutchins<deles...@google.com>
* testsuite/g++.dg/thread-ann/thread_annot_lock-82.C
(new regression test)
Similar comments here.
Index: gcc/testsuite/g++.dg/thread-ann/thread_annot_lock-82.C
===================================================================
--- gcc/testsuite/g++.dg/thread-ann/thread_annot_lock-82.C (revision 0)
+++ gcc/testsuite/g++.dg/thread-ann/thread_annot_lock-82.C (revision 0)
@@ -0,0 +1,30 @@
+// Test template methods in the presence of cloned constructors.
+// Regression test for bugfix.
+// { dg-do compile }
+// { dg-options "-Wthread-safety -O3" }
+
+#include "thread_annot_common.h"
+
+class Foo;
+void do_something(void* a);
+
+class Foo {
+ Mutex mu_;
+
+ // with optimization turned on, ipa-sra should eliminate the hidden
+ // "this" argument, thus invalidating EXCLUSIVE_LOCKS_REQUIRED.
+ inline void clone_me_ipasra(Foo* f) EXCLUSIVE_LOCKS_REQUIRED(mu_) {
+ do_something(f);
+ }
+
+ void foo(Foo* f);
+};
+
+void Foo::foo(Foo* f) {
+ mu_.Lock();
+ // in the cloned version, it looks like the required lock is f->mu_
+ // we should detect this and ignore the attribute.
+ clone_me_ipasra(f);
+ mu_.Unlock();
+}
+
Index: gcc/tree-threadsafe-analyze.c
===================================================================
--- gcc/tree-threadsafe-analyze.c (revision 179371)
+++ gcc/tree-threadsafe-analyze.c (working copy)
@@ -2129,6 +2129,7 @@ process_function_attrs (gimple call, tree fdecl,
tree base_obj = NULL_TREE;
bool is_exclusive_lock;
bool is_trylock;
+ bool bad_fun_args = false;
s/bad_fun_args/optimized_args/ ?
gcc_assert (is_gimple_call (call));
@@ -2147,16 +2148,28 @@ process_function_attrs (gimple call, tree fdecl,
!= NULL_TREE)
current_bb_info->writes_ignored = false;
+ /* If the given function is a clone, and if some of the parameters of the
+ clone have been optimized away, then the function attribute is no longer
+ correct, and we should supress certain warnings. Clones are often created
s/supress/suppress/
+ when -fipa-sra is enabled, which happens by default at -O2 starting from
+ GCC-4.5. The clones could be created as early as when constructing SSA.
+ ipa-sra is particularly fond of optimizing away the "this" pointer,
+ which is a problem because that makes it impossible to determine the
+ base object, which then causes spurious errors. It's better to just
+ remain silent in this case. */
+ if ((TREE_CODE (TREE_TYPE (fdecl)) == FUNCTION_TYPE
+ || TREE_CODE (TREE_TYPE (fdecl)) == METHOD_TYPE) /* sanity check */
+&& (fdecl != DECL_ORIGIN (fdecl)) /* is it a clone? */
+&& (type_num_arguments (TREE_TYPE (fdecl)) != /* compare args */
+ type_num_arguments (TREE_TYPE (DECL_ORIGIN(fdecl)))))
gah, thunderbird is so bad with spaces. I suppose these predicates are
properly aligned. Need space before '(fdecl)'.
I'm half tempted to suggest that we should be asserting that we're
dealing with a FUNCTION_TYPE or a METHOD_TYPE, but this is OK too.
+ bad_fun_args = true;
+
/* If the function is a class member, the first argument of the function
(i.e. "this" pointer) would be the base object. Note that here we call
DECL_ORIGIN on fdecl first before we check whether it's a METHOD_TYPE
because if fdecl is a cloned method, the TREE_CODE of its type would be
FUNCTION_DECL instead of METHOD_DECL, which would lead us to not grab
- its base object. One possible situation where fdecl could be a clone is
- when -fipa-sra is enabled. (-fipa-sra is enabled by default at -O2
- starting from GCC-4.5.). The clones could be created as early as when
- constructing SSA. Also note that the parameters of a cloned method could
- be optimized away. */
+ its base object. */
if (TREE_CODE (TREE_TYPE (DECL_ORIGIN (fdecl))) == METHOD_TYPE
&& gimple_call_num_args(call)> 0)
base_obj = gimple_call_arg (call, 0);
@@ -2285,7 +2298,8 @@ process_function_attrs (gimple call, tree fdecl,
current_bb_info, locus);
}
- if (warn_thread_unguarded_func)
+ /* suppress warnings if the function arguments are no longer accurate. */
+ if (warn_thread_unguarded_func&& !bad_fun_args)
{
/* Handle the attributes specifying the lock requirements of
functions. */
OK with those changes.
Diego.
