[PATCH] D19689: Add Subjects to NoDebugAttr [NFC]

[Robinson, Paul via cfe-commits]

Generally tests test something other than "this program doesn't crash" - should it test that we apply the attribute correctly? (either via ast dump, or checking the resulting DWARF doesn't have debug info on the relevant entity)

Or is this not actually a new/separate codepath? In which case do we really need the test?

It's a –verify test which is about diagnostics, rather than not-crashing.  It parallels line 3 of Sema/attr-nodebug.c, which verifies the attribute can be applied to a function.  Without this test, you could remove "ObjCMethod" from the Subjects line and no test would fail.  I put "ObjCMethod" on the Subjects line because the hand-coded condition used "isFunctionOrMethod" which permits Objective-C methods.  The new test passes with old and new compilers, demonstrating the NFC claim.

Now, if we decide the 'nodebug' attribute should not apply to Objective-C, that's fine with me, in which case the new test should verify that a diagnostic *is* produced.

I am admittedly clueless about Objective-C, are they handled differently from other functions by the time we get to CGDebugInfo?  If there's another path I should tweak, I'd like to know about it.
Thanks,
--paulr

From: David Blaikie [mailto:dblaikie at gmail.com]
Sent: Thursday, April 28, 2016 4:26 PM
To: reviews+D19689+public+514682b5314c52ad at reviews.llvm.org; Robinson, Paul
Cc: Aaron Ballman; cfe-commits
Subject: Re: [PATCH] D19689: Add Subjects to NoDebugAttr [NFC]

LGTM

On Thu, Apr 28, 2016 at 2:10 PM, Paul Robinson via cfe-commits <cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>> wrote:
probinson created this revision.
probinson added a reviewer: aaron.ballman.
probinson added a subscriber: cfe-commits.

The 'nodebug' attribute had hand-coded constraints; replace those with a Subjects line in Attr.td.
Also add a missing test to verify the attribute is okay on an Objective-C method.

http://reviews.llvm.org/D19689

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaDeclAttr.cpp
  test/Sema/attr-nodebug.c
  test/SemaObjC/attr-nodebug.m

Index: test/SemaObjC/attr-nodebug.m
===================================================================
--- test/SemaObjC/attr-nodebug.m
+++ test/SemaObjC/attr-nodebug.m
@@ -0,0 +1,5 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+ at interface NSObject
+- (void)doSomething __attribute__((nodebug));
+ at end

Generally tests test something other than "this program doesn't crash" - should it test that we apply the attribute correctly? (either via ast dump, or checking the resulting DWARF doesn't have debug info on the relevant entity)

Or is this not actually a new/separate codepath? In which case do we really need the test?

Index: test/Sema/attr-nodebug.c
===================================================================
--- test/Sema/attr-nodebug.c
+++ test/Sema/attr-nodebug.c
@@ -3,7 +3,7 @@
 int a __attribute__((nodebug));

 void b() {
-  int b __attribute__((nodebug)); // expected-warning {{'nodebug' only applies to variables with static storage duration and functions}}
+  int b __attribute__((nodebug)); // expected-warning {{'nodebug' attribute only applies to functions and global variables}}
 }

 void t1() __attribute__((nodebug));
Index: lib/Sema/SemaDeclAttr.cpp
===================================================================
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -3572,18 +3572,6 @@
 }

 static void handleNoDebugAttr(Sema &S, Decl *D, const AttributeList &Attr) {
-  if (const VarDecl *VD = dyn_cast<VarDecl>(D)) {
-    if (!VD->hasGlobalStorage())
-      S.Diag(Attr.getLoc(),
-             diag::warn_attribute_requires_functions_or_static_globals)
-        << Attr.getName();
-  } else if (!isFunctionOrMethod(D)) {
-    S.Diag(Attr.getLoc(),
-           diag::warn_attribute_requires_functions_or_static_globals)
-      << Attr.getName();
-    return;
-  }
-
   D->addAttr(::new (S.Context)
              NoDebugAttr(Attr.getRange(), S.Context,
                          Attr.getAttributeSpellingListIndex()));
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -2504,9 +2504,6 @@
 def warn_incomplete_encoded_type : Warning<
   "encoding of %0 type is incomplete because %1 component has unknown encoding">,
   InGroup<DiagGroup<"encode-type">>;
-def warn_attribute_requires_functions_or_static_globals : Warning<
-  "%0 only applies to variables with static storage duration and functions">,
-  InGroup<IgnoredAttributes>;
 def warn_gnu_inline_attribute_requires_inline : Warning<
   "'gnu_inline' attribute requires function to be marked 'inline',"
   " attribute ignored">,
Index: include/clang/Basic/Attr.td
===================================================================
--- include/clang/Basic/Attr.td
+++ include/clang/Basic/Attr.td
@@ -973,6 +973,8 @@

 def NoDebug : InheritableAttr {
   let Spellings = [GCC<"nodebug">];
+  let Subjects = SubjectList<[FunctionLike, ObjCMethod, GlobalVar], WarnDiag,
+                              "ExpectedFunctionGlobalVarMethodOrProperty">;
   let Documentation = [NoDebugDocs];
 }




_______________________________________________
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/8d7880ac/attachment-0001.html>