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

[David Blaikie via cfe-commits]

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