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