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:dblai...@gmail.com]
Sent: Wednesday, April 27, 2016 3:43 PM
To: reviews+d19567+public+1ee0c82c0824b...@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@lists.llvm.org<mailto:cfe-commits@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@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
