Message ID | CAF6KqwX_pkGn9mWis_fAa7v2Wz69yjeTwPoqocX2OeqC_SQ4nQ@mail.gmail.com |
---|---|
State | New |
Headers | show |
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
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.
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