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