Issue 4955041: Shared locks
|
Created:
13 years, 5 months ago by supertri Modified:
3 years, 5 months ago CC:
Ollie Wild Visibility:
Public. |
Patch Set 1 #
Total comments: 31
Patch Set 2 : Updated based on edits by reviewers, plus separated #
Total comments: 26
Patch Set 3 : Addressed reviewer comments, inc. new warning for different lock kinds #Patch Set 4 : Fix enum names #
MessagesTotal messages: 8
http://codereview.appspot.com/4955041/diff/1/include/clang/Basic/DiagnosticSe... File include/clang/Basic/DiagnosticSemaKinds.td (right): http://codereview.appspot.com/4955041/diff/1/include/clang/Basic/DiagnosticSe... include/clang/Basic/DiagnosticSemaKinds.td:1402: "expecting lock '%0' to be held at start of each loop">, What happens if someone changes whether a lock is held shared vs. exclusive inside a loop? http://codereview.appspot.com/4955041/diff/1/include/clang/Basic/DiagnosticSe... include/clang/Basic/DiagnosticSemaKinds.td:1419: "calling function '%0' requires %select{shared|exclusive}2 lock '%1'">, Is it possible to separate the lock-required annotation patch from the shared-lock implementation? http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.cpp File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:774: enum AccType { Spell out Access please. http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:789: // "upgrade" or "downgrade" between these two options. FWIW, "upgrade" locks don't require reentrancy: http://www.boost.org/doc/libs/1_47_0/doc/html/thread/synchronization.html#thr... http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:793: // the exclusive or shared version of the lock. I don't quite understand this sentence. Can't you look up the mutex in the lockset, and then just read the Locktype? http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:840: bool LocksetContains(LockID Lock, LockType LockRequested = SHARED) const { I would probably avoid the default argument here. It's nice to be explicit about how we want the lock to be held. http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:845: if (LockRequested == SHARED || LockHeld->Locktype == EXCLUSIVE) Why not "LockHeld->Locktype >= LockRequested"? and then just return the expression rather than using "if (x) return true; else return false;" http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:875: // version of the a lock with this expression s/the a/the/ http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:879: // update LSet to use the newer LockData Capitalize and punctuate comments! http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:974: switch (AT) { This looks suspiciously similar to the block you added to checkDereference(). Can you share them? http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:1042: /// FIXME: We need to also visit CallExprs (i.e. for global functions). i.e. seems odd here. "visit CallExprs to catch/check global functions."? http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:1083: addLock(ExpLocation, *I, SHARED); Can you share this logic with the ExclusiveLockFunction case? http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:1088: // When we encounter an exclusive trylock function, we need to add it You can probably break this out into a separate patch. This is the kind of thing I wish you were using public git branches for. Then you could present a patch series, and I could keep reviewing until I got tired, but they'd be clearly split into pieces so I'd review related things together. </not-actionable> http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:1174: for (SharedLocksRequiredAttr::args_iterator I = SLRAttr->args_begin(), Again, share the logic when possible. http://codereview.appspot.com/4955041/diff/1/test/SemaCXX/warn-thread-safety-... File test/SemaCXX/warn-thread-safety-analysis.cpp (right): http://codereview.appspot.com/4955041/diff/1/test/SemaCXX/warn-thread-safety-... test/SemaCXX/warn-thread-safety-analysis.cpp:437: // expected-warning {{calling function 'aa_elr_fun' requires exclusive lock 'aa_mu'}} You're not escaping the 80-column violation here, so I'd just put the expected-warning on the previous line.
Sign in to reply to this message.
Whoops, forgot to +Ollie.
Sign in to reply to this message.
http://codereview.appspot.com/4955041/diff/1/include/clang/Basic/DiagnosticSe... File include/clang/Basic/DiagnosticSemaKinds.td (right): http://codereview.appspot.com/4955041/diff/1/include/clang/Basic/DiagnosticSe... include/clang/Basic/DiagnosticSemaKinds.td:1402: "expecting lock '%0' to be held at start of each loop">, On 2011/08/26 19:20:47, Jeffrey Yasskin wrote: > What happens if someone changes whether a lock is held shared vs. exclusive > inside a loop? Good point. Right now, nothing. I am adding some logic. http://codereview.appspot.com/4955041/diff/1/include/clang/Basic/DiagnosticSe... include/clang/Basic/DiagnosticSemaKinds.td:1419: "calling function '%0' requires %select{shared|exclusive}2 lock '%1'">, On 2011/08/26 19:20:47, Jeffrey Yasskin wrote: > Is it possible to separate the lock-required annotation patch from the > shared-lock implementation? Yes. I will experiment with git add --patch. http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.cpp File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:774: enum AccType { On 2011/08/26 19:20:47, Jeffrey Yasskin wrote: > Spell out Access please. Done. http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:774: enum AccType { On 2011/08/26 19:20:47, Jeffrey Yasskin wrote: > Spell out Access please. Done. http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:789: // "upgrade" or "downgrade" between these two options. On 2011/08/26 19:20:47, Jeffrey Yasskin wrote: > FWIW, "upgrade" locks don't require reentrancy: > http://www.boost.org/doc/libs/1_47_0/doc/html/thread/synchronization.html#thr... Changed to: Note that this analysis does not currently support either re-entrant locking or lock "upgrading" and "downgrading" between exclusive and shared. http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:793: // the exclusive or shared version of the lock. On 2011/08/26 19:20:47, Jeffrey Yasskin wrote: > I don't quite understand this sentence. Can't you look up the mutex in the > lockset, and then just read the Locktype? Yes. Comment was stale and has been deleted. http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:840: bool LocksetContains(LockID Lock, LockType LockRequested = SHARED) const { On 2011/08/26 19:20:47, Jeffrey Yasskin wrote: > I would probably avoid the default argument here. It's nice to be explicit about > how we want the lock to be held. Refactored to make into two methods, one of which just checks if the lock is in the lockset held any way and one of which checks if the lock is in the lockset with the passed in locktype. I think this is more clear. http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:845: if (LockRequested == SHARED || LockHeld->Locktype == EXCLUSIVE) On 2011/08/26 19:20:47, Jeffrey Yasskin wrote: > Why not "LockHeld->Locktype >= LockRequested"? > > and then just return the expression rather than using "if (x) return true; else > return false;" Done. http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:875: // version of the a lock with this expression On 2011/08/26 19:20:47, Jeffrey Yasskin wrote: > s/the a/the/ Done. http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:879: // update LSet to use the newer LockData On 2011/08/26 19:20:47, Jeffrey Yasskin wrote: > Capitalize and punctuate comments! Done. http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:974: switch (AT) { On 2011/08/26 19:20:47, Jeffrey Yasskin wrote: > This looks suspiciously similar to the block you added to checkDereference(). > Can you share them? Done. http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:1042: /// FIXME: We need to also visit CallExprs (i.e. for global functions). On 2011/08/26 19:20:47, Jeffrey Yasskin wrote: > i.e. seems odd here. "visit CallExprs to catch/check global functions."? Done. http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:1083: addLock(ExpLocation, *I, SHARED); On 2011/08/26 19:20:47, Jeffrey Yasskin wrote: > Can you share this logic with the ExclusiveLockFunction case? Done. http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:1088: // When we encounter an exclusive trylock function, we need to add it On 2011/08/26 19:20:47, Jeffrey Yasskin wrote: > You can probably break this out into a separate patch. > > This is the kind of thing I wish you were using public git branches for. Then > you could present a patch series, and I could keep reviewing until I got tired, > but they'd be clearly split into pieces so I'd review related things together. > </not-actionable> I could present a patch series through Reitveld with or without public git branches; I think the public git branch issue is orthogonal. I will try and break this into a few separate patches. http://codereview.appspot.com/4955041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:1174: for (SharedLocksRequiredAttr::args_iterator I = SLRAttr->args_begin(), On 2011/08/26 19:20:47, Jeffrey Yasskin wrote: > Again, share the logic when possible. Done. It is possible to break out shared functionality even further, but it was a bit ugly right now. I added a FIXME to revisit this question after completing the implementation a bit more (which may impact refactoring decisions). http://codereview.appspot.com/4955041/diff/1/test/SemaCXX/warn-thread-safety-... File test/SemaCXX/warn-thread-safety-analysis.cpp (right): http://codereview.appspot.com/4955041/diff/1/test/SemaCXX/warn-thread-safety-... test/SemaCXX/warn-thread-safety-analysis.cpp:437: // expected-warning {{calling function 'aa_elr_fun' requires exclusive lock 'aa_mu'}} On 2011/08/26 19:20:47, Jeffrey Yasskin wrote: > You're not escaping the 80-column violation here, so I'd just put the > expected-warning on the previous line. I prefer it on the line below because then it fits easier on the screen and is consistent with the rest of this file and the parsing tests file.
Sign in to reply to this message.
FYI, has this been mailed upstream? Definitely go ahead and mail it if not. Mostly aesthetic comments... http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:781: SHARED, LLVM has very clear style rules for enums: LockKind, and LK_Shared. http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:798: // We need to track whether a lock is held shared or exclusively. Note that Doxyment please. http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:803: LockType Locktype; How about "Kind"? http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:805: LockData(SourceLocation Acq, LockType LT) You can use the same name in the constructor argument as the class member. It Just Works with initializer lists. http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:811: if (LT < Locktype) If we're going to do things based on the particular order in which the enum is written, we need some thorough comments on that enum to document the ordering semantics. I'd rather use an exhaustive switch here, so that if the enum ever changes, we'll get a compiler warning about not handling that here. http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:857: bool LocksetContains(LockID Lock) { lower case first letter... http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:893: // version of a lock with this expression. Is this because we don't support re-entrant locks yet? If so, we should have a FIXME here, or somewhere else clarifying that. http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:897: // Update LSet to use the newer LockData. This comment doesn't add any value for the reader. http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1049: if (SpecificAttr->args_size() == 0) {// The lock held is the "this" object. I would move this comment to the body of the if, rather than tacking it onto the line like this... http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1081: // to our lockset, marked as exclusive needs a full stop http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1082: case attr::ExclusiveLockFunction: { Why curlies http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1089: case attr::SharedLockFunction: { Why curlies?
Sign in to reply to this message.
This patch adds a warning if we acquire a lock with different kinds (exclusive vs. shared). http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:781: SHARED, On 2011/09/01 22:00:05, chandlerc wrote: > LLVM has very clear style rules for enums: > > LockKind, and LK_Shared. Done. http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:798: // We need to track whether a lock is held shared or exclusively. Note that On 2011/09/01 22:00:05, chandlerc wrote: > Doxyment please. Done. http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:803: LockType Locktype; On 2011/09/01 22:00:05, chandlerc wrote: > How about "Kind"? Replaced with LKind. http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:805: LockData(SourceLocation Acq, LockType LT) On 2011/09/01 22:00:05, chandlerc wrote: > You can use the same name in the constructor argument as the class member. It > Just Works with initializer lists. Done. http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:811: if (LT < Locktype) On 2011/09/01 22:00:05, chandlerc wrote: > If we're going to do things based on the particular order in which the enum is > written, we need some thorough comments on that enum to document the ordering > semantics. > > I'd rather use an exhaustive switch here, so that if the enum ever changes, > we'll get a compiler warning about not handling that here. Good point. This method is now deleted after discussions with Jeffrey. Instead, we flag an error and always set to exclusive (since that suppresses more future warnings). http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:857: bool LocksetContains(LockID Lock) { On 2011/09/01 22:00:05, chandlerc wrote: > lower case first letter... Done. http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:893: // version of a lock with this expression. On 2011/09/01 22:00:05, chandlerc wrote: > Is this because we don't support re-entrant locks yet? If so, we should have a > FIXME here, or somewhere else clarifying that. Done. http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:897: // Update LSet to use the newer LockData. On 2011/09/01 22:00:05, chandlerc wrote: > This comment doesn't add any value for the reader. Done. http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1049: if (SpecificAttr->args_size() == 0) {// The lock held is the "this" object. On 2011/09/01 22:00:05, chandlerc wrote: > I would move this comment to the body of the if, rather than tacking it onto the > line like this... Done. http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1081: // to our lockset, marked as exclusive On 2011/09/01 22:00:05, chandlerc wrote: > needs a full stop Done. http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1082: case attr::ExclusiveLockFunction: { On 2011/09/01 22:00:05, chandlerc wrote: > Why curlies Done. http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:1089: case attr::SharedLockFunction: { On 2011/09/01 22:00:05, chandlerc wrote: > Why curlies? Done.
Sign in to reply to this message.
FYI, sorry the comments got stashed on the wrong thread. This is the one they apply to. You need to audit 'LT' and 'AT' variables and switch them to 'LK' and 'AK'. http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:781: SHARED, On 2011/09/02 20:54:16, supertri wrote: > On 2011/09/01 22:00:05, chandlerc wrote: > > LLVM has very clear style rules for enums: > > > > LockKind, and LK_Shared. > > Done. Not quite: LK_Shared, not LK_SHARED.
Sign in to reply to this message.
http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4955041/diff/7001/lib/Sema/AnalysisBasedWarning... lib/Sema/AnalysisBasedWarnings.cpp:781: SHARED, On 2011/09/02 23:14:10, chandlerc wrote: > On 2011/09/02 20:54:16, supertri wrote: > > On 2011/09/01 22:00:05, chandlerc wrote: > > > LLVM has very clear style rules for enums: > > > > > > LockKind, and LK_Shared. > > > > Done. > > Not quite: LK_Shared, not LK_SHARED. Fixed and replaced AT with AK, LT with LK.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
