[PATCH] [Annotalysis] Support trylock attributes on virtual methods.

[PATCH] [Annotalysis] Support trylock attributes on virtual methods.

[Delesley Hutchins]

[-- Attachment #1: Type: text/plain, Size: 613 bytes --]

This patch fixes a bug wherein the trylock attribute would not work if
it was attached to a virtual method.

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-08  DeLesley Hutchins  <delesley@google.com>
   * tree-threadsafe-analyze.c:
     factors out code to get function decl.

testsuite/Changelog.google-4_6:
2011-11-08  DeLesley Hutchins <delesley@google.com>
   * g++.dg/thread-ann/thread_annot_lock-85.C:
    New regression test

-- 
DeLesley Hutchins | Software Engineer | delesley@google.com | 505-206-0315

[-- Attachment #2: gcc-virtual-trylock.patch --]
[-- Type: text/x-patch, Size: 2941 bytes --]

Index: testsuite/g++.dg/thread-ann/thread_annot_lock-85.C
===================================================================
--- testsuite/g++.dg/thread-ann/thread_annot_lock-85.C	(revision 0)
+++ testsuite/g++.dg/thread-ann/thread_annot_lock-85.C	(revision 0)
@@ -0,0 +1,22 @@
+// Regression test, handle trylock on virtual method. 
+// { dg-do compile }
+// { dg-options "-Wthread-safety" }
+
+#include "thread_annot_common.h"
+
+class LOCKABLE Lock {
+ public:
+ virtual ~Lock() {}
+ virtual bool TryLock() EXCLUSIVE_TRYLOCK_FUNCTION(true) { return true; }
+ void Unlock() UNLOCK_FUNCTION() {}
+};
+
+
+void foo() {
+  Lock x;
+  Lock *p = &x;
+  if (p->TryLock()) {
+    p->Unlock();
+  }
+}
+
Index: tree-threadsafe-analyze.c
===================================================================
--- tree-threadsafe-analyze.c	(revision 180984)
+++ tree-threadsafe-analyze.c	(working copy)
@@ -2508,26 +2508,14 @@
   return;
 }
 
-/* The main routine that handles gimple call statements.  */
 
-static void
-handle_call_gs (gimple call, struct bb_threadsafe_info *current_bb_info)
-{
+/* Get the function declaration from a gimple call stmt.  This handles both
+   ordinary function calls and virtual methods. */
+static tree
+get_fdecl_from_gimple_stmt (gimple call) {
   tree fdecl = gimple_call_fndecl (call);
-  int num_args = gimple_call_num_args (call);
-  int arg_index = 0;
-  tree arg_type = NULL_TREE;
-  tree arg;
-  tree lhs;
-  location_t locus;
-
-  if (!gimple_has_location (call))
-    locus = input_location;
-  else
-    locus = gimple_location (call);
-
   /* If the callee fndecl is NULL, check if it is a virtual function,
-     and if so, try to get its decl through the reference object.  */
+       and if so, try to get its decl through the reference object.  */
   if (!fdecl)
     {
       tree callee = gimple_call_fn (call);
@@ -2546,7 +2534,28 @@
               fdecl = lang_hooks.get_virtual_function_decl (callee, objtype);                    
         }
     }
+  return fdecl;
+}
 
+
+/* The main routine that handles gimple call statements.  */
+
+static void
+handle_call_gs (gimple call, struct bb_threadsafe_info *current_bb_info)
+{
+  tree fdecl = get_fdecl_from_gimple_stmt (call);
+  int num_args = gimple_call_num_args (call);
+  int arg_index = 0;
+  tree arg_type = NULL_TREE;
+  tree arg;
+  tree lhs;
+  location_t locus;
+
+  if (!gimple_has_location (call))
+    locus = input_location;
+  else
+    locus = gimple_location (call);
+
   /* The callee fndecl could be NULL, e.g., when the function is passed in
      as an argument.  */
   if (fdecl)
@@ -2839,7 +2848,8 @@
         }
       else if (is_gimple_call (gs))
         {
-          tree fdecl = gimple_call_fndecl (gs);
+          tree fdecl = get_fdecl_from_gimple_stmt (gs);
+
           /* The function decl could be null in some cases, e.g.
              a function pointer passed in as a parameter.  */
           if (fdecl

Re: [PATCH] [Annotalysis] Support trylock attributes on virtual methods.

[Ollie Wild]

On Tue, Nov 8, 2011 at 12:11 PM, Delesley Hutchins <delesley@google.com> wrote:
> This patch fixes a bug wherein the trylock attribute would not work if
> it was attached to a virtual method.

Diego, can you please review this?

Thanks,
Ollie

Re: [PATCH] [Annotalysis] Support trylock attributes on virtual methods.

[Diego Novillo]

On 11-11-08 13:11 , Delesley Hutchins wrote:
> This patch fixes a bug wherein the trylock attribute would not work if
> it was attached to a virtual method.
>
> 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-08  DeLesley Hutchins<delesley@google.com>
>     * tree-threadsafe-analyze.c:
>       factors out code to get function decl.

Fix formatting.  Blank line after date.  Specify the name of the 
modified function in '()'.

>
> testsuite/Changelog.google-4_6:
> 2011-11-08  DeLesley Hutchins<delesley@google.com>
>     * g++.dg/thread-ann/thread_annot_lock-85.C:
>      New regression test
>

Blank line after date.


> Index: testsuite/g++.dg/thread-ann/thread_annot_lock-85.C
> ===================================================================
> --- testsuite/g++.dg/thread-ann/thread_annot_lock-85.C	(revision 0)
> +++ testsuite/g++.dg/thread-ann/thread_annot_lock-85.C	(revision 0)
> @@ -0,0 +1,22 @@
> +// Regression test, handle trylock on virtual method.
> +// { dg-do compile }
> +// { dg-options "-Wthread-safety" }
> +
> +#include "thread_annot_common.h"
> +
> +class LOCKABLE Lock {
> + public:
> + virtual ~Lock() {}
> + virtual bool TryLock() EXCLUSIVE_TRYLOCK_FUNCTION(true) { return true; }
> + void Unlock() UNLOCK_FUNCTION() {}
> +};
> +
> +
> +void foo() {
> +  Lock x;
> +  Lock *p = &x;
> +  if (p->TryLock()) {
> +    p->Unlock();
> +  }
> +}
> +
> Index: tree-threadsafe-analyze.c
> ===================================================================
> --- tree-threadsafe-analyze.c	(revision 180984)
> +++ tree-threadsafe-analyze.c	(working copy)
> @@ -2508,26 +2508,14 @@
>    return;
>  }
>
> -/* The main routine that handles gimple call statements.  */
>
> -static void
> -handle_call_gs (gimple call, struct bb_threadsafe_info *current_bb_info)
> -{
> +/* Get the function declaration from a gimple call stmt.  This handles both
> +   ordinary function calls and virtual methods. */
> +static tree
> +get_fdecl_from_gimple_stmt (gimple call) {

Opening brace on line below.
Reference 'call' argument in capitals.

>    tree fdecl = gimple_call_fndecl (call);
> -  int num_args = gimple_call_num_args (call);
> -  int arg_index = 0;
> -  tree arg_type = NULL_TREE;
> -  tree arg;
> -  tree lhs;
> -  location_t locus;
> -
> -  if (!gimple_has_location (call))
> -    locus = input_location;
> -  else
> -    locus = gimple_location (call);
> -
>    /* If the callee fndecl is NULL, check if it is a virtual function,
> -     and if so, try to get its decl through the reference object.  */
> +       and if so, try to get its decl through the reference object.  */
>    if (!fdecl)
>      {
>        tree callee = gimple_call_fn (call);
> @@ -2546,7 +2534,28 @@
>                fdecl = lang_hooks.get_virtual_function_decl (callee, objtype);
>          }
>      }
> +  return fdecl;
> +}
>
> +
> +/* The main routine that handles gimple call statements.  */
> +
> +static void
> +handle_call_gs (gimple call, struct bb_threadsafe_info *current_bb_info)

Since you are modifying this code, could you add documentation for CALL 
and CURRENT_BB_INFO?

> +{
> +  tree fdecl = get_fdecl_from_gimple_stmt (call);
> +  int num_args = gimple_call_num_args (call);
> +  int arg_index = 0;
> +  tree arg_type = NULL_TREE;
> +  tree arg;
> +  tree lhs;
> +  location_t locus;
> +
> +  if (!gimple_has_location (call))
> +    locus = input_location;
> +  else
> +    locus = gimple_location (call);
> +
>    /* The callee fndecl could be NULL, e.g., when the function is passed in
>       as an argument.  */
>    if (fdecl)
> @@ -2839,7 +2848,8 @@
>          }
>        else if (is_gimple_call (gs))
>          {
> -          tree fdecl = gimple_call_fndecl (gs);
> +          tree fdecl = get_fdecl_from_gimple_stmt (gs);
> +

What about the other spots where we call gimple_call_fndecl, shouldn't 
we call get_fdecl_from_gimple_stmt instead?


Diego.

Re: [PATCH] [Annotalysis] Support trylock attributes on virtual methods.

[Delesley Hutchins]

[-- Attachment #1: Type: text/plain, Size: 318 bytes --]

> What about the other spots where we call gimple_call_fndecl, shouldn't we
> call get_fdecl_from_gimple_stmt instead?

Good point.  I've gone through and updated the necessary spots.

New patch with all fixes is attached.

  -DeLesley



-- 
DeLesley Hutchins | Software Engineer | delesley@google.com | 505-206-0315

[-- Attachment #2: gcc-virtual-trylock2.patch --]
[-- Type: text/x-patch, Size: 6154 bytes --]

Index: testsuite/g++.dg/thread-ann/thread_annot_lock-85.C
===================================================================
--- testsuite/g++.dg/thread-ann/thread_annot_lock-85.C	(revision 0)
+++ testsuite/g++.dg/thread-ann/thread_annot_lock-85.C	(revision 0)
@@ -0,0 +1,22 @@
+// Regression test, handle trylock on virtual method. 
+// { dg-do compile }
+// { dg-options "-Wthread-safety" }
+
+#include "thread_annot_common.h"
+
+class LOCKABLE Lock {
+ public:
+ virtual ~Lock() {}
+ virtual bool TryLock() EXCLUSIVE_TRYLOCK_FUNCTION(true) { return true; }
+ void Unlock() UNLOCK_FUNCTION() {}
+};
+
+
+void foo() {
+  Lock x;
+  Lock *p = &x;
+  if (p->TryLock()) {
+    p->Unlock();
+  }
+}
+
Index: tree-threadsafe-analyze.c
===================================================================
--- tree-threadsafe-analyze.c	(revision 181268)
+++ tree-threadsafe-analyze.c	(working copy)
@@ -564,6 +564,38 @@
     return false;
 }
 
+
+/* Get the function declaration from a gimple call stmt (CALL).  This handles
+   both ordinary function calls and virtual methods. */
+
+static tree
+get_fdecl_from_gimple_stmt (gimple call)
+{
+  tree fdecl = gimple_call_fndecl (call);
+  /* If the callee fndecl is NULL, check if it is a virtual function,
+       and if so, try to get its decl through the reference object.  */
+  if (!fdecl)
+    {
+      tree callee = gimple_call_fn (call);
+      if (TREE_CODE (callee) == OBJ_TYPE_REF)
+        {
+          tree objtype = TREE_TYPE (TREE_TYPE (OBJ_TYPE_REF_OBJECT (callee)));
+          /* Check to make sure objtype is a valid type.
+             OBJ_TYPE_REF_OBJECT does not always return the correct static type of the callee.
+             For example:  Given  foo(void* ptr) { ((Foo*) ptr)->doSomething(); }
+             objtype will be void, not Foo.  Whether or not this happens depends on the details
+             of how a particular call is lowered to GIMPLE, and there is no easy fix that works
+             in all cases.  For now, we simply rely on gcc's type information; if that information
+             is not accurate, then the analysis will be less precise.
+           */
+          if (TREE_CODE (objtype) == RECORD_TYPE)
+              fdecl = lang_hooks.get_virtual_function_decl (callee, objtype);
+        }
+    }
+  return fdecl;
+}
+
+
 /* Given a CALL gimple statment, check if its function decl is annotated
    with "lock_returned" attribute. If so, return the lock specified in
    the attribute. Otherise, return NULL_TREE.  */
@@ -571,7 +603,7 @@
 static tree
 get_lock_returned_by_call (gimple call)
 {
-  tree fdecl = gimple_call_fndecl (call);
+  tree fdecl = get_fdecl_from_gimple_stmt (call);
   tree attr = (fdecl
                ? lookup_attribute ("lock_returned", DECL_ATTRIBUTES (fdecl))
                : NULL_TREE);
@@ -828,7 +860,7 @@
                 }
               else if (is_gimple_call (def_stmt))
                 {
-                  tree fdecl = gimple_call_fndecl (def_stmt);
+                  tree fdecl = get_fdecl_from_gimple_stmt (def_stmt);
                   tree real_lock = get_lock_returned_by_call (def_stmt);
                   if (real_lock)
                     {
@@ -1661,6 +1693,7 @@
   gcc_unreachable ();
 }
 
+
 /* A helper function that adds the LOCKABLE, acquired by CALL, to the
    corresponding lock sets (LIVE_EXCL_LOCKS or LIVE_SHARED_LOCKS) depending
    on the boolean parameter IS_EXCLUSIVE_LOCK. If the CALL is a trylock call,
@@ -1967,7 +2000,6 @@
         }
       else if (TREE_CODE (get_leftmost_base_var (arg)) == PARM_DECL)
         {
-          tree fdecl = gimple_call_fndecl (call);
           tree new_base = get_actual_argument_from_parameter (
               call, fdecl, get_leftmost_base_var (arg));
           lockable = get_canonical_lock_expr (arg, NULL_TREE, false, new_base);
@@ -2542,12 +2574,17 @@
   return;
 }
 
-/* The main routine that handles gimple call statements.  */
 
+/* The main routine that handles gimple call statements.  This will update
+   the set of held locks.
+   CALL            -- the gimple call statement.
+   CURRENT_BB_INFO -- a pointer to the lockset structures for the current
+                      basic block.
+*/
 static void
 handle_call_gs (gimple call, struct bb_threadsafe_info *current_bb_info)
 {
-  tree fdecl = gimple_call_fndecl (call);
+  tree fdecl = get_fdecl_from_gimple_stmt (call);
   int num_args = gimple_call_num_args (call);
   int arg_index = 0;
   tree arg_type = NULL_TREE;
@@ -2560,27 +2597,6 @@
   else
     locus = gimple_location (call);
 
-  /* If the callee fndecl is NULL, check if it is a virtual function,
-     and if so, try to get its decl through the reference object.  */
-  if (!fdecl)
-    {
-      tree callee = gimple_call_fn (call);
-      if (TREE_CODE (callee) == OBJ_TYPE_REF)
-        {
-          tree objtype = TREE_TYPE (TREE_TYPE (OBJ_TYPE_REF_OBJECT (callee)));
-          /* Check to make sure objtype is a valid type.
-             OBJ_TYPE_REF_OBJECT does not always return the correct static type of the callee.   
-             For example:  Given  foo(void* ptr) { ((Foo*) ptr)->doSomething(); }
-             objtype will be void, not Foo.  Whether or not this happens depends on the details 
-             of how a particular call is lowered to GIMPLE, and there is no easy fix that works 
-             in all cases.  For now, we simply rely on gcc's type information; if that information 
-             is not accurate, then the analysis will be less precise.
-           */
-          if (TREE_CODE (objtype) == RECORD_TYPE)
-              fdecl = lang_hooks.get_virtual_function_decl (callee, objtype);                    
-        }
-    }
-
   /* The callee fndecl could be NULL, e.g., when the function is passed in
      as an argument.  */
   if (fdecl)
@@ -2873,7 +2889,8 @@
         }
       else if (is_gimple_call (gs))
         {
-          tree fdecl = gimple_call_fndecl (gs);
+          tree fdecl = get_fdecl_from_gimple_stmt (gs);
+
           /* The function decl could be null in some cases, e.g.
              a function pointer passed in as a parameter.  */
           if (fdecl

Re: [PATCH] [Annotalysis] Support trylock attributes on virtual methods.

[Diego Novillo]

On 11-11-10 17:23 , Delesley Hutchins wrote:

> +    {
> +      tree callee = gimple_call_fn (call);
> +      if (TREE_CODE (callee) == OBJ_TYPE_REF)
> +        {
> +          tree objtype = TREE_TYPE (TREE_TYPE (OBJ_TYPE_REF_OBJECT (callee)));
> +          /* Check to make sure objtype is a valid type.
> +             OBJ_TYPE_REF_OBJECT does not always return the correct static type of the callee.
> +             For example:  Given  foo(void* ptr) { ((Foo*) ptr)->doSomething(); }
> +             objtype will be void, not Foo.  Whether or not this happens depends on the details
> +             of how a particular call is lowered to GIMPLE, and there is no easy fix that works
> +             in all cases.  For now, we simply rely on gcc's type information; if that information
> +             is not accurate, then the analysis will be less precise.

Re-format for 80 cols.

OK with that change.


Diego.