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

[Richard Trieu]

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?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120827/f449e41e/attachment.html>