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

Robinson, Paul via cfe-commits cfe-commits at lists.llvm.org
Tue May 17 18:06:37 PDT 2016

About to be away for a week, but I will take this up (and the other one) when I get back.

From: David Blaikie [mailto:dblaikie at gmail.com]
Sent: Tuesday, May 17, 2016 3:05 PM
To: reviews+D19567+public+1ee0c82c0824bedd at reviews.llvm.org; David Blaikie
Cc: Robinson, Paul; Aaron Ballman; Adrian Prantl; cfe-commits
Subject: Re: [PATCH] D19567: PR21823: 'nodebug' attribute on global/static variables


On Mon, May 2, 2016 at 11:51 AM, David Blaikie via cfe-commits <cfe-commits at lists.llvm.org<mailto: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



cfe-commits mailing list
cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160518/8b157193/attachment-0001.html>

More information about the cfe-commits mailing list