PR21823: 'nodebug' attribute on global/static variables

[probinson]

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.

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

[aprantl]

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]

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

[aprantl]

Ah great, I didn't know that!

[dblaikie]

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.

[probinson]

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

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

[probinson]

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:[e-mail]]
Sent: Wednesday, April 27, 2016 3:43 PM
To: [e-mail]; Robinson, Paul
Cc: Aaron Ballman; Adrian Prantl; cfe-commits
Subject: Re: [PATCH] D19567: PR21823: 'nodebug' attribute on global/static variables

[dblaikie]

In 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

[dblaikie]

ping

[probinson]

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:[e-mail]]
Sent: Tuesday, May 17, 2016 3:05 PM
To: [e-mail]; David Blaikie
Cc: Robinson, Paul; Aaron Ballman; Adrian Prantl; cfe-commits
Subject: Re: [PATCH] D19567: PR21823: 'nodebug' attribute on global/static variables

ping

cfe/trunk/include/clang/Basic/Attr.td

 def NoCommon : InheritableAttr {
   let Spellings = [GCC<"nocommon">];
   let Subjects = SubjectList<[Var]>;
   let Documentation = [Undocumented];
 }
 
 def NoDebug : InheritableAttr {
   let Spellings = [GCC<"nodebug">];
-  let Documentation = [Undocumented];
+  let Documentation = [NoDebugDocs];
 }
 
 def NoDuplicate : InheritableAttr {
   let Spellings = [GNU<"noduplicate">, CXX11<"clang", "noduplicate">];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [NoDuplicateDocs];
 }
 

cfe/trunk/include/clang/Basic/AttrDocs.td

 merely want to document its non-standard retain and release semantics, should
 use the retaining behavior attributes (``ns_returns_retained``,
 ``ns_returns_not_retained``, etc).
 
 Query for this feature with ``__has_attribute(objc_method_family)``.
   }];
 }
 
+def NoDebugDocs : Documentation {
+  let Category = DocCatVariable;
+  let Content = [{
+The ``nodebug`` attribute allows you to suppress debugging information for a
+function, or for a variable declared with static storage duration, such as
+globals, class static data members, and static locals.
+  }];
+}
+
 def NoDuplicateDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 The ``noduplicate`` attribute can be placed on function declarations to control
 whether function calls to this function can be duplicated or not as a result of
 optimizations. This is required for the implementation of functions with
 certain special requirements, like the OpenCL "barrier" function, that might
 need to be run concurrently by all the threads that are executing in lockstep

cfe/trunk/lib/CodeGen/CGDebugInfo.cpp

