[PATCH] D19754: Allow 'nodebug' on local variables

[David Blaikie via cfe-commits]

On Tue, May 3, 2016 at 4:38 PM, Paul Robinson via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> probinson marked 2 inline comments as done.
>
> ================
> Comment at: include/clang/Basic/Attr.td:86-88
> @@ -85,1 +85,5 @@
> +def NonParmVar : SubsetSubject<Var,
> +                               [{S->getKind() != Decl::ImplicitParam &&
> +                                 S->getKind() != Decl::ParmVar &&
> +                                 S->getKind() !=
> Decl::NonTypeTemplateParm}]>;
>  def NonBitField : SubsetSubject<Field,
> ----------------
> probinson wrote:
> > aaron.ballman wrote:
> > > Can you add tests for each of these cases to ensure that the
> diagnostic fires on all of them?
> > Actually not sure how to apply an attribute to an ImplicitParam....
> Well, this is all very exciting.  I tried
> ```
> template<__attribute__((nodebug)) int i> int t() { return i; }
> int g() { return t<2>(); }
> ```
> but got no diagnostic.  In fact. putting assert(false) in
> handleNoDebugAttr shows that it's never called.  Unsurprisingly, debug info
> for the template parameter still appears.
> Maybe nobody has ever been so foolish as to try to put an attribute on a
> template parameter before?  (It is mildly disturbing that putting an
> attribute in an apparently impossible place yields no diagnostic and no
> semantic effect.)
> I was obviously modeling this check on NormalVar, which (it turns out) is
> never used. And if you can't put attributes on template parameters or (it
> would seem likely) implicit parameters, then NonParmVar should check for
> nothing more than ParmVar.
> And that's what I'll do.
>
> (Marking as Done because the set of tests now matches the specified
> condition.)
>
> ================
> Comment at: test/CodeGenCXX/debug-info-nodebug.cpp:50
> @@ -49,1 +49,3 @@
>    NODEBUG static int static_local = 6;
> +  NODEBUG const  int const_local = 7;
> +  NODEBUG        int normal_local = 8;
> ----------------
> dblaikie wrote:
> > Doesn't look like the const case is any different from the non-const
> case, is it?
> Given a white-box analysis of the compiler internals, you are correct;
> this is in contrast to the static const/non-const handling, which *does*
> use different paths.
> I am unwilling to trust that the const/non-const paths for locals will
> forever use the same path.  You could also look at it as "the test is the
> spec."
>

But even then - what about any other property of a variable? What if it's
in a nested scope instead of a top level scope? What if it's declared in a
condition (if (int x = ...)). What if it's volatile? We could pick
arbitrary properties that /could/ have an effect on debug info but we
generally believe to be orthogonal to the feature at hand & I'd say 'const'
is in the same group of things. (a const local variable still needs
storage, etc, - the address can be escaped and accessed from elsewhere and
determined to be unique - and both variables in this example could be
optimized away entirely by the compiler because they're unused, so the
const is no worse off there in that theoretical concern)


>
> ================
> Comment at: test/CodeGenObjC/debug-info-nodebug.m:17
> @@ +16,3 @@
> +  // CHECK-NOT: !DILocalVariable(name: "strongSelf"
> +  __attribute__((nodebug)) __typeof(self) weakSelf = self;
> +  Block = [^{
> ----------------
> dblaikie wrote:
> > Is this case outside of the block interesting in some way? It doesn't
> look like it.
> The attribute on "weakSelf" is what triggers the second modified path in
> CGDebugInfo and suppresses the DILocalVariable for that name.
> The attribute on "strongSelf" goes through the normal EmitDeclare path.
> So in that sense, it is not interesting.
>
> I should not have been so hesitant to work out what was going on here,
> sorry about that.  My cluelessness about Objective-C knows no bounds.
> I'm changing the test to verify that the DILocalVariable does not appear,
> while the member info does still appear, and updating the comment to
> reflect this new knowledge.
>
>
> http://reviews.llvm.org/D19754
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160504/f26ec3a5/attachment.html>