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