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

[David Blaikie via cfe-commits]

On Thu, Apr 28, 2016 at 4:46 PM, Robinson, Paul <paul.robinson at sony.com>
wrote:

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

Ah, OK - thanks for the context :)


>
>
> 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> 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
> 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/20160429/5f20548f/attachment.html>