Issue 4893047: Adding support for guardedby/ptguardedby
Patch Set 1 #
Total comments: 42
Patch Set 2 : Incorporating reviews and fixing bugs #
Total comments: 1
MessagesTotal messages: 4
http://codereview.appspot.com/4893047/diff/1/include/clang/Basic/DiagnosticSe... File include/clang/Basic/DiagnosticSemaKinds.td (right): http://codereview.appspot.com/4893047/diff/1/include/clang/Basic/DiagnosticSe... include/clang/Basic/DiagnosticSemaKinds.td:1340: "accessing variable '%0' requires lock '%1'">, Perhaps, "... requires mutex '%1' held exclusively" or "... requires exclusive lock on '%1'"? http://codereview.appspot.com/4893047/diff/1/include/clang/Basic/DiagnosticSe... include/clang/Basic/DiagnosticSemaKinds.td:1346: "dereferencing variable '%0' requires lock '%1'">, This is more technically correct as "accessing dereferenced variable...", but probably the wording you have is sufficient. http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.cpp File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:795: /// add a very coarse-grained value analysis to track some easy examples of What do you mean here? Give an example in the comment? http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:811: CheckDeclRefExprAccess(DR, /*Deref = */true); I hate boolean parameters. Make this an enum? http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:814: default: break; Comment that variable reads are handled in VisitCastExpr. http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:830: if (CE->getCastKind() != CK_LValueToRValue) Does this catch reads of class types, or just primitives? http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:836: /// Whenever we identify an access (read or right) of a DeclRefExpr, we need to s/right/write/ http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:839: /// also check for pt_guarded_by and guarded_var annotations. Do you mean "pt_guarded_var"?
Sign in to reply to this message.
http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.cpp File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:755: void CheckDeclRefExprAccess(DeclRefExpr *Exp, bool Deref = false); A followup change should rename all of these to start with a lower case letter. http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:794: /// need to check for pt_guarded_by annotations. In future, we may eventually In *the* future http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:809: DeclRefExpr *DR I usually attach the = to this line... However, why bother with the variable? Then you don't even need {}s http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:814: default: break; On 2011/08/19 23:16:51, Jeffrey Yasskin (google) wrote: > Comment that variable reads are handled in VisitCastExpr. Also, break on the next line. http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:840: void BuildLockset::CheckDeclRefExprAccess(DeclRefExpr *Exp, E or DRE is more consistent... http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:845: ValueDecl *D = Exp->getDecl(); const? http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:846: SourceLocation ExpLocation = Exp->getExprLoc(); What's the value of the variable? http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:848: if(!D || !D->hasAttrs()) I would do this immediately after the variable its checking. http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:851: AttrVec &ArgAttrs = D->getAttrs(); const? Also, sink this until right before its actually used. http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:859: for(unsigned int i = 0; i < ArgAttrs.size(); ++i) { s/unsigned int/unsigned/, and pull the size out http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:870: if(PtGuardedByAttr *PGB = dyn_cast<PtGuardedByAttr>(ArgAttrs[i])) { PGBAttr. http://codereview.appspot.com/4893047/diff/1/test/SemaCXX/warn-thread-safety-... File test/SemaCXX/warn-thread-safety-analysis.cpp (right): http://codereview.appspot.com/4893047/diff/1/test/SemaCXX/warn-thread-safety-... test/SemaCXX/warn-thread-safety-analysis.cpp:4: /************************************** These didn't get fixed? http://codereview.appspot.com/4893047/diff/1/test/SemaCXX/warn-thread-safety-... test/SemaCXX/warn-thread-safety-analysis.cpp:323: expected-warning {{accessing variable 'sls_guard_var' requires some lock}} please apply my style comments from the previous review.
Sign in to reply to this message.
Here is an updated patch set for guarded_by annotations. I need to talk to Chandler a bit about identifying reads and writes of non-primitive types, but otherwise I am hoping to commit this patch today, if possible. http://codereview.appspot.com/4893047/diff/1/include/clang/Basic/DiagnosticSe... File include/clang/Basic/DiagnosticSemaKinds.td (right): http://codereview.appspot.com/4893047/diff/1/include/clang/Basic/DiagnosticSe... include/clang/Basic/DiagnosticSemaKinds.td:1340: "accessing variable '%0' requires lock '%1'">, On 2011/08/19 23:16:51, Jeffrey Yasskin (google) wrote: > Perhaps, "... requires mutex '%1' held exclusively" or "... requires exclusive > lock on '%1'"? Changed to accessing variable '%0' requires mutex '%1' to be held http://codereview.appspot.com/4893047/diff/1/include/clang/Basic/DiagnosticSe... include/clang/Basic/DiagnosticSemaKinds.td:1346: "dereferencing variable '%0' requires lock '%1'">, On 2011/08/19 23:16:51, Jeffrey Yasskin (google) wrote: > This is more technically correct as "accessing dereferenced variable...", but > probably the wording you have is sufficient. Done. http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.cpp File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:755: void CheckDeclRefExprAccess(DeclRefExpr *Exp, bool Deref = false); On 2011/08/23 21:58:18, chandlerc wrote: > A followup change should rename all of these to start with a lower case letter. Ok. http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:794: /// need to check for pt_guarded_by annotations. In future, we may eventually On 2011/08/23 21:58:18, chandlerc wrote: > In *the* future Done. http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:795: /// add a very coarse-grained value analysis to track some easy examples of On 2011/08/19 23:16:51, Jeffrey Yasskin (google) wrote: > What do you mean here? Give an example in the comment? Done. http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:809: DeclRefExpr *DR Ah, ok. This section was refactored anyway due to an unrelated bug. On 2011/08/23 21:58:18, chandlerc wrote: > I usually attach the = to this line... > > However, why bother with the variable? Then you don't even need {}s http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:811: CheckDeclRefExprAccess(DR, /*Deref = */true); After discussion, we rearranged when pt_guarded_by annotations are checked and this became two methods. On 2011/08/19 23:16:51, Jeffrey Yasskin (google) wrote: > I hate boolean parameters. Make this an enum? http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:814: default: break; On 2011/08/19 23:16:51, Jeffrey Yasskin (google) wrote: > Comment that variable reads are handled in VisitCastExpr. Done. http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:814: default: break; On 2011/08/23 21:58:18, chandlerc wrote: > On 2011/08/19 23:16:51, Jeffrey Yasskin (google) wrote: > > Comment that variable reads are handled in VisitCastExpr. > > Also, break on the next line. Done. http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:830: if (CE->getCastKind() != CK_LValueToRValue) It does not catch reads of class types. For now, I added a FIXME, although I may know how to deal with this problem. On 2011/08/19 23:16:51, Jeffrey Yasskin (google) wrote: > Does this catch reads of class types, or just primitives? http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:836: /// Whenever we identify an access (read or right) of a DeclRefExpr, we need to On 2011/08/19 23:16:51, Jeffrey Yasskin (google) wrote: > s/right/write/ Done. http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:839: /// also check for pt_guarded_by and guarded_var annotations. On 2011/08/19 23:16:51, Jeffrey Yasskin (google) wrote: > Do you mean "pt_guarded_var"? Done. http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:840: void BuildLockset::CheckDeclRefExprAccess(DeclRefExpr *Exp, On 2011/08/23 21:58:18, chandlerc wrote: > E or DRE is more consistent... Refactored to take Expr *. http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:845: ValueDecl *D = Exp->getDecl(); On 2011/08/23 21:58:18, chandlerc wrote: > const? Done. http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:846: SourceLocation ExpLocation = Exp->getExprLoc(); On 2011/08/23 21:58:18, chandlerc wrote: > What's the value of the variable? Done. http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:848: if(!D || !D->hasAttrs()) On 2011/08/23 21:58:18, chandlerc wrote: > I would do this immediately after the variable its checking. Done. http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:851: AttrVec &ArgAttrs = D->getAttrs(); On 2011/08/23 21:58:18, chandlerc wrote: > const? > > Also, sink this until right before its actually used. Done. http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:859: for(unsigned int i = 0; i < ArgAttrs.size(); ++i) { On 2011/08/23 21:58:18, chandlerc wrote: > s/unsigned int/unsigned/, and pull the size out Done. http://codereview.appspot.com/4893047/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:870: if(PtGuardedByAttr *PGB = dyn_cast<PtGuardedByAttr>(ArgAttrs[i])) { On 2011/08/23 21:58:18, chandlerc wrote: > PGBAttr. Done. http://codereview.appspot.com/4893047/diff/1/test/SemaCXX/warn-thread-safety-... File test/SemaCXX/warn-thread-safety-analysis.cpp (right): http://codereview.appspot.com/4893047/diff/1/test/SemaCXX/warn-thread-safety-... test/SemaCXX/warn-thread-safety-analysis.cpp:4: /************************************** On 2011/08/23 21:58:18, chandlerc wrote: > These didn't get fixed? They did. This review was sent out a week ago. http://codereview.appspot.com/4893047/diff/1/test/SemaCXX/warn-thread-safety-... test/SemaCXX/warn-thread-safety-analysis.cpp:323: expected-warning {{accessing variable 'sls_guard_var' requires some lock}} Will send out a separate patch that mass-applies to all committed tests. On 2011/08/23 21:58:18, chandlerc wrote: > please apply my style comments from the previous review.
Sign in to reply to this message.
http://codereview.appspot.com/4893047/diff/7001/lib/Sema/AnalysisBasedWarning... File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4893047/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:926: /// \brief For unary operations which read and write a variable, we need to check Whoops, over 80 char. Fixed in my local copy.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||
