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

Wed Apr 27 17:55:14 PDT 2016

Huh.  There are strange interactions here, which makes me even more nervous about testing fewer cases.
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.  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'.
               "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.

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

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

On Wed, Apr 27, 2016 at 3:24 PM, Paul Robinson via cfe-commits <cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>> wrote:
probinson added a comment.

In http://reviews.llvm.org/D19567#413997, @dblaikie wrote:

> For 3 code paths (that seem fairly independent from one another) I'd only really expect to see 3 variables in the test - one to exercise each codepath. What's the reason for the larger set of test cases?

I'm not interested in testing code-paths, I'm interested in testing a feature, and relying on the fact that (at the moment) only 3 code paths are involved is not really robust.  Granted there is some duplication, and I took that out (r267804), but there are more than 3 relevant cases from an end-user-feature perspective.

We generally have to assume /something/ about the implementation to constrain testing. eg: we don't assume that putting a variable inside a namespace would make a difference here so we don't go & test all these case inside a namespace as well as in the global namespace. So I'm not sure why we wouldn't go further down that path, knowing that there are only a small number of ways global variable definitions can impact debug info generation.

> Then it might be simpler just to include 6 variables, one of each kind with the attribute, one of each kind without. & I think you could check this reliably with:

>

> CHECK: DIGlobalVariable

>  CHECK-NEXT: "name"

>

> repeated three times with a CHECK-NOT: DIGlobalVariable at the end - that way the CHECK wouldn't skip past an existing variable, and you'd check you got the right ones. I think.

If I cared only about DIGlobalVariable that does sound like it would work.  Sadly, the static const data member comes out as a DIDerivedType, not a DIGlobalVariable.  Also, it's *really* important to us that the DICompositeType for "S1" is not present; that's actually where we get the major benefit.

But that codepath is already tested by any test for a TU that has a static member without a definition, right?

As for the composite type - this is much like testing in LLVM where we test that a certain inlining occurred when we test the inliner - not the knock-on effect that that has on some other optimization which we've already tested separately.

Once you have more than one kind of thing to look for (or -NOT look for) it gets a lot more tedious to show it's all correct in one pass like you are suggesting.

(Re. getting rid of the types:  We have licensees with major template metaprogramming going on, but if they can mark those variables 'nodebug' and consequently suppress the instantiated types from the debug info, they should see a major size win.  One licensee reported that reducing some name from 19 characters to 1 char got them a 5MB reduction. Imagine if they could get rid of all of those types...)

Repository:
  rL LLVM

http://reviews.llvm.org/D19567

_______________________________________________
cfe-commits mailing list
cfe-commits at lists.llvm.org<mailto: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/20160428/65745ed8/attachment-0001.html>