Allow 'nodebug' on local variables

[probinson]

Summary

Expand the purview of the 'nodebug' attribute from functions/methods and static-storage variables to include local variables. Parameters and members of aggregates are still excluded, and probably should stay that way.

[aaron.ballman]

include/clang/Basic/Attr.td

86–88

Can you add tests for each of these cases to ensure that the diagnostic fires on all of them?

[probinson]

include/clang/Basic/Attr.td

86–88

Actually not sure how to apply an attribute to an ImplicitParam....

[dblaikie]

test/CodeGenCXX/debug-info-nodebug.cpp

50

Doesn't look like the const case is any different from the non-const case, is it?

test/CodeGenObjC/debug-info-nodebug.m

17

Is this case outside of the block interesting in some way? It doesn't look like it.

[probinson]

include/clang/Basic/Attr.td

86–88

Well, this is all very exciting. I tried

template<__attribute__((nodebug)) int i> int t() { return i; }
int g() { return t<2>(); }

but got no diagnostic. In fact. putting assert(false) in handleNoDebugAttr shows that it's never called. Unsurprisingly, debug info for the template parameter still appears.
Maybe nobody has ever been so foolish as to try to put an attribute on a template parameter before? (It is mildly disturbing that putting an attribute in an apparently impossible place yields no diagnostic and no semantic effect.)
I was obviously modeling this check on NormalVar, which (it turns out) is never used. And if you can't put attributes on template parameters or (it would seem likely) implicit parameters, then NonParmVar should check for nothing more than ParmVar.
And that's what I'll do.

(Marking as Done because the set of tests now matches the specified condition.)

test/CodeGenCXX/debug-info-nodebug.cpp

50

Given a white-box analysis of the compiler internals, you are correct; this is in contrast to the static const/non-const handling, which *does* use different paths.
I am unwilling to trust that the const/non-const paths for locals will forever use the same path. You could also look at it as "the test is the spec."

test/CodeGenObjC/debug-info-nodebug.m

17

The attribute on "weakSelf" is what triggers the second modified path in CGDebugInfo and suppresses the DILocalVariable for that name.
The attribute on "strongSelf" goes through the normal EmitDeclare path. So in that sense, it is not interesting.

I should not have been so hesitant to work out what was going on here, sorry about that. My cluelessness about Objective-C knows no bounds.
I'm changing the test to verify that the DILocalVariable does not appear, while the member info does still appear, and updating the comment to reflect this new knowledge.

[probinson]

Correct the attribute condition, and refine the Objective-C test.

[aaron.ballman]

LGTM!

[cfe-commits]

This would be a great conversation to have at the social, sadly I will
have to miss it this month.

dblaikie wrote:

Doesn't look like the const case is any different from the non-const
case, is it?

Given a white-box analysis of the compiler internals, you are correct;
this is in contrast to the static const/non-const handling, which *does*
use different paths.
I am unwilling to trust that the const/non-const paths for locals will
forever use the same path.  You could also look at it as "the test is
the spec."

But even then - what about any other property of a variable? What if it's
in a nested scope instead of a top level scope? What if it's declared in
a condition (if (int x = ...)). What if it's volatile? We could pick
arbitrary properties that /could/ have an effect on debug info but we
generally believe to be orthogonal to the feature at hand

What you are describing is what testing literature refers to as criteria
for equivalence classes. There is some level of judgment to that, yes.

& I'd say 'const' is in the same group of things.

I would have thought exactly that for the static-storage case, but it is
demonstrably not true. Therefore, const/not is a valid distinction for
the equivalence classes in the static case. Needing a separate const
test for the static case, it seems completely appropriate to have the
same for the auto case. In other words, in my judgment the storage-class
doesn't seem relevant to the equivalence class criteria for the test.

(a const local variable still needs storage, etc, - the address can be
escaped and accessed from elsewhere and determined to be unique

All of those objections apply equally to the static case, and yet the
static case must have separate tests.

• and both variables in this example could be optimized away entirely

by the compiler because they're unused, so the const is no worse off
there in that theoretical concern)

Again not different for the (file-)static case. If a file-static
variable is not used in the CU there's no reason for it to be emitted.
As it happens I *did* need uses to get the static cases to work, and
(currently) don't need uses to get the local cases to work, so in the
interest of not including elements in the test case that are irrelevant
to the feature-under-test, I didn't add uses of the locals.

This is an argument for doing the test exactly as I did: first run it
to prove debug info IS emitted in the absence of the attribute, then
again to prove debug info is suppressed in the presence of the attribute.
That way if optimization or lack-of-use means the variable is not emitted,
the test can be adjusted to make sure that condition does not apply,
proving that the lack of debug info is properly because of the attribute
and not because of some other irrelevant circumstance.

--paulr

---

cfe-commits mailing list
[e-mail]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[probinson]

Removed the apparently redundant test for a const local variable.

Yes, I'm back to this after way longer than expected. I believe, for this patch specifically, the extra const local variable was the only identifiable problem. There were other issues regarding the overall test, which were partly discussed here and partly in D19567, but those were not specific to this patch.

@dblaikie, I propose that with the 'const' test removed, we allow this patch to proceed (which completes the implementation, something I would dearly like to have done before 3.9 branches) with the promise that I'll come back around to the broader topic of the test file in general as a follow-up.

[probinson]

test/CodeGenCXX/debug-info-nodebug.cpp

50

const case removed.

[dblaikie]

Sure, looks good - thanks. Will discuss the broader test issues later/separately.

