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