[thread-annotations] [patch] Fix an issue in handling same lock field of different objects

[Le-Chun Wu <lcwu at google dot com>]

I just checked in this patch to the thread-annotations branch to fix
an issue where the thread-safety analysis would mistakenly consider
the same lock field of two different objects identical and emit bogus
warnings (or omit necessary warnings). The patch was bootstrapped and
tested on i686-linux-gnu.

Le-chun

2009-01-07  Le-Chun Wu  <lcwu@google.com>

        * tree-threadsafe-analyze.c (lock_set_contains): Check the lock set
        with the fully-qualified name if the lock is a field.
        * g++.dg/thread-ann/thread_annot_lock-35.C: New test.
        * g++.dg/thread-ann/thread_annot_lock-36.C: New test.


Index: gcc/tree-threadsafe-analyze.c
===================================================================
--- gcc/tree-threadsafe-analyze.c	(revision 143109)
+++ gcc/tree-threadsafe-analyze.c	(working copy)
@@ -974,21 +974,25 @@ lock_set_contains (const struct pointer_
       && pointer_set_contains (lock_set, error_mark_node))
     return lock;

-  /* Maybe the lock is already in canonical form. Try it first.  */
-  if (pointer_set_contains (lock_set, lock))
-    return lock;
-
-  if (leftmost_operand_is_field_decl (lock) && (base_obj != NULL_TREE))
+  /* If the lock is a field and the base is not 'this' pointer, we need to
+     check the lock set with the fully-qualified lock name. Otherwise, we
+     could be confused by the same lock field of a different object.  */
+  if (leftmost_operand_is_field_decl (lock)
+      && base_obj != NULL_TREE
+      && !is_base_object_this_pointer (base_obj))
     {
-      /* Get the canonical lock tree */
+      /* canonical lock is a fully-qualified name. */
       tree canonical_lock = get_canonical_expr (lock, base_obj,
                                                 true /* is_temp_expr */);
       tree result = (pointer_set_contains (lock_set, canonical_lock)
                      ? canonical_lock : NULL_TREE);
-      /* Since we know full_lock is not going to be used any more, we might
-         as well free it even though it's not necessary.  */
       return result;
     }
+  /* Check the lock set with the given lock directly as it could already be
+     in canonical form.  */
+  else if (pointer_set_contains (lock_set, lock))
+    return lock;
+  /* If the lock is not yet bound to a decl, try to bind it now.  */
   else if (TREE_CODE (lock) == IDENTIFIER_NODE)
     {
       void **entry;
Index: gcc/testsuite/g++.dg/thread-ann/thread_annot_lock-35.C
===================================================================
--- gcc/testsuite/g++.dg/thread-ann/thread_annot_lock-35.C	(revision 0)
+++ gcc/testsuite/g++.dg/thread-ann/thread_annot_lock-35.C	(revision 0)
@@ -0,0 +1,35 @@
+// Test the analyzer's ability to distinguish the lock field of different
+// objects.
+// { dg-do compile }
+// { dg-options "-Wthread-safety -Wthread-unsupported-lock-name -O" }
+
+#include "thread_annot_common.h"
+
+class Foo {
+ private:
+  Mutex lock_;
+  int a_ GUARDED_BY(lock_);
+
+ public:
+  void Func(Foo* child) LOCKS_EXCLUDED(lock_) {
+     Foo *new_foo = new Foo;
+
+     MutexLock l(&lock_);
+
+     child->Func(new_foo); // There shouldn't be any warning here as the
+                           // acquired lock is not in child.
+     child->bar(7); // { dg-warning "Calling function 'bar' requires lock" }
+     child->a_ = 5; // { dg-warning "Writing to variable 'child->a_'
requires lock" }
+  }
+
+  void bar(int y) EXCLUSIVE_LOCKS_REQUIRED(lock_) {
+    a_ = y;
+  }
+};
+
+Foo *x;
+
+main() {
+  Foo *child = new Foo;
+  x->Func(child);
+}
Index: gcc/testsuite/g++.dg/thread-ann/thread_annot_lock-36.C
===================================================================
--- gcc/testsuite/g++.dg/thread-ann/thread_annot_lock-36.C	(revision 0)
+++ gcc/testsuite/g++.dg/thread-ann/thread_annot_lock-36.C	(revision 0)
@@ -0,0 +1,36 @@
+// Test the analyzer's ability to distinguish the lock field of different
+// objects.
+// { dg-do compile }
+// { dg-options "-Wthread-safety -Wthread-unsupported-lock-name -O" }
+
+#include "thread_annot_common.h"
+
+class Foo {
+ private:
+  Mutex lock_;
+  int a_ GUARDED_BY(lock_);
+
+ public:
+  void Func(Foo* child) LOCKS_EXCLUDED(lock_) {
+     Foo *new_foo = new Foo;
+
+     MutexLock l(&lock_);
+
+     child->lock_.Lock();
+     child->Func(new_foo); // { dg-warning "Cannot call function
'Func' with lock 'child->lock_' held" }
+     child->bar(7);
+     child->a_ = 5;
+     child->lock_.Unlock();
+  }
+
+  void bar(int y) EXCLUSIVE_LOCKS_REQUIRED(lock_) {
+    a_ = y;
+  }
+};
+
+Foo *x;
+
+main() {
+  Foo *child = new Foo;
+  x->Func(child);
+}