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.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
