[PATCH] D19567: PR21823: 'nodebug' attribute on global/static variables

[David Blaikie via cfe-commits]

ping

On Mon, May 2, 2016 at 11:51 AM, David Blaikie via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> dblaikie added a comment.
>
> In http://reviews.llvm.org/D19567#414906, @probinson wrote:
>
> > Huh.  There are strange interactions here, which makes me even more
> nervous about testing fewer cases.
>
>
> Generally this sort of thing makes me more interested in testing fewer
> cases so we can see/make sure they're properly covering, as you just did by
> the sounds of it. It's hard to see if everything's really covered if
> there's lots of redundant coverage that adds noise to the test case.
>
> > As it happens, the test as written did not exercise all 3 modified
> paths. Because 'struct S2' had all its members marked with nodebug, none of
> the static-member references caused debug info for the class as a whole to
> be attempted.
>
>
> Not sure I quite follow. Even without nodebug:
>
>   struct foo { static const int x = 3; int y; };
>   int i = foo::x;
>
> doesn't produce any debug info for 'foo', x, etc. Just for 'i'.
>
> >   I needed to add a variable of type S2 (without 'nodebug') in order to
> exercise that path.  Once that was done, then the modification to
> CollectRecordFields became necessary.
>
> >                 Even in an unmodified compiler, "static_const_member"
> never shows up as a DIGlobalVariable, although the other cases all do.  So,
> testing only for DIGlobalVariable wouldn't be sufficient to show that it
> gets suppressed by 'nodebug'.
>
>
> Have you tested cases where the static member is ODR used and/or defined:
>
>   struct foo { static const int x = 3; };
>   const int foo::x; // defined
>   void sink(void*);
>   void use() { sink(&foo::x); } // ODR used
>
> I'm guessing that the out of line definition will create the
> DIGlobalVariable you're not seeing. But probably through a common codepath
> you've already corrected for.
>
> The ODR use probably doesn't cause anything interesting to happen.
>
> >   "static_member" shows up unless we have modified both
> EmitGlobalVariable(VarDecl case) and CollectRecordFields, given that the
> test actually tries to emit debug info for S2 as a whole.
>
>
> I would imagine this could still boil down to: check-not DIGlobalVariable,
> check-not DIFlagStaticMember ?
>
> But once the test is smaller/more targeted, checking the extra details of
> the member list of a composite type, etc, seems OK.
>
> > So, the genuinely most-minimal did-this-change-do-anything test would
> need only "static_member" and "const_global_int_def" to show that each path
> had some effect.  This approach does not fill me with warm fuzzies, or the
> feeling that future changes in this area will not break the intended
> behavior.  What do you think?
>
> > --paulr
>
>
>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D19567
>
>
>
> _______________________________________________
> 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/20160517/ae7ffbcc/attachment.html>