On Nov 15, 2012, at 5:43 PM, Richard Trieu <[email protected]> wrote: > On Thu, Nov 15, 2012 at 11:55 AM, John McCall <[email protected]> wrote: > On Nov 15, 2012, at 11:47 AM, Richard Trieu <[email protected]> wrote: >> On Thu, Nov 15, 2012 at 12:58 AM, John McCall <[email protected]> wrote: >> On Nov 14, 2012, at 7:37 PM, Richard Trieu <[email protected]> wrote: >>> On Wed, Nov 14, 2012 at 7:17 PM, John McCall <[email protected]> wrote: >>> On Nov 14, 2012, at 5:04 PM, Richard Trieu <[email protected]> wrote: >>>> On Mon, Aug 27, 2012 at 4:18 PM, Richard Trieu <[email protected]> wrote: >>>> On Tue, Jun 19, 2012 at 2:49 PM, Richard Trieu <[email protected]> wrote: >>>> On Fri, Jun 8, 2012 at 6:41 PM, John McCall <[email protected]> wrote: >>>> On Jun 8, 2012, at 5:47 PM, Richard Trieu wrote: >>>>> On Fri, Jun 8, 2012 at 4:29 PM, John McCall <[email protected]> wrote: >>>>> On Jun 8, 2012, at 4:24 PM, Richard Trieu wrote: >>>>> > Add a bit to VarDecl that is set to true when its address is taken or >>>>> > it gets passed by non-const reference. >>>>> >>>>> Just out of curiosity, is this bit set for variables of class type that >>>>> are used as an operand of a trivial copy constructor or copy assignment >>>>> operator? >>>>> >>>>> John. >>>>> >>>>> No, the bit is not currently set for those uses. >>>> >>>> Then please document the desired semantics more accurately, like "the >>>> address is taken or bound to a reference other than either operand of a >>>> trivial copy operation" or whatever semantics we actually settle on. >>>> >>>> Please document that this is only accurate when all possible accesses have >>>> been seen. For example, it is only accurate for local variables after >>>> their scope ends, and it is only accurate for global variables at the end >>>> of the translation unit and only if they have internal linkage. It is >>>> also basically meaningless within templates. >>>> >>>> Please document that this is only meant to be a conservative approximation >>>> of whether the address is used; it may be true in situations when the >>>> address is actually statically provable to not escape. Alas, your patch >>>> does not implement a conservative approximation. Among other things, >>>> you're only setting this bit in direct uses, but you really do need to be >>>> looking through anything that propagates reference-ness, including member >>>> accesses, base/derived casts, l-value reinterpreting casts, comma >>>> operators, and conditional operators; I do not claim that this list is >>>> exhaustive. Other things which take references to objects are lambda >>>> by-reference captures and member calls on the object; this, too, I do not >>>> claim to be exhaustive. >>>> >>>> That leads into the very interesting question of how we should validate >>>> the correctness of this bit. We really can't rely on it until we have >>>> some sort of validation story. >>>> >>>> Arguably, this bit should never be set on a variable of reference type. >>>> >>>> + /// \brief Whether this variable has its address taken or is >>>> referenced. >>>> + unsigned HasReference : 1; >>>> >>>> + /// Whether this variable has its address taken or is referenced. >>>> + bool hasReference() const { return VarDeclBits.HasReference; } >>>> + void setReference(bool ref) { VarDeclBits.HasReference = ref; } >>>> >>>> This is a poor choice of names; it is confusable with both the concept of >>>> a variable of reference type and the concept of whether a declaration has >>>> been referenced in the translation unit. I would go with isAddressTaken / >>>> setAddressTaken, with the C++ behavior with references being understood >>>> (and documented). >>>> >>>> Also, the semantics you're actually tracking are quite a bit more subtle >>>> than whether the address it taken, because you're specifically ignoring >>>> initializations of const references. That's quite suspect and quite a bit >>>> less generically useful. Also note that it is not illegal to bind a const >>>> reference to something and then modify it through that reference, either >>>> due to a mutable field or due to the constness being cast away. >>>> >>>> setAddressTaken can default its argument to true. >>>> >>>> You are not initializing this field to false. >>>> >>>> You are not serializing and deserialization this field. The ParmVarDecl >>>> abbreviation should assume this bit is 'false', I guess. >>>> >>>> --- lib/Sema/SemaInit.cpp (revision 158223) >>>> +++ lib/Sema/SemaInit.cpp (working copy) >>>> @@ -6211,6 +6211,12 @@ >>>> Expr *InitE = Init.get(); >>>> assert(InitE && "No initialization expression?"); >>>> >>>> + QualType QT = Entity.getType(); >>>> + if (QT->isReferenceType() && >>>> !QT.getNonReferenceType().isConstQualified()) >>>> + if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(InitE->IgnoreParens())) >>>> + if (VarDecl *VD = dyn_cast<VarDecl>(DRE->getDecl())) >>>> + VD->setReference(true); >>>> >>>> This is not an appropriate place for this check; not everything goes >>>> through PerformCopyInitialization. This needs to happen in >>>> InitializationSequence::Perform for a reference-binding step. >>>> >>>> + if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(op)) >>>> + if (VarDecl *VD = dyn_cast<VarDecl>(DRE->getDecl())) >>>> + VD->setReference(true); >>>> >>>> You're not even looking through dyn_casts here. You should extract out a >>>> function which walks an expression and marks variables that are bound in >>>> this way. Alternatively, you may be able to piggy-back on the C++11 >>>> ODR-use code. >>>> >>>> John. >>>> >>>> Another shot at adding an AddressTaken bit. A warning was created to show >>>> which part of the code construct was responsible for setting the >>>> AddressTaken bit. At the point where a reference (const or not) is bound, >>>> or an address is taken, a recursive function is called that will traverse >>>> the expression and set this bit for matching VarDecls. Also, the >>>> ASTReaderDecl and ASTWriterDecl have been updated for this bit. >>>> >>>> Currently, the analysis doesn't go into class method calls and is one of >>>> the unhandled ways for the address to leak. Classes weren't important to >>>> my original use of this bit, but I'm open to suggestions on how to handle >>>> them. >>>> >>>> Ping? >>>> >>>> Ping again. >>> >>> I still don't know what this bit is actually trying to record. >>> >>> John. >>> >>> The bit specifies if the variable is accessible from somewhere else in the >>> program. The two ways to determine this are either the address of the >>> variable is taken or there is a reference to the variable. >>> >>> This bit will be used to augment -Wloop-analysis to check while loops. >>> There is a common pattern with threaded code that currently gives a false >>> positive. >> >> Then your warning is written too aggressively. >> >> I am not at all convinced that this is generically useful enough to put into >> the AST. >> >> John. >> >> For for-loops, this isn't too aggressive. For while loops, it is too >> aggressive, hence why I'm trying to get this bit into VarDecls before >> enabling the warning on while loops. >> >> Do you have a different suggestion on how else to detect these cases? > > I don't even know what your warning is about; -Wloop-analysis is an almost > entirely useless name for a diagnostic group. But it sounds like you > basically want to do a data flow analysis. You should do this on the CFG, > and quite possibly in the static analyzer. > > John. > > Two questions. First, is this the kind of warning that is wanted in the > static analyzer? And second, won't using a CFG-based approach be more > expensive and make this warning less likely to be used? The CFG approach > would require processing everything while the AST-based approach only > requires processing the loops.
Okay, so your warning seems to be about informing users that their loop conditions are invariant within the loop? -Wloop-analysis is a really poor name for that. This seems like a very reasonable warning for the static analyzer to me. There are certainly disadvantages to using static analysis instead of a compiler warning, but in this case I think the difference in quality, plus the relative unlikelihood of the bug, justify it. Also, any problem you have with 'while' loops, you necessarily also have with (strictly more general!) 'for' loops. So you need to think of this as a generic bug in your analysis, even though you may only have test cases so far with 'while' loops. Also, it is not necessarily the case that, at the time of processing a loop, you will have seen the code which takes the address of a variable. So it's not clear to me that you can do this online; you need to have seen the entire function body. John.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