include/clang/Basic/Attr.td

 
 // This is the type of a variable which C++11 allows alignas(...) to appertain
 // to.
 def NormalVar : SubsetSubject<Var,
                               [{S->getStorageClass() != VarDecl::Register &&
                                 S->getKind() != Decl::ImplicitParam &&
                                 S->getKind() != Decl::ParmVar &&
                                 S->getKind() != Decl::NonTypeTemplateParm}]>;
+def NonParmVar : SubsetSubject<Var,
+                               [{S->getKind() != Decl::ImplicitParam &&
+                                 S->getKind() != Decl::ParmVar &&
+                                 S->getKind() != Decl::NonTypeTemplateParm}]>;
 def NonBitField : SubsetSubject<Field,
                                 [{!S->isBitField()}]>;
 
 def ObjCInstanceMethod : SubsetSubject<ObjCMethod,
                                        [{S->isInstanceMethod()}]>;
 
 def ObjCInterfaceDeclInitMethod : SubsetSubject<ObjCMethod,
                                [{S->getMethodFamily() == OMF_init &&

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

include/clang/Basic/AttrDocs.td

 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.
+function or method, or for a variable that is not a parameter or a non-static
+data member.
   }];
 }
 
 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

lib/CodeGen/CGDebugInfo.cpp

return DBuilder.createStructType(Unit, "", Unit, 0, FieldOffset, 0, Flags,
                                    nullptr, Elements);
 }
 
 void CGDebugInfo::EmitDeclare(const VarDecl *VD, llvm::Value *Storage,
                               llvm::Optional<unsigned> ArgNo,
                               CGBuilderTy &Builder) {
   assert(DebugKind >= codegenoptions::LimitedDebugInfo);
   assert(!LexicalBlockStack.empty() && "Region stack mismatch, stack empty!");
+  if (VD->hasAttr<NoDebugAttr>())
+    return;
 
   bool Unwritten =
       VD->isImplicit() || (isa<Decl>(VD->getDeclContext()) &&
                            cast<Decl>(VD->getDeclContext())->isImplicit());
   llvm::DIFile *Unit = nullptr;
   if (!Unwritten)
     Unit = getOrCreateFile(VD->getLocation());
   llvm::DIType *Ty;

 void CGDebugInfo::EmitDeclareOfBlockDeclRefVariable(
     const VarDecl *VD, llvm::Value *Storage, CGBuilderTy &Builder,
     const CGBlockInfo &blockInfo, llvm::Instruction *InsertPoint) {
   assert(DebugKind >= codegenoptions::LimitedDebugInfo);
   assert(!LexicalBlockStack.empty() && "Region stack mismatch, stack empty!");
 
   if (Builder.GetInsertBlock() == nullptr)
     return;
+  if (VD->hasAttr<NoDebugAttr>())
+    return;
 
   bool isByRef = VD->hasAttr<BlocksAttr>();
 
   uint64_t XOffset = 0;
   llvm::DIFile *Unit = getOrCreateFile(VD->getLocation());
   llvm::DIType *Ty;
   if (isByRef)
     Ty = EmitTypeForVarWithBlocksAttr(VD, &XOffset);

test/CodeGenCXX/debug-info-nodebug.cpp

void func3() {
   S2 junk;
   func1(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.
+// Function-local static, const, and normal variables.
 void func4() {
   NODEBUG static int static_local = 6;
+  NODEBUG const  int const_local = 7;
+  NODEBUG        int normal_local = 8;
 }
 // YESINFO-DAG: !DIGlobalVariable(name: "static_local"
 // NOINFO-NOT:  !DIGlobalVariable(name: "static_local"
+// YESINFO-DAG: !DILocalVariable(name: "const_local"
+// NOINFO-NOT:  !DILocalVariable(name: "const_local"
+// YESINFO-DAG: !DILocalVariable(name: "normal_local"
+// NOINFO-NOT:  !DILocalVariable(name: "normal_local"

test/CodeGenObjC/debug-info-nodebug.m

+// RUN: %clang_cc1 -triple arm-apple-ios -emit-llvm -debug-info-kind=limited -fblocks  %s -o - | FileCheck %s
+// Objective-C code cargo-culted from debug-info-lifetime-crash.m.
+@protocol NSObject
+- (id)copy;
+@end
+@class W;
+@interface View1
+@end
+@implementation Controller {
+    void (^Block)(void);
+}
+- (void)View:(View1 *)View foo:(W *)W
+{
+  // Put one inside a block because that's a different path in the compiler.
+  // CHECK-NOT: !DILocalVariable(name: "weakSelf"
+  // CHECK-NOT: !DILocalVariable(name: "strongSelf"
+  __attribute__((nodebug)) __typeof(self) weakSelf = self;
+  Block = [^{
+    __attribute__((nodebug)) __typeof(self) strongSelf = weakSelf;
+    } copy];
+}
+@end

test/Sema/attr-nodebug.c

 // RUN: %clang_cc1 %s -verify -fsyntax-only
 
 int a __attribute__((nodebug));
 
-void b() {
-  int b __attribute__((nodebug)); // expected-warning {{'nodebug' attribute only applies to functions and global variables}}
+void b(int p __attribute__((nodebug))) { // expected-warning {{'nodebug' attribute only applies to variables and functions}}
+  int b __attribute__((nodebug));
 }
 
 void t1() __attribute__((nodebug));
 
 void t2() __attribute__((nodebug(2))); // expected-error {{'nodebug' attribute takes no arguments}}