Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(3039)

Issue 4275075: [google] Port lock annotations/analysis to google/main branch (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
13 years, 1 month ago by Le-Chun Wu
Modified:
13 years, 1 month ago
CC:
gcc-patches_gcc.gnu.org
Visibility:
Public.

Patch Set 1 #

Patch Set 2 : Retry uploading the patch set. #

Total comments: 10

Patch Set 3 : Incorporated Diego's review comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+11 lines, -4 lines) Patch
M gcc/common.opt View 1 2 2 chunks +4 lines, -4 lines 0 comments Download
M gcc/tree.h View 1 2 1 chunk +7 lines, -0 lines 0 comments Download

Messages

Total messages: 4
Le-Chun Wu
Port annotalysis (lock annotations and analysis) from GCC-4.4.3 source tree to (GCC-4.6 based) google/main branch ...
13 years, 1 month ago (2011-03-24 19:31:03 UTC) #1
Diego Novillo
Looks OK with some fixes below. Could you also update http://gcc.gnu.org/wiki/ThreadSafetyAnnotation? It still points to ...
13 years, 1 month ago (2011-03-24 21:42:18 UTC) #2
Diego Novillo
On Thu, Mar 24, 2011 at 15:30, Le-Chun Wu <lcwu@google.com> wrote: > 2011-03-24 Le-Chun Wu ...
13 years, 1 month ago (2011-03-24 21:43:53 UTC) #3
Le-Chun Wu
13 years, 1 month ago (2011-03-24 22:51:43 UTC) #4
> 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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b