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

[Richard Trieu]

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