Re: [google] Enhance Annotalysis to support cloned functions/methods (issue4591066)

[lcwu]

http://codereview.appspot.com/4591066/diff/3001/gcc/tree-threadsafe-analyze.c
File gcc/tree-threadsafe-analyze.c (right):

http://codereview.appspot.com/4591066/diff/3001/gcc/tree-threadsafe-analyze.c#newcode1159
gcc/tree-threadsafe-analyze.c:1159: gcc_assert (false);
On 2011/06/11 17:52:51, Diego Novillo wrote:

>+ else
>+  gcc_assert (false);
>+



Change to gcc_unreachable ();


Done.

http://codereview.appspot.com/4591066/diff/3001/gcc/tree-threadsafe-analyze.c#newcode2151
gcc/tree-threadsafe-analyze.c:2151: && gimple_call_num_args(call) > 0)
On 2011/06/11 17:52:51, Diego Novillo wrote:

2147  starting from GCC-4.5.). The clones could be created as

early as when

  2148  constructing SSA. Also note that the parameters of a cloned

method

could
  2149  be optimized away.  */
  2150   if (TREE_CODE (TREE_TYPE (DECL_ORIGIN (fdecl))) == METHOD_TYPE
  2151   && gimple_call_num_args(call) > 0)



Wouldn't it be easier to make fdecl == DECL_ORIGIN (fdecl) earlier in

the

function?



It's OK either way, though.


Yes, my original fix was to make fdecl = DECL_ORIGINAL (fdecl). But I
later changed it to this way because I wanted to tolerate the case where
the base object (i.e. "this" pointer) is an object instead of a pointer
only when fdecl is a clone. (i.e. I don't want to arbitrarily relax it.)
That's why I kept fdecl intact.

http://codereview.appspot.com/4591066/

Re: [google] Port lock annotations/analysis to google/main branch (issue4275075)

[lcwu]

Could you also update http://gcc.gnu.org/wiki/ThreadSafetyAnnotation?
It still points to the old branch and seems to have stale content.


Will do.



Any plans for mainline merge?


I don't actually have a time frame for that, but that is the ultimate
goal.



http://codereview.appspot.com/4275075/diff/2001/gcc/common.opt
File gcc/common.opt (right):

http://codereview.appspot.com/4275075/diff/2001/gcc/common.opt#newcode682
gcc/common.opt:682: Make the thread safety analysis try to bind the
function parameters used in the attributes
On 2011/03/24 21:42:18, Diego Novillo wrote:

Hmm, you've added these warnings twice now.  I had added the flags to

fix our

builds.  The warnings I added are just above these.  If there's

anything new,

you can just overwrite it.


Ah, I didn't notice that, and the build went OK. Duplicate entries were
removed.

http://codereview.appspot.com/4275075/diff/2001/gcc/cp/parser.c
File gcc/cp/parser.c (left):

http://codereview.appspot.com/4275075/diff/2001/gcc/cp/parser.c#oldcode5766
gcc/cp/parser.c:5766: is_attribute_list = non_attr;
On 2011/03/24 21:42:18, Diego Novillo wrote:

Why are you taking this out?


Without doing, the parser would start evaluating/binding the identifier
nodes after processing the first argument, which would limit our ability
to allow users to use names not in scope. The detailed explanation is in
the comments above (line 5778 to 5801 in the patched file).

http://codereview.appspot.com/4275075/diff/2001/gcc/cp/parser.c#oldcode5802
gcc/cp/parser.c:5802: VEC_safe_insert (tree, gc, expression_list, 0,
identifier);
On 2011/03/24 21:42:18, Diego Novillo wrote:

Likewise.


In the original implementation, the first identifier argument was not
pushed into the expression_list vector, so we had to do that here. With
my patch, the first argument is pushed to the vector in line 5802 (in
patched file).

http://codereview.appspot.com/4275075/diff/2001/gcc/cp/parser.c
File gcc/cp/parser.c (right):

http://codereview.appspot.com/4275075/diff/2001/gcc/cp/parser.c#newcode1742
gcc/cp/parser.c:1742: tree current_declarator_scope;
On 2011/03/24 21:42:18, Diego Novillo wrote:

You're going to find some amusing merge conflicts the next time

google/main

merges from trunk ;)  All this has been factored out of cp/parser.c


Thanks for the heads-up. We will deal with them then. :-)

http://codereview.appspot.com/4275075/diff/2001/gcc/tree.h
File gcc/tree.h (right):

http://codereview.appspot.com/4275075/diff/2001/gcc/tree.h#newcode5366
gcc/tree.h:5366: /* Extract and return all lock attributes from the
given attribute list.  */
On 2011/03/24 21:42:18, Diego Novillo wrote:

Blank line above comment.


Done.

http://codereview.appspot.com/4275075/