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

[Richard Trieu]

On Thu, Nov 15, 2012 at 11:55 AM, John McCall <rjmccall at apple.com> wrote:

> On Nov 15, 2012, at 11:47 AM, Richard Trieu <rtrieu at google.com> wrote:
>
> On Thu, Nov 15, 2012 at 12:58 AM, John McCall <rjmccall at apple.com> wrote:
>
>> 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.
>>
>
> For for-loops, this isn't too aggressive.  For while loops, it is too
> aggressive, hence why I'm trying to get this bit into VarDecls before
> enabling the warning on while loops.
>
> Do you have a different suggestion on how else to detect these cases?
>
>
> I don't even know what your warning is about;  -Wloop-analysis is an
> almost entirely useless name for a diagnostic group.  But it sounds like
> you basically want to do a data flow analysis.  You should do this on the
> CFG, and quite possibly in the static analyzer.
>
> John.
>

Two questions.  First, is this the kind of warning that is wanted in the
static analyzer?  And second, won't using a CFG-based approach be more
expensive and make this warning less likely to be used?  The CFG approach
would require processing everything while the AST-based approach only
requires processing the loops.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121115/8ed37cd5/attachment.html>