Page MenuHomePhabricator

PR21823: 'nodebug' attribute on global/static variables
ClosedPublic

Authored by probinson on Apr 26 2016, 4:18 PM.

Details

Summary

'nodebug' on a function will currently suppress all debug info for the function and its contents.
'nodebug on a static-duration variable will currently suppress debug info for the associated static initializer function (if any).
This patch suppresses all debug info for the variable, as requested in PR21823.

As a follow-up I'd like to expand applicability of 'nodebug' to any variable, and non-static data members.

Diff Detail

Repository
rL LLVM

Event Timeline

probinson updated this revision to Diff 55116.Apr 26 2016, 4:18 PM
probinson retitled this revision from to PR21823: 'nodebug' attribute on global/static variables.
probinson updated this object.
probinson added reviewers: dblaikie, aprantl.
probinson added a subscriber: cfe-commits.
aaron.ballman accepted this revision.Apr 26 2016, 4:58 PM
aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added a subscriber: aaron.ballman.

LGTM on the attribute part. Someone else should pipe up if the debug info part looks incorrect, but it looks reasonable to me.

include/clang/Basic/Attr.td
976 ↗(On Diff #55116)

This isn't your problem to fix (though I would not complain if you did fix it!), but the lack of a Subjects line should be fixed at some point.

This revision is now accepted and ready to land.Apr 26 2016, 4:58 PM
aprantl edited edge metadata.Apr 27 2016, 9:03 AM

Debug info changes otherwise look good to me.

test/CodeGenCXX/debug-info-nodebug.cpp
17 ↗(On Diff #55116)

I don't think you can chain to -NOT checks like that. You may need to run FileCheck once times for each negative check.

probinson added inline comments.Apr 27 2016, 9:50 AM
include/clang/Basic/Attr.td
976 ↗(On Diff #55116)

Okay.... I think that's more invasive than I want to be right now.
But I'm anticipating expanding the attribute to non-global variables, in which case adding the appropriate Subjects line could be the way to go.

test/CodeGenCXX/debug-info-nodebug.cpp
17 ↗(On Diff #55116)

The YESINFO checks are done in one run, the NOINFO checks are done in another run. So I'm not mixing -DAG with -NOT, which certainly doesn't have a sensible effect.
However, a sequence of -NOT checks will verify that none of the specified patterns appear in the range. In this test, all the NOINFO checks are -NOT so they all cover the entire output file. (And I know it works, because I missed one place in CGDebugInfo.cpp and the test caught it!)

Ah great, I didn't know that!

dblaikie edited edge metadata.Apr 27 2016, 10:31 AM

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?

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.

This revision was automatically updated to reflect the committed changes.

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.

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.
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...)

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@gmail.com]
Sent: Wednesday, April 27, 2016 3:43 PM
To: reviews+D19567+public+1ee0c82c0824bedd@reviews.llvm.org; Robinson, Paul
Cc: Aaron Ballman; Adrian Prantl; cfe-commits
Subject: Re: [PATCH] D19567: PR21823: 'nodebug' attribute on global/static variables

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

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

From: David Blaikie [mailto:dblaikie@gmail.com]
Sent: Tuesday, May 17, 2016 3:05 PM
To: reviews+D19567+public+1ee0c82c0824bedd@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

ping