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

[Eli Friedman]

On Wed, Nov 14, 2012 at 8:44 PM, Richard Trieu <rtrieu at google.com> wrote:
> On Wed, Nov 14, 2012 at 8:14 PM, Eli Friedman <eli.friedman at gmail.com>
> wrote:
>>
>> 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
>
>
> Normally, there would be a sleep statement or some other work in the loop.
> The point is that the loop could be interrupted by StartWork() changing the
> value of done, thus not an infinite loop.

Okay; just checking.

-Eli