[cfe-commits] [Patch] Add Reference bit to VarDecl

[John McCall]

On Nov 14, 2012, at 7:37 PM, Richard Trieu <rtrieu at google.com> wrote:
> On Wed, Nov 14, 2012 at 7:17 PM, John McCall <rjmccall at apple.com> wrote:
> On Nov 14, 2012, at 5:04 PM, Richard Trieu <rtrieu at google.com> wrote:
>> On Mon, Aug 27, 2012 at 4:18 PM, Richard Trieu <rtrieu at google.com> wrote:
>> On Tue, Jun 19, 2012 at 2:49 PM, Richard Trieu <rtrieu at google.com> wrote:
>> On Fri, Jun 8, 2012 at 6:41 PM, John McCall <rjmccall at apple.com> wrote:
>> On Jun 8, 2012, at 5:47 PM, Richard Trieu wrote:
>>> On Fri, Jun 8, 2012 at 4:29 PM, John McCall <rjmccall at apple.com> 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.

Then your warning is written too aggressively.

I am not at all convinced that this is generically useful enough to put into the AST.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121115/f6e950ce/attachment.html>