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

[Richard Trieu]

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