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

[Eli Friedman]

On Wed, 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.
>
> 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