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