On Wed, Nov 14, 2012 at 8:44 PM, Richard Trieu <[email protected]> wrote: > On Wed, Nov 14, 2012 at 8:14 PM, Eli Friedman <[email protected]> > wrote: >> >> On Wed, 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. >> > >> > bool done = false; >> > StartWork(&done); >> > while (!done) >> > ; >> >> That can't be the actual pattern; LLVM (and I assume gcc) will >> optimize this to an infinite loop. >> >> -Eli > > > Normally, there would be a sleep statement or some other work in the loop. > The point is that the loop could be interrupted by StartWork() changing the > value of done, thus not an infinite loop.
Okay; just checking. -Eli _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
