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