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

[Diego Novillo <dnovillo at google dot com>]

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.