Add Subjects to NoDebugAttr [NFC]

[probinson]

Summary

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.

[dblaikie]

LGTM

[probinson]

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:[e-mail]]
Sent: Thursday, April 28, 2016 4:26 PM
To: [e-mail]; Robinson, Paul
Cc: Aaron Ballman; cfe-commits
Subject: Re: [PATCH] D19689: Add Subjects to NoDebugAttr [NFC]

LGTM

[aaron.ballman]

This turns out to be a bit of a mess: Alias and NoDebug use the same diagnostic kind text, neither of which match the actual subject list. Alias doesn't appertain to an obj-c method or property. NoDebug doesn't appertain to an obj-c property. The diagnostic text for that diagnostic kind doesn't match the subject list it is describing in either case. And the diagnostic text talks about "global variables" when what it really means are "variables that have global *storage*" and so static storage duration is a bit more accurate than "global variable" which implies that the following should diagnose:

void f() {
  static int i __attribute__((nodebug));
}

I have a WIP patch (D18768) that will hopefully make this situation better. Your current changes will add a bit of overhead to my refactoring, but it's a drop in the bucket compared to the other diagnostic wording changes, so I'm not overly concerned. I think the patch LGTM despite my comments.

include/clang/Basic/Attr.td

977 ↗

(On Diff #55482)

It's really strange that the diagnostic kind is ExpectedFunctionGlobalVarMethodOrProperty but the subject list does not have objective-c properties. It's even more strange that this diagnostic kind corresponds to the diagnostic text "functions and global variables" without mention of objective-c methods or properties. I see that the Alias attribute suffers from this same discombobulation.

test/Sema/attr-nodebug.c

6 ↗

(On Diff #55482)

Global variables is a misnomer though since this really applies to variables with static storage duration (including local variables).

[probinson]

I'll proceed from here without doing anything about the inconsistencies, because this particular one should go away and you're already doing something about the rest of it.

include/clang/Basic/Attr.td

977 ↗

(On Diff #55482)

I agree, but I didn't want to do anything about it because my next step is to replace GlobalVar with Var in the SubjectList, and therefore change the diagnostic enum to something else, making the inconsistency moot (at least for this attribute).

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 Subjects = SubjectList<[FunctionLike, ObjCMethod, GlobalVar], WarnDiag,
+                              "ExpectedFunctionGlobalVarMethodOrProperty">;
   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/DiagnosticSemaKinds.td

 def err_attribute_wrong_decl_type : Error<warn_attribute_wrong_decl_type.Text>;
 def warn_type_attribute_wrong_type : Warning<
   "'%0' only applies to %select{function|pointer|"
   "Objective-C object or block pointer}1 types; type here is %2">,
   InGroup<IgnoredAttributes>;
 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">,
   InGroup<IgnoredAttributes>;
 def err_attribute_vecreturn_only_vector_member : Error<
   "the vecreturn attribute can only be used on a class or structure with one member, which must be a vector">;
 def err_attribute_vecreturn_only_pod_record : Error<
   "the vecreturn attribute can only be used on a POD (plain old data) class or structure (i.e. no virtual functions)">;

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

void Sema::AddModeAttr(SourceRange AttrRange, Decl *D, IdentifierInfo *Name,
   else
     cast<ValueDecl>(D)->setType(NewTy);
 
   D->addAttr(::new (Context)
              ModeAttr(AttrRange, Context, Name, SpellingListIndex));
 }
 
 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()));
 }
 
 AlwaysInlineAttr *Sema::mergeAlwaysInlineAttr(Decl *D, SourceRange Range,
                                               IdentifierInfo *Ident,
                                               unsigned AttrSpellingListIndex) {

cfe/trunk/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' 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));
 
 void t2() __attribute__((nodebug(2))); // expected-error {{'nodebug' attribute takes no arguments}}

cfe/trunk/test/SemaObjC/attr-nodebug.m

+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+@interface NSObject
+- (void)doSomething __attribute__((nodebug));
+@end