Code review - Issue 4894048: Threadsafety: Initial lockset implementationhttps://codereview.appspot.com/2011-08-23T18:26:04+00:00rietveld
Message from unknown
2011-08-15T23:50:20+00:00supertriurn:md5:4c08f730b89099ab3d53741cf4c487e1
Message from unknown
2011-08-19T22:21:22+00:00supertriurn:md5:7f123a65ce3880c968f63fe1c1637ad2
Message from chandlerc@gmail.com
2011-08-19T23:54:06+00:00chandlercurn:md5:a8c24f9c7db25d077df62a76e4ed77b4
Sorry these comments are so late in the process. I thought i had gone through this code much more carefully before, but I hadn't.
http://codereview.appspot.com/4894048/diff/1/include/clang/Analysis/CFG.h
File include/clang/Analysis/CFG.h (right):
http://codereview.appspot.com/4894048/diff/1/include/clang/Analysis/CFG.h#newcode800
include/clang/Analysis/CFG.h:800:
extaneous blank line
http://codereview.appspot.com/4894048/diff/1/include/clang/Analysis/CFG.h#newcode811
include/clang/Analysis/CFG.h:811:
ditto
http://codereview.appspot.com/4894048/diff/1/include/clang/Analysis/CFG.h#newcode822
include/clang/Analysis/CFG.h:822:
and ditto
http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-analysis.cpp
File test/SemaCXX/warn-thread-safety-analysis.cpp (right):
http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-analysis.cpp#newcode8
test/SemaCXX/warn-thread-safety-analysis.cpp:8: *************************************/
If you're going to use banners, use LLVM or Clang's standard banner format.
That said, I don't think banners are really important in test files... But I know you like them. At the least, I wouldn't bother for the helper section but ::shrug::
http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-analysis.cpp#newcode10
test/SemaCXX/warn-thread-safety-analysis.cpp:10: // more complicated classes, taken from gcc tests
I'd not bother with this comment. it also seems weirdly out of context here.
http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-analysis.cpp#newcode52
test/SemaCXX/warn-thread-safety-analysis.cpp:52: // sls_mw.x = 5; // turn mu into sls_mw.mu
FIXME?
http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-analysis.cpp#newcode114
test/SemaCXX/warn-thread-safety-analysis.cpp:114: else {
Consistent else style?
http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-analysis.cpp#newcode131
test/SemaCXX/warn-thread-safety-analysis.cpp:131: Destructable d; // same behaviour without this line
This comment doesn't have enough context for me to understand it, or the code...
http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-analysis.cpp#newcode144
test/SemaCXX/warn-thread-safety-analysis.cpp:144: expected-warning {{unlocking 'sls_mu' that was not acquired}}
When using the continuation of the comment line, please keep the '//' prefix on every line. That way the reader knows at a glance that these are still comments.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp
File lib/Sema/AnalysisBasedWarnings.cpp (right):
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode604
lib/Sema/AnalysisBasedWarnings.cpp:604: llvm::BitVector BitSet;
Maybe 'VisitedBlockIDs'
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode608
lib/Sema/AnalysisBasedWarnings.cpp:608: CFGBlockSet(const CFGBlockSet &S) : BitSet(S.BitSet) {}
Is this necessary?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode618
lib/Sema/AnalysisBasedWarnings.cpp:618: bool count(const CFGBlock *Block) {
Why call this one count?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode633
lib/Sema/AnalysisBasedWarnings.cpp:633: /// iterator to use CFGBlockSets.
It would be good to comment above on CFGBlockSet that this is an adaptor designed specifically to allow usage in po_iterator_storage. Otherwise, the somewhat strange API doesn't make a lot of sense.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode650
lib/Sema/AnalysisBasedWarnings.cpp:650: namespace { //anonymous namespace for clang::thread_safety
This comment seems weird.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode655
lib/Sema/AnalysisBasedWarnings.cpp:655: typedef llvm::po_iterator<const CFG*, CFGBlockSet, true> dfpo_iterator;
dfpo? Whynot just 'po_iterator?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode659
lib/Sema/AnalysisBasedWarnings.cpp:659: void Initialize(const CFG *Cfg) {
maybe just 'Graph' as the name?
Also, why isn't this just inlined into the constructor?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode663
lib/Sema/AnalysisBasedWarnings.cpp:663: for (dfpo_iterator I = dfpo_iterator::begin(Cfg, BSet),
Does this copy the BSet? You create a CFGBlockSet member in the po_storage above, I expected that to provide all the storage necessary. Why do we need a local variable?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode687
lib/Sema/AnalysisBasedWarnings.cpp:687: typedef llvm::SmallVector<DelayedDiag, 4> DiagList;
I would move all the code related to delayed diagnostic emission down to be right above the function that actually needs it.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode706
lib/Sema/AnalysisBasedWarnings.cpp:706: S.Diag((*I).first, (*I).second);
->
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode709
lib/Sema/AnalysisBasedWarnings.cpp:709:
extraneous newline.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode710
lib/Sema/AnalysisBasedWarnings.cpp:710: /// Thread-safety analysis works by comparing lock expressions. Within the
Need a \brief bit in the comment.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode732
lib/Sema/AnalysisBasedWarnings.cpp:732: /// Note we perform substitution on free variables such as "this" when
This comment doesn't really make sense for me... where did 'x' come from?
Your example below makes lots of sense, its the paragraph of text that doesn't necessarily seem to agree with it.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode739
lib/Sema/AnalysisBasedWarnings.cpp:739: class Lock {
Why is this called 'Lock'? That seems like an overly generic name for this class.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode740
lib/Sema/AnalysisBasedWarnings.cpp:740: Expr *LockExp; // currently used for debugging
Can you nuke this now?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode749
lib/Sema/AnalysisBasedWarnings.cpp:749: Lock(SourceLocation Acq, Expr *LExpr) : LockExp(LExpr), AcquireLoc(Acq) {
It seems odd that the parameter order is the reverse from ethe member order.
Also, maybe "Loc" instead of "Acq"? Or better, use the exact name of the members. That works in initializer lists.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode757
lib/Sema/AnalysisBasedWarnings.cpp:757: }
Is this necessary? I thought you removed it.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode771
lib/Sema/AnalysisBasedWarnings.cpp:771: // output by SourceLocation when iterating through this lockset.
This last sentence doesn't parse. Specifically, its not a "However" its a "Consequentially", but maybe it can just be reworded to be simpler.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode781
lib/Sema/AnalysisBasedWarnings.cpp:781: }
Another method I thought You removed..
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode790
lib/Sema/AnalysisBasedWarnings.cpp:790: /// is a way to get simple, and consistent, lock names.
Is there a reason not to store the text that makes up the expression itself? Might be worth at least a comment.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode795
lib/Sema/AnalysisBasedWarnings.cpp:795: // currently only used for debugging
Again, can we remove it then?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode820
lib/Sema/AnalysisBasedWarnings.cpp:820: llvm_unreachable("Expected lock expression!"); // FIXME: add diagnostic
unreachable likely isn't appropriate here. report_fatal_error or some such. (or a diagnostic)
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode831
lib/Sema/AnalysisBasedWarnings.cpp:831: class BuildLockset : public EvaluatedExprVisitor<BuildLockset> {
Do we still want this visitor here?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode868
lib/Sema/AnalysisBasedWarnings.cpp:868: S.Diag(Lock.getLoc(), diag::warn_double_unlock) << Lock.getName();
This isn't necessarily a double unlock. It's quite likely that they're unlocking something that has never been locked. We should diagnose it as an unlock without a matching lock.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode874
lib/Sema/AnalysisBasedWarnings.cpp:874: // FIXME: checking for guarded_by/var and pt_guarded_by/var
Maybe omit this visit method and move the fixme into the class?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode889
lib/Sema/AnalysisBasedWarnings.cpp:889: for(unsigned int i = 0; i < ArgAttrs.size(); ++i) {
just 'unsigned'
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode891
lib/Sema/AnalysisBasedWarnings.cpp:891: switch(Attr->getKind()){
space before {!!!! ;]
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode892
lib/Sema/AnalysisBasedWarnings.cpp:892:
No need for the blank line..
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode896
lib/Sema/AnalysisBasedWarnings.cpp:896: ExclusiveLockFunctionAttr *ELF =
Can't we get something about Attr in here?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode897
lib/Sema/AnalysisBasedWarnings.cpp:897: dyn_cast<ExclusiveLockFunctionAttr>(Attr);
This should be a cast, not a dyn_cast.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode900
lib/Sema/AnalysisBasedWarnings.cpp:900: AddLock(ExpLocation, Parent);
We should break after this, no? We don't need to go into the loop.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode904
lib/Sema/AnalysisBasedWarnings.cpp:904: }
no need for curlies.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode911
lib/Sema/AnalysisBasedWarnings.cpp:911: // from the lockset (they better be in there!)
Maybe mention that we will actually diagnose if they aren't in the lockset?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode914
lib/Sema/AnalysisBasedWarnings.cpp:914: dyn_cast<UnlockFunctionAttr>(ArgAttrs[i]);
cast<
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode916
lib/Sema/AnalysisBasedWarnings.cpp:916: if (UF->args_size() == 0) // lock held is the "this" object
Complete sentences in comments please.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode917
lib/Sema/AnalysisBasedWarnings.cpp:917: RemoveLock(ExpLocation, Parent);
Again, break.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode930
lib/Sema/AnalysisBasedWarnings.cpp:930: //FIXME: else if (isa <LockReturnedAttr> (ArgAttrs[i])) { // rewrite rules
Huh? This comment seems stale. Move it into the switch? You can add cases and then comment before the berak.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode937
lib/Sema/AnalysisBasedWarnings.cpp:937: /// issue warnings for any locks that are not contained in the intersection.
Can you be a bit more clear here? Clearly you're not warning on every lock in the program not contained in the intersection.
This doesn't appear to merge anything. It's just warning about the symmetric difference of the two sets, and computing the intersection.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode938
lib/Sema/AnalysisBasedWarnings.cpp:938: Lockset mergeAndWarn(Sema &S, Lockset LSet1, Lockset LSet2,
This should be a static function.
In general, use anonymous namespaces for types, and static keyword for functions.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode943
lib/Sema/AnalysisBasedWarnings.cpp:943: for (Lockset::iterator I = LSet2.begin(), E = LSet2.end();
Does this really not fit on a single line?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode952
lib/Sema/AnalysisBasedWarnings.cpp:952: for (Lockset::iterator I = LSet1.begin(), E = LSet1.end();
Does this really not fit on a single line?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode958
lib/Sema/AnalysisBasedWarnings.cpp:958: Intersection = Fact.remove(Intersection, *I);
I wonder if it would be more efficient to build the intersection once rather than removing these. Essentially, we could keep a small vector and add entries to it when they are in the intersection, and then return an immutable set from that.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode976
lib/Sema/AnalysisBasedWarnings.cpp:976: /// Compare this lockset with the loopEndSet, and output a warning if the two
For your comments, please always add a \brief line at the start, and use the \param tag to marke comments for specific parameters. I think you can do that in a followup change, but its important to do.
Which lockset is "this" lockset? I think this comment is just stale.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode981
lib/Sema/AnalysisBasedWarnings.cpp:981: const Lockset PreLoopSet,
This argument order and naming confuses me. Lets get the PreLoopSet first, followed by the LoopEndSet, and name them consistently. I'me fine with either Loop{Start,End}Set or with {Pre,Post}LoopSet.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode986
lib/Sema/AnalysisBasedWarnings.cpp:986: // Warn for locks held at the end of the loop, but not at the start.
This seems backwards. Why not first exampine PReLoopSet, and then LoopEndSet?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1002
lib/Sema/AnalysisBasedWarnings.cpp:1002: // first statement location, fall back on the current location.
Is this just because we don't track the location of the releases of locks? I wonder if it would be worth doing that to make diagnosticts better. At least worth a comment.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1004
lib/Sema/AnalysisBasedWarnings.cpp:1004: FirstLocInLoop = I->getLoc();
Why would this ever happen? Why can't we insist on always having a location? It seems really unfortunate to diagnose this on a random lock acquisition long before the loop that causes a problem.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1019
lib/Sema/AnalysisBasedWarnings.cpp:1019: CFG *Cfg = AC.getCFG();
'Graph' instead of 'Cfg' or some such?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1030
lib/Sema/AnalysisBasedWarnings.cpp:1030: std::vector<Lockset> EntryLocksets(Cfg->getNumBlockIDs(),
I'd like to leave some FIXMEs around to try and improve the performance impact of this. As it stands, this will be a very expensive analysis for large CFG functions.
At the very least, we'll almost certainly want to have SmallVectors for these to optimize small CFGs, but all of that can take the form of followup patches.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1035
lib/Sema/AnalysisBasedWarnings.cpp:1035: TopologicallySortedCFG tsCFG(Cfg);
SortedGraph maybe?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1040
lib/Sema/AnalysisBasedWarnings.cpp:1040:
Extraneous newline.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1041
lib/Sema/AnalysisBasedWarnings.cpp:1041: // get the next block and process it
This comment doesn't really add any value.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1042
lib/Sema/AnalysisBasedWarnings.cpp:1042: const CFGBlock *CurrBlock = (*I);
Extraneous parens.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1047
lib/Sema/AnalysisBasedWarnings.cpp:1047: // Create a default initial lockset in case there are no predecessors.
Why not only do this when !LocksetInitialized after your loop below?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1065
lib/Sema/AnalysisBasedWarnings.cpp:1065: PE = CurrBlock->pred_end(); PrevBlockIter != PE; ++PrevBlockIter) {
Either use PI and PE, or use full names for both.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1081
lib/Sema/AnalysisBasedWarnings.cpp:1081: // Process the current CFG block.
This is clear from the code.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1090
lib/Sema/AnalysisBasedWarnings.cpp:1090: Lockset Exitset = LocksetBuilder.getLockset();
Is copying these free? Because otherwise we're making this way more expensive than it needs to be.
Maybe you just want to store a reference here?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1091
lib/Sema/AnalysisBasedWarnings.cpp:1091: EntryLocksets[CurrBlockID] = Entryset;
Why can't we make Entryset a reference to this up above, and build it in-place?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1115
lib/Sema/AnalysisBasedWarnings.cpp:1115: if (!FinalLockset.isEmpty()) { // locks still held at end of CFG!
This comment doesn't add anything.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/SemaDeclAttr.cpp
File lib/Sema/SemaDeclAttr.cpp (right):
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/SemaDeclAttr.cpp#newcode278
lib/Sema/SemaDeclAttr.cpp:278: SmallVectorImpl<Expr*> &outArgs,
Just "Args" is fine.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/SemaDeclAttr.cpp#newcode381
lib/Sema/SemaDeclAttr.cpp:381: SmallVector<Expr*, 1> ArgVector;
FIXME to actually use the argcs?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/SemaDeclAttr.cpp#newcode616
lib/Sema/SemaDeclAttr.cpp:616: SmallVector<Expr*, 1> Args;
FIXME to actually use the args? Also it'd be nice to consistently use Args or ArgVector
Message from unknown
2011-08-22T18:28:19+00:00supertriurn:md5:543fe68538e2201b63498fb428433e56
Message from supertri@google.com
2011-08-22T18:30:16+00:00supertriurn:md5:fa1be25aff54adff9a932b807a8b0bc8
I made various small edits throughout the patch. Let me know if I can commit it today. DeLesley, do you see the comment for you in the topological sort section of AnalysisBasedWarnings?
Cheers,
Caitlin
http://codereview.appspot.com/4894048/diff/1/include/clang/Analysis/CFG.h
File include/clang/Analysis/CFG.h (right):
http://codereview.appspot.com/4894048/diff/1/include/clang/Analysis/CFG.h#newcode800
include/clang/Analysis/CFG.h:800:
This section is deleted in the later patch.
On 2011/08/19 23:54:06, chandlerc wrote:
> extaneous blank line
http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-analysis.cpp
File test/SemaCXX/warn-thread-safety-analysis.cpp (right):
http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-analysis.cpp#newcode8
test/SemaCXX/warn-thread-safety-analysis.cpp:8: *************************************/
Changed to llvm style.
On 2011/08/19 23:54:06, chandlerc wrote:
> If you're going to use banners, use LLVM or Clang's standard banner format.
>
> That said, I don't think banners are really important in test files... But I
> know you like them. At the least, I wouldn't bother for the helper section but
> ::shrug::
http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-analysis.cpp#newcode10
test/SemaCXX/warn-thread-safety-analysis.cpp:10: // more complicated classes, taken from gcc tests
Deleted comment.
On 2011/08/19 23:54:06, chandlerc wrote:
> I'd not bother with this comment. it also seems weirdly out of context here.
http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-analysis.cpp#newcode52
test/SemaCXX/warn-thread-safety-analysis.cpp:52: // sls_mw.x = 5; // turn mu into sls_mw.mu
On 2011/08/19 23:54:06, chandlerc wrote:
> FIXME?
Done.
http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-analysis.cpp#newcode114
test/SemaCXX/warn-thread-safety-analysis.cpp:114: else {
On 2011/08/19 23:54:06, chandlerc wrote:
> Consistent else style?
Done.
http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-analysis.cpp#newcode131
test/SemaCXX/warn-thread-safety-analysis.cpp:131: Destructable d; // same behaviour without this line
Got rid of "Destructable"
On 2011/08/19 23:54:06, chandlerc wrote:
> This comment doesn't have enough context for me to understand it, or the code...
Done.
http://codereview.appspot.com/4894048/diff/1/test/SemaCXX/warn-thread-safety-analysis.cpp#newcode144
test/SemaCXX/warn-thread-safety-analysis.cpp:144: expected-warning {{unlocking 'sls_mu' that was not acquired}}
I think in the context of "expected-warning" comments that actually makes it more confusing since only having one '//' prefix makes the line for which the warning applies clear. Also, this style is consistent with the previously committed test file.
On 2011/08/19 23:54:06, chandlerc wrote:
> When using the continuation of the comment line, please keep the '//' prefix on
> every line. That way the reader knows at a glance that these are still comments.
http://codereview.appspot.com/4894048/diff/2001/include/clang/Analysis/CFG.h
File include/clang/Analysis/CFG.h (right):
http://codereview.appspot.com/4894048/diff/2001/include/clang/Analysis/CFG.h#newcode799
include/clang/Analysis/CFG.h:799:
Deleted this change.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp
File lib/Sema/AnalysisBasedWarnings.cpp (right):
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode604
lib/Sema/AnalysisBasedWarnings.cpp:604: llvm::BitVector BitSet;
On 2011/08/19 23:54:06, chandlerc wrote:
> Maybe 'VisitedBlockIDs'
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode608
lib/Sema/AnalysisBasedWarnings.cpp:608: CFGBlockSet(const CFGBlockSet &S) : BitSet(S.BitSet) {}
Deleted.
On 2011/08/19 23:54:06, chandlerc wrote:
> Is this necessary?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode618
lib/Sema/AnalysisBasedWarnings.cpp:618: bool count(const CFGBlock *Block) {
Removed.
On 2011/08/19 23:54:06, chandlerc wrote:
> Why call this one count?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode633
lib/Sema/AnalysisBasedWarnings.cpp:633: /// iterator to use CFGBlockSets.
On 2011/08/19 23:54:06, chandlerc wrote:
> It would be good to comment above on CFGBlockSet that this is an adaptor
> designed specifically to allow usage in po_iterator_storage. Otherwise, the
> somewhat strange API doesn't make a lot of sense.
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode650
lib/Sema/AnalysisBasedWarnings.cpp:650: namespace { //anonymous namespace for clang::thread_safety
changed
On 2011/08/19 23:54:06, chandlerc wrote:
> This comment seems weird.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode655
lib/Sema/AnalysisBasedWarnings.cpp:655: typedef llvm::po_iterator<const CFG*, CFGBlockSet, true> dfpo_iterator;
Changed.
On 2011/08/19 23:54:06, chandlerc wrote:
> dfpo? Whynot just 'po_iterator?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode659
lib/Sema/AnalysisBasedWarnings.cpp:659: void Initialize(const CFG *Cfg) {
Changed to CFGraph and inlined.
On 2011/08/19 23:54:06, chandlerc wrote:
> maybe just 'Graph' as the name?
>
> Also, why isn't this just inlined into the constructor?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode663
lib/Sema/AnalysisBasedWarnings.cpp:663: for (dfpo_iterator I = dfpo_iterator::begin(Cfg, BSet),
I think we need to pass a CFGBlockSet into the constructor for po_storage. DeLeseley, any additional comments on this?
On 2011/08/19 23:54:06, chandlerc wrote:
> Does this copy the BSet? You create a CFGBlockSet member in the po_storage
> above, I expected that to provide all the storage necessary. Why do we need a
> local variable?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode687
lib/Sema/AnalysisBasedWarnings.cpp:687: typedef llvm::SmallVector<DelayedDiag, 4> DiagList;
On 2011/08/19 23:54:06, chandlerc wrote:
> I would move all the code related to delayed diagnostic emission down to be
> right above the function that actually needs it.
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode706
lib/Sema/AnalysisBasedWarnings.cpp:706: S.Diag((*I).first, (*I).second);
On 2011/08/19 23:54:06, chandlerc wrote:
> ->
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode709
lib/Sema/AnalysisBasedWarnings.cpp:709:
On 2011/08/19 23:54:06, chandlerc wrote:
> extraneous newline.
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode710
lib/Sema/AnalysisBasedWarnings.cpp:710: /// Thread-safety analysis works by comparing lock expressions. Within the
On 2011/08/19 23:54:06, chandlerc wrote:
> Need a \brief bit in the comment.
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode732
lib/Sema/AnalysisBasedWarnings.cpp:732: /// Note we perform substitution on free variables such as "this" when
Moved below example and clarified.
On 2011/08/19 23:54:06, chandlerc wrote:
> This comment doesn't really make sense for me... where did 'x' come from?
>
> Your example below makes lots of sense, its the paragraph of text that doesn't
> necessarily seem to agree with it.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode739
lib/Sema/AnalysisBasedWarnings.cpp:739: class Lock {
I think it is short, to the point, and makes sense in this context. We track a set (a lockset) of what locks we have currently acquired. It makes sense to me to call it a lockset and to call the things that go into it locks. Do you have another suggestion?
On 2011/08/19 23:54:06, chandlerc wrote:
> Why is this called 'Lock'? That seems like an overly generic name for this
> class.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode740
lib/Sema/AnalysisBasedWarnings.cpp:740: Expr *LockExp; // currently used for debugging
On 2011/08/19 23:54:06, chandlerc wrote:
> Can you nuke this now?
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode749
lib/Sema/AnalysisBasedWarnings.cpp:749: Lock(SourceLocation Acq, Expr *LExpr) : LockExp(LExpr), AcquireLoc(Acq) {
Changed to Loc
On 2011/08/19 23:54:06, chandlerc wrote:
> It seems odd that the parameter order is the reverse from ethe member order.
>
> Also, maybe "Loc" instead of "Acq"? Or better, use the exact name of the
> members. That works in initializer lists.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode757
lib/Sema/AnalysisBasedWarnings.cpp:757: }
I did, in the patch that refactors to use an ImmutableMap. I merged the two patches together so we can commit all the edits at once.
On 2011/08/19 23:54:06, chandlerc wrote:
> Is this necessary? I thought you removed it.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode771
lib/Sema/AnalysisBasedWarnings.cpp:771: // output by SourceLocation when iterating through this lockset.
Removed the "however"
On 2011/08/19 23:54:06, chandlerc wrote:
> This last sentence doesn't parse. Specifically, its not a "However" its a
> "Consequentially", but maybe it can just be reworded to be simpler.
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode781
lib/Sema/AnalysisBasedWarnings.cpp:781: }
I did, in the patch that refactors to use an ImmutableMap. I merged the two patches together so we can commit all the edits at once.
On 2011/08/19 23:54:06, chandlerc wrote:
> Another method I thought You removed..
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode790
lib/Sema/AnalysisBasedWarnings.cpp:790: /// is a way to get simple, and consistent, lock names.
Added a comment that it is for security reasons. Is there anything else I should say?
On 2011/08/19 23:54:06, chandlerc wrote:
> Is there a reason not to store the text that makes up the expression itself?
> Might be worth at least a comment.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode795
lib/Sema/AnalysisBasedWarnings.cpp:795: // currently only used for debugging
removed.
On 2011/08/19 23:54:06, chandlerc wrote:
> Again, can we remove it then?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode820
lib/Sema/AnalysisBasedWarnings.cpp:820: llvm_unreachable("Expected lock expression!"); // FIXME: add diagnostic
On 2011/08/19 23:54:06, chandlerc wrote:
> unreachable likely isn't appropriate here. report_fatal_error or some such. (or
> a diagnostic)
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode831
lib/Sema/AnalysisBasedWarnings.cpp:831: class BuildLockset : public EvaluatedExprVisitor<BuildLockset> {
I think we do for now. It definitely makes it easier to structure. I will add a fixme to revisit this question later.
On 2011/08/19 23:54:06, chandlerc wrote:
> Do we still want this visitor here?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode868
lib/Sema/AnalysisBasedWarnings.cpp:868: S.Diag(Lock.getLoc(), diag::warn_double_unlock) << Lock.getName();
Changed text of warning title.
On 2011/08/19 23:54:06, chandlerc wrote:
> This isn't necessarily a double unlock. It's quite likely that they're unlocking
> something that has never been locked. We should diagnose it as an unlock without
> a matching lock.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode874
lib/Sema/AnalysisBasedWarnings.cpp:874: // FIXME: checking for guarded_by/var and pt_guarded_by/var
Patch that fixes this is already under review (please review it). Would rather keep like this and just submit the next patch as soon as possible.
On 2011/08/19 23:54:06, chandlerc wrote:
> Maybe omit this visit method and move the fixme into the class?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode889
lib/Sema/AnalysisBasedWarnings.cpp:889: for(unsigned int i = 0; i < ArgAttrs.size(); ++i) {
On 2011/08/19 23:54:06, chandlerc wrote:
> just 'unsigned'
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode891
lib/Sema/AnalysisBasedWarnings.cpp:891: switch(Attr->getKind()){
On 2011/08/19 23:54:06, chandlerc wrote:
> space before {!!!! ;]
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode892
lib/Sema/AnalysisBasedWarnings.cpp:892:
I would like to keep one blank line before each case.
On 2011/08/19 23:54:06, chandlerc wrote:
> No need for the blank line..
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode896
lib/Sema/AnalysisBasedWarnings.cpp:896: ExclusiveLockFunctionAttr *ELF =
?
On 2011/08/19 23:54:06, chandlerc wrote:
> Can't we get something about Attr in here?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode897
lib/Sema/AnalysisBasedWarnings.cpp:897: dyn_cast<ExclusiveLockFunctionAttr>(Attr);
On 2011/08/19 23:54:06, chandlerc wrote:
> This should be a cast, not a dyn_cast.
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode900
lib/Sema/AnalysisBasedWarnings.cpp:900: AddLock(ExpLocation, Parent);
In that case args_begin() == args_end(), so we will not execute any iterations of the loop.
On 2011/08/19 23:54:06, chandlerc wrote:
> We should break after this, no? We don't need to go into the loop.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode904
lib/Sema/AnalysisBasedWarnings.cpp:904: }
On 2011/08/19 23:54:06, chandlerc wrote:
> no need for curlies.
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode911
lib/Sema/AnalysisBasedWarnings.cpp:911: // from the lockset (they better be in there!)
On 2011/08/19 23:54:06, chandlerc wrote:
> Maybe mention that we will actually diagnose if they aren't in the lockset?
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode914
lib/Sema/AnalysisBasedWarnings.cpp:914: dyn_cast<UnlockFunctionAttr>(ArgAttrs[i]);
On 2011/08/19 23:54:06, chandlerc wrote:
> cast<
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode916
lib/Sema/AnalysisBasedWarnings.cpp:916: if (UF->args_size() == 0) // lock held is the "this" object
On 2011/08/19 23:54:06, chandlerc wrote:
> Complete sentences in comments please.
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode917
lib/Sema/AnalysisBasedWarnings.cpp:917: RemoveLock(ExpLocation, Parent);
On 2011/08/19 23:54:06, chandlerc wrote:
> Again, break.
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode930
lib/Sema/AnalysisBasedWarnings.cpp:930: //FIXME: else if (isa <LockReturnedAttr> (ArgAttrs[i])) { // rewrite rules
Moved. The next patch adds cases.
On 2011/08/19 23:54:06, chandlerc wrote:
> Huh? This comment seems stale. Move it into the switch? You can add cases and
> then comment before the berak.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode937
lib/Sema/AnalysisBasedWarnings.cpp:937: /// issue warnings for any locks that are not contained in the intersection.
Changed to intersectAndWarn. Added more comments.
On 2011/08/19 23:54:06, chandlerc wrote:
> Can you be a bit more clear here? Clearly you're not warning on every lock in
> the program not contained in the intersection.
>
> This doesn't appear to merge anything. It's just warning about the symmetric
> difference of the two sets, and computing the intersection.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode938
lib/Sema/AnalysisBasedWarnings.cpp:938: Lockset mergeAndWarn(Sema &S, Lockset LSet1, Lockset LSet2,
On 2011/08/19 23:54:06, chandlerc wrote:
> This should be a static function.
>
> In general, use anonymous namespaces for types, and static keyword for
> functions.
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode943
lib/Sema/AnalysisBasedWarnings.cpp:943: for (Lockset::iterator I = LSet2.begin(), E = LSet2.end();
On 2011/08/19 23:54:06, chandlerc wrote:
> Does this really not fit on a single line?
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode952
lib/Sema/AnalysisBasedWarnings.cpp:952: for (Lockset::iterator I = LSet1.begin(), E = LSet1.end();
On 2011/08/19 23:54:06, chandlerc wrote:
> Does this really not fit on a single line?
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode958
lib/Sema/AnalysisBasedWarnings.cpp:958: Intersection = Fact.remove(Intersection, *I);
I believe we looked into it and removing should be pretty efficient.
Also, the common case is that there is nothing to remove (no error).
Also, I think we would still have to add one element at a time.
On 2011/08/19 23:54:06, chandlerc wrote:
> I wonder if it would be more efficient to build the intersection once rather
> than removing these. Essentially, we could keep a small vector and add entries
> to it when they are in the intersection, and then return an immutable set from
> that.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode976
lib/Sema/AnalysisBasedWarnings.cpp:976: /// Compare this lockset with the loopEndSet, and output a warning if the two
Fixed comment. Added brief here and other places.
On 2011/08/19 23:54:06, chandlerc wrote:
> For your comments, please always add a \brief line at the start, and use the
> \param tag to marke comments for specific parameters. I think you can do that in
> a followup change, but its important to do.
>
> Which lockset is "this" lockset? I think this comment is just stale.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode981
lib/Sema/AnalysisBasedWarnings.cpp:981: const Lockset PreLoopSet,
Changed LoopEndSet to LastLoopSet. PreLoopSet is the lockset directly *before* the loop, whereas LastLoopSet is the last CFG block of the loop. I have added some clarification in the comments.
On 2011/08/19 23:54:06, chandlerc wrote:
> This argument order and naming confuses me. Lets get the PreLoopSet first,
> followed by the LoopEndSet, and name them consistently. I'me fine with either
> Loop{Start,End}Set or with {Pre,Post}LoopSet.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode986
lib/Sema/AnalysisBasedWarnings.cpp:986: // Warn for locks held at the end of the loop, but not at the start.
Swapped these two blocks.
On 2011/08/19 23:54:06, chandlerc wrote:
> This seems backwards. Why not first exampine PReLoopSet, and then LoopEndSet?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1002
lib/Sema/AnalysisBasedWarnings.cpp:1002: // first statement location, fall back on the current location.
This warning is reported at the start of a loop. I have added comments to that effect.
On 2011/08/19 23:54:06, chandlerc wrote:
> Is this just because we don't track the location of the releases of locks? I
> wonder if it would be worth doing that to make diagnosticts better. At least
> worth a comment.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1004
lib/Sema/AnalysisBasedWarnings.cpp:1004: FirstLocInLoop = I->getLoc();
I think we will always have a location, and have added an assertion to that effect.
On 2011/08/19 23:54:06, chandlerc wrote:
> Why would this ever happen? Why can't we insist on always having a location? It
> seems really unfortunate to diagnose this on a random lock acquisition long
> before the loop that causes a problem.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1019
lib/Sema/AnalysisBasedWarnings.cpp:1019: CFG *Cfg = AC.getCFG();
Changed to CFGraph.
On 2011/08/19 23:54:06, chandlerc wrote:
> 'Graph' instead of 'Cfg' or some such?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1030
lib/Sema/AnalysisBasedWarnings.cpp:1030: std::vector<Lockset> EntryLocksets(Cfg->getNumBlockIDs(),
FIXME added. We could even use arrays since the size of the vectors is fixed at the number of CFG Blocks. Note that the calls to LocksetFactory.getEmptySet() all return the same thing.
On 2011/08/19 23:54:06, chandlerc wrote:
> I'd like to leave some FIXMEs around to try and improve the performance impact
> of this. As it stands, this will be a very expensive analysis for large CFG
> functions.
>
> At the very least, we'll almost certainly want to have SmallVectors for these to
> optimize small CFGs, but all of that can take the form of followup patches.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1035
lib/Sema/AnalysisBasedWarnings.cpp:1035: TopologicallySortedCFG tsCFG(Cfg);
On 2011/08/19 23:54:06, chandlerc wrote:
> SortedGraph maybe?
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1040
lib/Sema/AnalysisBasedWarnings.cpp:1040:
On 2011/08/19 23:54:06, chandlerc wrote:
> Extraneous newline.
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1041
lib/Sema/AnalysisBasedWarnings.cpp:1041: // get the next block and process it
On 2011/08/19 23:54:06, chandlerc wrote:
> This comment doesn't really add any value.
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1042
lib/Sema/AnalysisBasedWarnings.cpp:1042: const CFGBlock *CurrBlock = (*I);
On 2011/08/19 23:54:06, chandlerc wrote:
> Extraneous parens.
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1047
lib/Sema/AnalysisBasedWarnings.cpp:1047: // Create a default initial lockset in case there are no predecessors.
There is no public default constructor for locksets. Instead, you have to set it to another lockset. getEmptySet() is very cheap since there is only one empty lockset.
On 2011/08/19 23:54:06, chandlerc wrote:
> Why not only do this when !LocksetInitialized after your loop below?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1065
lib/Sema/AnalysisBasedWarnings.cpp:1065: PE = CurrBlock->pred_end(); PrevBlockIter != PE; ++PrevBlockIter) {
On 2011/08/19 23:54:06, chandlerc wrote:
> Either use PI and PE, or use full names for both.
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1081
lib/Sema/AnalysisBasedWarnings.cpp:1081: // Process the current CFG block.
I would still like to keep this comment.
On 2011/08/19 23:54:06, chandlerc wrote:
> This is clear from the code.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1090
lib/Sema/AnalysisBasedWarnings.cpp:1090: Lockset Exitset = LocksetBuilder.getLockset();
Since we are using ImmutableMaps, I believe copying these is pretty much free.
On 2011/08/19 23:54:06, chandlerc wrote:
> Is copying these free? Because otherwise we're making this way more expensive
> than it needs to be.
>
> Maybe you just want to store a reference here?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1091
lib/Sema/AnalysisBasedWarnings.cpp:1091: EntryLocksets[CurrBlockID] = Entryset;
See above comment.
On 2011/08/19 23:54:06, chandlerc wrote:
> Why can't we make Entryset a reference to this up above, and build it in-place?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1115
lib/Sema/AnalysisBasedWarnings.cpp:1115: if (!FinalLockset.isEmpty()) { // locks still held at end of CFG!
Deleted.
On 2011/08/19 23:54:06, chandlerc wrote:
> This comment doesn't add anything.
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/SemaDeclAttr.cpp
File lib/Sema/SemaDeclAttr.cpp (right):
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/SemaDeclAttr.cpp#newcode278
lib/Sema/SemaDeclAttr.cpp:278: SmallVectorImpl<Expr*> &outArgs,
On 2011/08/19 23:54:06, chandlerc wrote:
> Just "Args" is fine.
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/SemaDeclAttr.cpp#newcode381
lib/Sema/SemaDeclAttr.cpp:381: SmallVector<Expr*, 1> ArgVector;
Refactored this to make it more clear that I am just checking one argument. (ArgVector was a vector for that Arg, in contrast with Args). In this case, and so the method for passing it along is different.
On 2011/08/19 23:54:06, chandlerc wrote:
> FIXME to actually use the argcs?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/SemaDeclAttr.cpp#newcode616
lib/Sema/SemaDeclAttr.cpp:616: SmallVector<Expr*, 1> Args;
On 2011/08/19 23:54:06, chandlerc wrote:
See previous response.
> FIXME to actually use the args? Also it'd be nice to consistently use Args or
> ArgVector
Message from delesley@google.com
2011-08-22T18:58:03+00:00delesleyurn:md5:8db1331a57dc2d267f8ef00d0d5b49fa
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp
File lib/Sema/AnalysisBasedWarnings.cpp (right):
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode604
lib/Sema/AnalysisBasedWarnings.cpp:604: llvm::BitVector BitSet;
It's supposed to implement an actual set interface for po_iterator. Thus the name CFGBlockSet. Calling it VisitedBlockIDs obscures the interface that it's supposed to provide.
On 2011/08/22 18:30:16, supertri wrote:
> On 2011/08/19 23:54:06, chandlerc wrote:
> > Maybe 'VisitedBlockIDs'
>
> Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode618
lib/Sema/AnalysisBasedWarnings.cpp:618: bool count(const CFGBlock *Block) {
On 2011/08/19 23:54:06, chandlerc wrote:
> Why call this one count?
Again, it has to implement the set interface, which specifies a method called "count". I included the method for completeness; the actual template instantiations may not require it in this case.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode640
lib/Sema/AnalysisBasedWarnings.cpp:640: CFGBlockSet Visited;
Should be CFGBlockSet &Visited.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode655
lib/Sema/AnalysisBasedWarnings.cpp:655: typedef llvm::po_iterator<const CFG*, CFGBlockSet, true> dfpo_iterator;
On 2011/08/19 23:54:06, chandlerc wrote:
> dfpo? Whynot just 'po_iterator?
Because I was uncomfortable with a naming conflict between the local po_iterator and llvm::po_iterator.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode659
lib/Sema/AnalysisBasedWarnings.cpp:659: void Initialize(const CFG *Cfg) {
It's not inlined, because I'm copying the RPO example code.
On 2011/08/19 23:54:06, chandlerc wrote:
> maybe just 'Graph' as the name?
>
> Also, why isn't this just inlined into the constructor?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode663
lib/Sema/AnalysisBasedWarnings.cpp:663: for (dfpo_iterator I = dfpo_iterator::begin(Cfg, BSet),
See comment on line 640, above: the set in po_iterator storage should be a reference.
On 2011/08/22 18:30:16, supertri wrote:
> I think we need to pass a CFGBlockSet into the constructor for po_storage.
> DeLeseley, any additional comments on this?
>
> On 2011/08/19 23:54:06, chandlerc wrote:
> > Does this copy the BSet? You create a CFGBlockSet member in the po_storage
> > above, I expected that to provide all the storage necessary. Why do we need a
> > local variable?
>
Message from chandlerc@gmail.com
2011-08-22T21:52:12+00:00chandlercurn:md5:338c9b3ccc7bb9f76320fa94fb5c43e8
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp
File lib/Sema/AnalysisBasedWarnings.cpp (right):
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode650
lib/Sema/AnalysisBasedWarnings.cpp:650: namespace { //anonymous namespace for clang::thread_safety
On 2011/08/22 18:30:16, supertri wrote:
> changed
>
> On 2011/08/19 23:54:06, chandlerc wrote:
> > This comment seems weird.
>
I'm suggesting removing it altogether. It's clearly an anonymous namespace, and there is a big banner at the top to tell us why.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode739
lib/Sema/AnalysisBasedWarnings.cpp:739: class Lock {
On 2011/08/22 18:30:16, supertri wrote:
> I think it is short, to the point, and makes sense in this context. We track a
> set (a lockset) of what locks we have currently acquired. It makes sense to me
> to call it a lockset and to call the things that go into it locks. Do you have
> another suggestion?
LockSignature? I dunno. Maybe others have better names here. My problem is that this *isn't* a lock. It models a lock in the users source code by a specific set of assumed-to-be-unique aspects. I might actually call this "LockID" (note, capitalized 'D') and call the inner vector "DeclSeq" or some such. The inner construct is an implementation detail. The class itself models a way to identify a lock (not an actual lock).
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode831
lib/Sema/AnalysisBasedWarnings.cpp:831: class BuildLockset : public EvaluatedExprVisitor<BuildLockset> {
On 2011/08/22 18:30:16, supertri wrote:
> I think we do for now. It definitely makes it easier to structure. I will add a
> fixme to revisit this question later.
I was really asking whether we want to use StmtVisitor rather than EvaluatedExprVisitior.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode892
lib/Sema/AnalysisBasedWarnings.cpp:892:
On 2011/08/22 18:30:16, supertri wrote:
> I would like to keep one blank line before each case.
That is not the pervailing LLVM or Clang style. Consistency is the goal here.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode896
lib/Sema/AnalysisBasedWarnings.cpp:896: ExclusiveLockFunctionAttr *ELF =
On 2011/08/22 18:30:16, supertri wrote:
> ?
The variable name 'ELF' is both confusing (there are many things to do w/ ELF, most of them are no ExclusiveLockFunctions), and misses key information: this is an *attribute*, not the exclusive lock function itself. maybe "ExclusiveAttr"?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode900
lib/Sema/AnalysisBasedWarnings.cpp:900: AddLock(ExpLocation, Parent);
On 2011/08/22 18:30:16, supertri wrote:
> In that case args_begin() == args_end(), so we will not execute any iterations
> of the loop.
But we shouldn't compute that the size is zero twice. What if this is a linked list, and size has linear complexity?
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode930
lib/Sema/AnalysisBasedWarnings.cpp:930: //FIXME: else if (isa <LockReturnedAttr> (ArgAttrs[i])) { // rewrite rules
On 2011/08/22 18:30:16, supertri wrote:
> Moved. The next patch adds cases.
See my comment above. The request to move was specifically tied to pro-actively adding a case. I want the comment attached to some actual C++ code that will be encountered again when fixing. Currently they aren't. I'm happy attaching them to the switch itself, or to an empty (stub) case. I prefer the latter.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1081
lib/Sema/AnalysisBasedWarnings.cpp:1081: // Process the current CFG block.
On 2011/08/22 18:30:16, supertri wrote:
> I would still like to keep this comment.
But why? What value does it add? I try to only add comments when there is some piece of information the reader is likely to be missing, in the hope that the comments are actually read... but fine, i'll desist..
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1091
lib/Sema/AnalysisBasedWarnings.cpp:1091: EntryLocksets[CurrBlockID] = Entryset;
On 2011/08/22 18:30:16, supertri wrote:
> See above comment.
I see why we don't *need* to do this, it would seem strictly cleaner to me to make Entryset and Exitset be references to the elements of the vector / array we already have created, and mutate them in place.
Even though copying relatively cheap, there is no reason to even bother in this case.
http://codereview.appspot.com/4894048/diff/10001/include/clang/Basic/DiagnosticSemaKinds.td
File include/clang/Basic/DiagnosticSemaKinds.td (right):
http://codereview.appspot.com/4894048/diff/10001/include/clang/Basic/DiagnosticSemaKinds.td#newcode1321
include/clang/Basic/DiagnosticSemaKinds.td:1321: def warn_unlock_but_no_acquire : Warning<
At some point I think we should consistently use "lock" and "unlock" as verbs, or use "acquire" and "release" as verbs, relegating "lock" to a noun. Follow-up patch is good for this, and maybe the one switching to C++0x's naming conventions would be a good time to fix thise.
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp
File lib/Sema/AnalysisBasedWarnings.cpp (right):
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode712
lib/Sema/AnalysisBasedWarnings.cpp:712: /// "x" is substituted for "this" so we get x->mu();
Is 'this' the only variable so substituted? Essentially, I'm not sure what you mean by 'free-variables'.
Are there other things (besides 'this') that are substituted? How does this work?
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode719
lib/Sema/AnalysisBasedWarnings.cpp:719: /// consistent with C++0x
Feel free to switch immediately (in a separate patch though!) to C++0x terminology.
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode785
lib/Sema/AnalysisBasedWarnings.cpp:785: SourceLocation AcquireLoc;
Why not just map to the source location? I don't see any real indicators of future information you're expecting to add.
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode812
lib/Sema/AnalysisBasedWarnings.cpp:812: /// \brief This is a visitor class which visits each expression in a CFGBlock.
This doesn't seem to match the API. It doesn't have anything to do with a CFGBlock. Maybe just "each expression in a Stmt"?
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode897
lib/Sema/AnalysisBasedWarnings.cpp:897: // FIXME: shared/trylock variations
This FIXME is now in a weird location. Why here? Is there some other comment we could attach it to that would make it more likely to be, well, fixed.
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode924
lib/Sema/AnalysisBasedWarnings.cpp:924: // the lockset in deterministic order.
This should be a doxyment. Also, it's not immediately obvious why this typedef is relevant to the requirement in this comment. Maybe attach the comment (and expand on it) to the EmitDiagnostics function.
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode949
lib/Sema/AnalysisBasedWarnings.cpp:949: /// locks that are not contained in the intersection.
You still need to make this more precise: "for any locks *in the union* that are not contained in the intersection."
Or you can just say that you're warning on the symmetric difference, as that's how it is defined.
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode998
lib/Sema/AnalysisBasedWarnings.cpp:998: /// a warning if the two locksets are unequal.
You've repeated all of the body of this comment here. ;] Brief sections can really be brief:
"Warn about different locksets in backedges of loops." or something...
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1001
lib/Sema/AnalysisBasedWarnings.cpp:1001: /// \param LoopEndSet Locks held in the last CFG block of the loop
This is using the old name.
I still find these names confusing... but oh well.
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1009
lib/Sema/AnalysisBasedWarnings.cpp:1009: assert(FirstLocInLoop.isValid()); // FIXME: fail gracefully?
Why FIXME? It seems like we want to always have a location, and asserting until we find all the edge cases that don't is reasonable.
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1043
lib/Sema/AnalysisBasedWarnings.cpp:1043: /// The main thread safety loop. Each block in the CFG is traversed exactly
Your brief comment is longer than the rest of the comments. =/ And "The main thread safety loop" doesn't really help the reader much. Maybe as the brief: "Check a function's CFG for thread-safety violations." Then the details can follow.
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1110
lib/Sema/AnalysisBasedWarnings.cpp:1110: for (CFGBlock::const_iterator I = CurrBlock->begin(), E = CurrBlock->end();
It would seem nice to not shadow I and E here... BI, BE?
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/SemaDeclAttr.cpp
File lib/Sema/SemaDeclAttr.cpp (right):
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/SemaDeclAttr.cpp#newcode274
lib/Sema/SemaDeclAttr.cpp:274: bool checkExpLockableObj(Sema & S, Decl *D, const AttributeList & Attr,
Please fix the coding conventions in this function, as commented repeatedly in prior reviews:
doxyments.
indentation.
space after &
variable names
Message from delesley@google.com
2011-08-22T22:02:52+00:00delesleyurn:md5:d6c92e039b4b080dae4eb80a5d0e495d
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp
File lib/Sema/AnalysisBasedWarnings.cpp (right):
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode712
lib/Sema/AnalysisBasedWarnings.cpp:712: /// "x" is substituted for "this" so we get x->mu();
"Free variable" is a standard term used the type system literature. It includes "this", as well as any arguments to functions, e.g. l is a free variable in l->mu:
foo(MyList *l) EXCLUSIVE_LOCKS_REQUIRED(l->mu) { ... }
MyList *myl;
foo(myl); // requires lock myl->mu to be held
Notice that l is replaced with myl, in the exact same way that "this" is replaced with the target object.
On 2011/08/22 21:52:12, chandlerc wrote:
> Is 'this' the only variable so substituted? Essentially, I'm not sure what you
> mean by 'free-variables'.
>
> Are there other things (besides 'this') that are substituted? How does this
> work?
Message from supertri@google.com
2011-08-23T17:47:37+00:00supertriurn:md5:5b17945757cc42738837a3878bbd97ff
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp
File lib/Sema/AnalysisBasedWarnings.cpp (right):
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode604
lib/Sema/AnalysisBasedWarnings.cpp:604: llvm::BitVector BitSet;
On 2011/08/22 18:58:03, delesley wrote:
> It's supposed to implement an actual set interface for po_iterator. Thus the
> name CFGBlockSet. Calling it VisitedBlockIDs obscures the interface that it's
> supposed to provide.
The change is from BitSet to VisitedBlockIDs (not CFGBlockSet to VisitedBlockIDs).
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode604
lib/Sema/AnalysisBasedWarnings.cpp:604: llvm::BitVector BitSet;
On 2011/08/22 18:58:03, delesley wrote:
> It's supposed to implement an actual set interface for po_iterator. Thus the
> name CFGBlockSet. Calling it VisitedBlockIDs obscures the interface that it's
> supposed to provide.
The change is from BitSet to VisitedBlockIDs (not CFGBlockSet to VisitedBlockIDs).
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode618
lib/Sema/AnalysisBasedWarnings.cpp:618: bool count(const CFGBlock *Block) {
On 2011/08/22 18:58:03, delesley wrote:
> On 2011/08/19 23:54:06, chandlerc wrote:
> > Why call this one count?
>
> Again, it has to implement the set interface, which specifies a method called
> "count". I included the method for completeness; the actual template
> instantiations may not require it in this case.
Since the "count" method was unused, it is now removed.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode640
lib/Sema/AnalysisBasedWarnings.cpp:640: CFGBlockSet Visited;
On 2011/08/22 18:58:03, delesley wrote:
> Should be CFGBlockSet &Visited.
Actually, we were able to delete both of these entire templates.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode650
lib/Sema/AnalysisBasedWarnings.cpp:650: namespace { //anonymous namespace for clang::thread_safety
On 2011/08/22 21:52:12, chandlerc wrote:
> On 2011/08/22 18:30:16, supertri wrote:
> > changed
> >
> > On 2011/08/19 23:54:06, chandlerc wrote:
> > > This comment seems weird.
> >
>
> I'm suggesting removing it altogether. It's clearly an anonymous namespace, and
> there is a big banner at the top to tell us why.
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode655
lib/Sema/AnalysisBasedWarnings.cpp:655: typedef llvm::po_iterator<const CFG*, CFGBlockSet, true> dfpo_iterator;
On 2011/08/22 18:58:03, delesley wrote:
> On 2011/08/19 23:54:06, chandlerc wrote:
> > dfpo? Whynot just 'po_iterator?
>
> Because I was uncomfortable with a naming conflict between the local po_iterator
> and llvm::po_iterator.
ah, ok. Chandler still votes for po_iterator, so that is what we have now.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode659
lib/Sema/AnalysisBasedWarnings.cpp:659: void Initialize(const CFG *Cfg) {
On 2011/08/22 18:58:03, delesley wrote:
> It's not inlined, because I'm copying the RPO example code.
>
> On 2011/08/19 23:54:06, chandlerc wrote:
> > maybe just 'Graph' as the name?
> >
> > Also, why isn't this just inlined into the constructor?
>
Ok. Changed to be inlined.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode663
lib/Sema/AnalysisBasedWarnings.cpp:663: for (dfpo_iterator I = dfpo_iterator::begin(Cfg, BSet),
On 2011/08/22 18:58:03, delesley wrote:
> See comment on line 640, above: the set in po_iterator storage should be a
> reference.
>
> On 2011/08/22 18:30:16, supertri wrote:
> > I think we need to pass a CFGBlockSet into the constructor for po_storage.
> > DeLeseley, any additional comments on this?
> >
> > On 2011/08/19 23:54:06, chandlerc wrote:
> > > Does this copy the BSet? You create a CFGBlockSet member in the po_storage
> > > above, I expected that to provide all the storage necessary. Why do we need
> a
> > > local variable?
> >
>
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode739
lib/Sema/AnalysisBasedWarnings.cpp:739: class Lock {
On 2011/08/22 21:52:12, chandlerc wrote:
> On 2011/08/22 18:30:16, supertri wrote:
> > I think it is short, to the point, and makes sense in this context. We track a
> > set (a lockset) of what locks we have currently acquired. It makes sense to me
> > to call it a lockset and to call the things that go into it locks. Do you have
> > another suggestion?
>
> LockSignature? I dunno. Maybe others have better names here. My problem is that
> this *isn't* a lock. It models a lock in the users source code by a specific set
> of assumed-to-be-unique aspects. I might actually call this "LockID" (note,
> capitalized 'D') and call the inner vector "DeclSeq" or some such. The inner
> construct is an implementation detail. The class itself models a way to identify
> a lock (not an actual lock).
Changed to LockID and DeclSeq.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode831
lib/Sema/AnalysisBasedWarnings.cpp:831: class BuildLockset : public EvaluatedExprVisitor<BuildLockset> {
Yes, we do. For this patch it does not matter, the change to StmtVisitor is important when we implement the guardedby/var stuff in a subsequent patch -- and it currently lives in that patch.
On 2011/08/22 21:52:12, chandlerc wrote:
> On 2011/08/22 18:30:16, supertri wrote:
> > I think we do for now. It definitely makes it easier to structure. I will add
> a
> > fixme to revisit this question later.
>
> I was really asking whether we want to use StmtVisitor rather than
> EvaluatedExprVisitior.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode892
lib/Sema/AnalysisBasedWarnings.cpp:892:
> That is not the pervailing LLVM or Clang style. Consistency is the goal here.
Deleted the initial blank line.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode896
lib/Sema/AnalysisBasedWarnings.cpp:896: ExclusiveLockFunctionAttr *ELF =
On 2011/08/22 21:52:12, chandlerc wrote:
> On 2011/08/22 18:30:16, supertri wrote:
> > ?
>
> The variable name 'ELF' is both confusing (there are many things to do w/ ELF,
> most of them are no ExclusiveLockFunctions), and misses key information: this is
> an *attribute*, not the exclusive lock function itself. maybe "ExclusiveAttr"?
Changed to ELFAttr.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode900
lib/Sema/AnalysisBasedWarnings.cpp:900: AddLock(ExpLocation, Parent);
On 2011/08/22 21:52:12, chandlerc wrote:
> On 2011/08/22 18:30:16, supertri wrote:
> > In that case args_begin() == args_end(), so we will not execute any iterations
> > of the loop.
>
> But we shouldn't compute that the size is zero twice. What if this is a linked
> list, and size has linear complexity?
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode930
lib/Sema/AnalysisBasedWarnings.cpp:930: //FIXME: else if (isa <LockReturnedAttr> (ArgAttrs[i])) { // rewrite rules
> See my comment above. The request to move was specifically tied to pro-actively
> adding a case. I want the comment attached to some actual C++ code that will be
> encountered again when fixing. Currently they aren't. I'm happy attaching them
> to the switch itself, or to an empty (stub) case. I prefer the latter.
Deleted comments.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1081
lib/Sema/AnalysisBasedWarnings.cpp:1081: // Process the current CFG block.
On 2011/08/22 21:52:12, chandlerc wrote:
> On 2011/08/22 18:30:16, supertri wrote:
> > I would still like to keep this comment.
>
> But why? What value does it add? I try to only add comments when there is some
> piece of information the reader is likely to be missing, in the hope that the
> comments are actually read... but fine, i'll desist..
Done.
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1091
lib/Sema/AnalysisBasedWarnings.cpp:1091: EntryLocksets[CurrBlockID] = Entryset;
On 2011/08/22 21:52:12, chandlerc wrote:
> On 2011/08/22 18:30:16, supertri wrote:
> > See above comment.
>
> I see why we don't *need* to do this, it would seem strictly cleaner to me to
> make Entryset and Exitset be references to the elements of the vector / array we
> already have created, and mutate them in place.
>
> Even though copying relatively cheap, there is no reason to even bother in this
> case.
Done.
http://codereview.appspot.com/4894048/diff/10001/include/clang/Basic/DiagnosticSemaKinds.td
File include/clang/Basic/DiagnosticSemaKinds.td (right):
http://codereview.appspot.com/4894048/diff/10001/include/clang/Basic/DiagnosticSemaKinds.td#newcode1321
include/clang/Basic/DiagnosticSemaKinds.td:1321: def warn_unlock_but_no_acquire : Warning<
On 2011/08/22 21:52:12, chandlerc wrote:
> At some point I think we should consistently use "lock" and "unlock" as verbs,
> or use "acquire" and "release" as verbs, relegating "lock" to a noun. Follow-up
> patch is good for this, and maybe the one switching to C++0x's naming
> conventions would be a good time to fix thise.
Added Fixme.
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp
File lib/Sema/AnalysisBasedWarnings.cpp (right):
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode712
lib/Sema/AnalysisBasedWarnings.cpp:712: /// "x" is substituted for "this" so we get x->mu();
On 2011/08/22 22:02:52, delesley wrote:
> "Free variable" is a standard term used the type system literature. It includes
> "this", as well as any arguments to functions, e.g. l is a free variable in
> l->mu:
>
> foo(MyList *l) EXCLUSIVE_LOCKS_REQUIRED(l->mu) { ... }
> MyList *myl;
> foo(myl); // requires lock myl->mu to be held
>
> Notice that l is replaced with myl, in the exact same way that "this" is
> replaced with the target object.
>
> On 2011/08/22 21:52:12, chandlerc wrote:
> > Is 'this' the only variable so substituted? Essentially, I'm not sure what you
> > mean by 'free-variables'.
> >
> > Are there other things (besides 'this') that are substituted? How does this
> > work?
>
I added a second example and removed academic terminology (as per Chandler's request). Please take a look.
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode785
lib/Sema/AnalysisBasedWarnings.cpp:785: SourceLocation AcquireLoc;
On 2011/08/22 21:52:12, chandlerc wrote:
> Why not just map to the source location? I don't see any real indicators of
> future information you're expecting to add.
We will be adding info for shared vs. exclusive locks.
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode812
lib/Sema/AnalysisBasedWarnings.cpp:812: /// \brief This is a visitor class which visits each expression in a CFGBlock.
On 2011/08/22 21:52:12, chandlerc wrote:
> This doesn't seem to match the API. It doesn't have anything to do with a
> CFGBlock. Maybe just "each expression in a Stmt"?
Comment changed.
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode897
lib/Sema/AnalysisBasedWarnings.cpp:897: // FIXME: shared/trylock variations
On 2011/08/22 21:52:12, chandlerc wrote:
> This FIXME is now in a weird location. Why here? Is there some other comment we
> could attach it to that would make it more likely to be, well, fixed.
Removed.
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode924
lib/Sema/AnalysisBasedWarnings.cpp:924: // the lockset in deterministic order.
On 2011/08/22 21:52:12, chandlerc wrote:
> This should be a doxyment. Also, it's not immediately obvious why this typedef
> is relevant to the requirement in this comment. Maybe attach the comment (and
> expand on it) to the EmitDiagnostics function.
Done.
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode949
lib/Sema/AnalysisBasedWarnings.cpp:949: /// locks that are not contained in the intersection.
On 2011/08/22 21:52:12, chandlerc wrote:
> You still need to make this more precise: "for any locks *in the union* that are
> not contained in the intersection."
>
> Or you can just say that you're warning on the symmetric difference, as that's
> how it is defined.
Done.
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode998
lib/Sema/AnalysisBasedWarnings.cpp:998: /// a warning if the two locksets are unequal.
On 2011/08/22 21:52:12, chandlerc wrote:
> You've repeated all of the body of this comment here. ;] Brief sections can
> really be brief:
>
> "Warn about different locksets in backedges of loops." or something...
Done.
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1001
lib/Sema/AnalysisBasedWarnings.cpp:1001: /// \param LoopEndSet Locks held in the last CFG block of the loop
On 2011/08/22 21:52:12, chandlerc wrote:
> This is using the old name.
>
> I still find these names confusing... but oh well.
Done.
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1009
lib/Sema/AnalysisBasedWarnings.cpp:1009: assert(FirstLocInLoop.isValid()); // FIXME: fail gracefully?
On 2011/08/22 21:52:12, chandlerc wrote:
> Why FIXME? It seems like we want to always have a location, and asserting until
> we find all the edge cases that don't is reasonable.
Done.
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1043
lib/Sema/AnalysisBasedWarnings.cpp:1043: /// The main thread safety loop. Each block in the CFG is traversed exactly
On 2011/08/22 21:52:12, chandlerc wrote:
> Your brief comment is longer than the rest of the comments. =/ And "The main
> thread safety loop" doesn't really help the reader much. Maybe as the brief:
> "Check a function's CFG for thread-safety violations." Then the details can
> follow.
Done.
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode1110
lib/Sema/AnalysisBasedWarnings.cpp:1110: for (CFGBlock::const_iterator I = CurrBlock->begin(), E = CurrBlock->end();
On 2011/08/22 21:52:12, chandlerc wrote:
> It would seem nice to not shadow I and E here... BI, BE?
Done.
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/SemaDeclAttr.cpp
File lib/Sema/SemaDeclAttr.cpp (right):
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/SemaDeclAttr.cpp#newcode274
lib/Sema/SemaDeclAttr.cpp:274: bool checkExpLockableObj(Sema & S, Decl *D, const AttributeList & Attr,
On 2011/08/22 21:52:12, chandlerc wrote:
> Please fix the coding conventions in this function, as commented repeatedly in
> prior reviews:
>
> doxyments.
> indentation.
> space after &
> variable names
Done.
Message from unknown
2011-08-23T17:49:12+00:00supertriurn:md5:0ce4a8e77cdd6311dddb70690f42db1d
Message from chandlerc@gmail.com
2011-08-23T18:10:04+00:00chandlercurn:md5:422683c19e3269e31b80aa45d6994e4e
Essentially cosmetic comments.
Please re-write the description of whats going in to really carefully explain what you're committing here as the patch has grown. Feel free to commit once you have that and have addressed the cosmetic issues below. We can handle the rest in post-commit review.
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarnings.cpp
File lib/Sema/AnalysisBasedWarnings.cpp (right):
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarnings.cpp#newcode607
lib/Sema/AnalysisBasedWarnings.cpp:607:
nuke blank line
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarnings.cpp#newcode666
lib/Sema/AnalysisBasedWarnings.cpp:666: /// \class Lock
Stale comment. You can actually delete this line, Doxygen should pick it up by looking below.
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarnings.cpp#newcode798
lib/Sema/AnalysisBasedWarnings.cpp:798: /// \class BuildLockset
Probably can remove this too
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarnings.cpp#newcode996
lib/Sema/AnalysisBasedWarnings.cpp:996: const Lockset LoopEntrySet,
indent is off
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarnings.cpp#newcode1063
lib/Sema/AnalysisBasedWarnings.cpp:1063: // Create a default initial lockset in case there are no predecessors.
This is no longer quite accurate... maybe just note that we already *have* a default initial lockset.
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarnings.cpp#newcode1099
lib/Sema/AnalysisBasedWarnings.cpp:1099: for (CFGBlock::const_iterator BI = CurrBlock->begin(), BE = CurrBlock->end();
80 columns
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarnings.cpp#newcode1143
lib/Sema/AnalysisBasedWarnings.cpp:1143:
Can probably nix 2 of the 4 blank lines.
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp
File lib/Sema/SemaDeclAttr.cpp (right):
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#newcode275
lib/Sema/SemaDeclAttr.cpp:275: /// resolves to a lockable object.
Join with the previous line.
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#newcode279
lib/Sema/SemaDeclAttr.cpp:279: // Flag error if could not get record type for this argument.
no 2 spaces after '//'
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#newcode299
lib/Sema/SemaDeclAttr.cpp:299: int Sidx = 0,
Sidx? Huh?
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#newcode301
lib/Sema/SemaDeclAttr.cpp:301: for(unsigned int Idx = Sidx; Idx < Attr.getNumArgs(); ++Idx) {
s/unsigned int/unsigned/
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#newcode307
lib/Sema/SemaDeclAttr.cpp:307: QualType Arg_QT = ArgExp->getType();
Arg_QT -> ArgTy
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#newcode309
lib/Sema/SemaDeclAttr.cpp:309: // Get record type.
This doesn't seem to to add information reall...
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#newcode313
lib/Sema/SemaDeclAttr.cpp:313: // Now check if we idx into a record type function param.
s/idx/index/
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#newcode320
lib/Sema/SemaDeclAttr.cpp:320: uint64_t ParamIdx_from1 = ArgValue.getZExtValue();
ParamIdxFromOne?
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#newcode321
lib/Sema/SemaDeclAttr.cpp:321: uint64_t ParamIdx_from0 = ParamIdx_from1 - 1;
ParamIdxFromZero?
Message from supertri@google.com
2011-08-23T18:26:04+00:00supertriurn:md5:31590738ea52920befb9bf6285ee7ac3
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarnings.cpp
File lib/Sema/AnalysisBasedWarnings.cpp (right):
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarnings.cpp#newcode607
lib/Sema/AnalysisBasedWarnings.cpp:607:
On 2011/08/23 18:10:04, chandlerc wrote:
> nuke blank line
Done.
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarnings.cpp#newcode666
lib/Sema/AnalysisBasedWarnings.cpp:666: /// \class Lock
On 2011/08/23 18:10:04, chandlerc wrote:
> Stale comment. You can actually delete this line, Doxygen should pick it up by
> looking below.
Done.
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarnings.cpp#newcode798
lib/Sema/AnalysisBasedWarnings.cpp:798: /// \class BuildLockset
On 2011/08/23 18:10:04, chandlerc wrote:
> Probably can remove this too
Done.
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarnings.cpp#newcode996
lib/Sema/AnalysisBasedWarnings.cpp:996: const Lockset LoopEntrySet,
On 2011/08/23 18:10:04, chandlerc wrote:
> indent is off
Done.
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarnings.cpp#newcode1063
lib/Sema/AnalysisBasedWarnings.cpp:1063: // Create a default initial lockset in case there are no predecessors.
On 2011/08/23 18:10:04, chandlerc wrote:
> This is no longer quite accurate... maybe just note that we already *have* a
> default initial lockset.
Done.
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarnings.cpp#newcode1099
lib/Sema/AnalysisBasedWarnings.cpp:1099: for (CFGBlock::const_iterator BI = CurrBlock->begin(), BE = CurrBlock->end();
On 2011/08/23 18:10:04, chandlerc wrote:
> 80 columns
Done.
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarnings.cpp#newcode1143
lib/Sema/AnalysisBasedWarnings.cpp:1143:
On 2011/08/23 18:10:04, chandlerc wrote:
> Can probably nix 2 of the 4 blank lines.
Done.
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp
File lib/Sema/SemaDeclAttr.cpp (right):
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#newcode275
lib/Sema/SemaDeclAttr.cpp:275: /// resolves to a lockable object.
On 2011/08/23 18:10:04, chandlerc wrote:
> Join with the previous line.
Done.
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#newcode279
lib/Sema/SemaDeclAttr.cpp:279: // Flag error if could not get record type for this argument.
On 2011/08/23 18:10:04, chandlerc wrote:
> no 2 spaces after '//'
Done.
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#newcode299
lib/Sema/SemaDeclAttr.cpp:299: int Sidx = 0,
On 2011/08/23 18:10:04, chandlerc wrote:
> Sidx? Huh?
Done.
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#newcode301
lib/Sema/SemaDeclAttr.cpp:301: for(unsigned int Idx = Sidx; Idx < Attr.getNumArgs(); ++Idx) {
On 2011/08/23 18:10:04, chandlerc wrote:
> s/unsigned int/unsigned/
Done.
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#newcode307
lib/Sema/SemaDeclAttr.cpp:307: QualType Arg_QT = ArgExp->getType();
On 2011/08/23 18:10:04, chandlerc wrote:
> Arg_QT -> ArgTy
Done.
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#newcode309
lib/Sema/SemaDeclAttr.cpp:309: // Get record type.
On 2011/08/23 18:10:04, chandlerc wrote:
> This doesn't seem to to add information reall...
Done.
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#newcode313
lib/Sema/SemaDeclAttr.cpp:313: // Now check if we idx into a record type function param.
On 2011/08/23 18:10:04, chandlerc wrote:
> s/idx/index/
Done.
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#newcode320
lib/Sema/SemaDeclAttr.cpp:320: uint64_t ParamIdx_from1 = ArgValue.getZExtValue();
On 2011/08/23 18:10:04, chandlerc wrote:
> ParamIdxFromOne?
Done.
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#newcode321
lib/Sema/SemaDeclAttr.cpp:321: uint64_t ParamIdx_from0 = ParamIdx_from1 - 1;
On 2011/08/23 18:10:04, chandlerc wrote:
> ParamIdxFromZero?
Done.