else {
 
     // Field number for non-static fields.
     unsigned fieldNo = 0;
 
     // Static and non-static members should appear in the same order as
     // the corresponding declarations in the source program.
     for (const auto *I : record->decls())
       if (const auto *V = dyn_cast<VarDecl>(I)) {
+        if (V->hasAttr<NoDebugAttr>())
+          continue;
         // Reuse the existing static member declaration if one exists
         auto MI = StaticDataMemberCache.find(V->getCanonicalDecl());
         if (MI != StaticDataMemberCache.end()) {
           assert(MI->second &&
                  "Static data member declaration should still exist");
           elements.push_back(MI->second);
         } else {
           auto Field = CreateRecordStaticField(V, RecordTy, record);
GV = DBuilder.createGlobalVariable(DContext, FieldName, LinkageName, Unit,
                                        Var->hasInternalLinkage(), Var, nullptr);
   }
   return GV;
 }
 
 void CGDebugInfo::EmitGlobalVariable(llvm::GlobalVariable *Var,
                                      const VarDecl *D) {
   assert(DebugKind >= codegenoptions::LimitedDebugInfo);
+  if (D->hasAttr<NoDebugAttr>())
+    return;
   // Create global variable debug descriptor.
   llvm::DIFile *Unit = nullptr;
   llvm::DIScope *DContext = nullptr;
   unsigned LineNo;
   StringRef DeclName, LinkageName;
   QualType T;
   collectVarDeclProps(D, Unit, LineNo, T, DeclName, LinkageName, DContext);
 
GV = DBuilder.createGlobalVariable(
         getOrCreateStaticDataMemberDeclarationOrNull(D));
   }
   DeclCache[D->getCanonicalDecl()].reset(static_cast<llvm::Metadata *>(GV));
 }
 
 void CGDebugInfo::EmitGlobalVariable(const ValueDecl *VD,
                                      llvm::Constant *Init) {
   assert(DebugKind >= codegenoptions::LimitedDebugInfo);
+  if (VD->hasAttr<NoDebugAttr>())
+    return;
   // Create the descriptor for the variable.
   llvm::DIFile *Unit = getOrCreateFile(VD->getLocation());
   StringRef Name = VD->getName();
   llvm::DIType *Ty = getOrCreateType(VD->getType(), Unit);
   if (const EnumConstantDecl *ECD = dyn_cast<EnumConstantDecl>(VD)) {
     const EnumDecl *ED = cast<EnumDecl>(ECD->getDeclContext());
     assert(isa<EnumType>(ED->getTypeForDecl()) && "Enum without EnumType?");
     Ty = getOrCreateType(QualType(ED->getTypeForDecl(), 0), Unit);

cfe/trunk/test/CodeGenCXX/debug-info-nodebug.cpp

+// RUN: %clang_cc1 -DSETNODEBUG=0 -emit-llvm -debug-info-kind=limited %s -o - | FileCheck %s --check-prefix=YESINFO
+// RUN: %clang_cc1 -DSETNODEBUG=1 -emit-llvm -debug-info-kind=limited %s -o - | FileCheck %s --check-prefix=NOINFO
+
+#if SETNODEBUG
+#define NODEBUG __attribute__((nodebug))
+#else
+#define NODEBUG
+#endif
+
+// Simple global variable declaration and definition.
+// Use the declaration so it gets emitted.
+NODEBUG int global_int_decl;
+NODEBUG int global_int_def = global_int_decl;
+// YESINFO-DAG: !DIGlobalVariable(name: "global_int_decl"
+// NOINFO-NOT:  !DIGlobalVariable(name: "global_int_decl"
+// YESINFO-DAG: !DIGlobalVariable(name: "global_int_def"
+// NOINFO-NOT:  !DIGlobalVariable(name: "global_int_def"
+
+// Const global variable. Use it so it gets emitted.
+NODEBUG static const int const_global_int_def = 1;
+void func1(int);
+void func2() { func1(const_global_int_def); }
+// YESINFO-DAG: !DIGlobalVariable(name: "const_global_int_def"
+// NOINFO-NOT:  !DIGlobalVariable(name: "const_global_int_def"
+
+// Global variable with a more involved type.
+// If the variable has no debug info, the type should not appear either.
+struct S1 {
+  int a;
+  int b;
+};
+NODEBUG S1 global_struct = { 2, 3 };
+// YESINFO-DAG: !DICompositeType({{.*}} name: "S1"
+// NOINFO-NOT:  !DICompositeType({{.*}} name: "S1"
+// YESINFO-DAG: !DIGlobalVariable(name: "global_struct"
+// NOINFO-NOT:  !DIGlobalVariable(name: "global_struct"
+
+// Static data members. Const member needs a use.
+struct S2 {
+  NODEBUG static int static_member;
+  NODEBUG static const int static_const_member = 4;
+};
+int S2::static_member = 5;
+int junk = S2::static_const_member;
+// YESINFO-DAG: !DIGlobalVariable(name: "static_member"
+// NOINFO-NOT:  !DIGlobalVariable(name: "static_member"
+// YESINFO-DAG: !DIDerivedType({{.*}} name: "static_const_member"
+// NOINFO-NOT:  !DIDerivedType({{.*}} name: "static_const_member"
+
+// Function-local static variable.
+void func3() {
+  NODEBUG static int func_local = 6;
+}
+// YESINFO-DAG: !DIGlobalVariable(name: "func_local"
+// NOINFO-NOT:  !DIGlobalVariable(name: "func_local"