<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 14 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
span.EmailStyle17
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri","sans-serif";}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal">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)<br>
<br>
Or is this not actually a new/separate codepath? In which case do we really need the test?<o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><a name="_MailEndCompose"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">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.<o:p></o:p></span></a></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">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.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">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.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Thanks,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">--paulr<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> David Blaikie [mailto:dblaikie@gmail.com]
<br>
<b>Sent:</b> Thursday, April 28, 2016 4:26 PM<br>
<b>To:</b> reviews+D19689+public+514682b5314c52ad@reviews.llvm.org; Robinson, Paul<br>
<b>Cc:</b> Aaron Ballman; cfe-commits<br>
<b>Subject:</b> Re: [PATCH] D19689: Add Subjects to NoDebugAttr [NFC]<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">LGTM<o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">On Thu, Apr 28, 2016 at 2:10 PM, Paul Robinson via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<o:p></o:p></p>
<p class="MsoNormal">probinson created this revision.<br>
probinson added a reviewer: aaron.ballman.<br>
probinson added a subscriber: cfe-commits.<br>
<br>
The 'nodebug' attribute had hand-coded constraints; replace those with a Subjects line in Attr.td.<br>
Also add a missing test to verify the attribute is okay on an Objective-C method.<br>
<br>
<a href="http://reviews.llvm.org/D19689" target="_blank">http://reviews.llvm.org/D19689</a><br>
<br>
Files:<br>
include/clang/Basic/Attr.td<br>
include/clang/Basic/DiagnosticSemaKinds.td<br>
lib/Sema/SemaDeclAttr.cpp<br>
test/Sema/attr-nodebug.c<br>
test/SemaObjC/attr-nodebug.m<br>
<br>
Index: test/SemaObjC/attr-nodebug.m<br>
===================================================================<br>
--- test/SemaObjC/attr-nodebug.m<br>
+++ test/SemaObjC/attr-nodebug.m<br>
@@ -0,0 +1,5 @@<br>
+// RUN: %clang_cc1 -fsyntax-only -verify %s<br>
+// expected-no-diagnostics<br>
+@interface NSObject<br>
+- (void)doSomething __attribute__((nodebug));<br>
+@end<o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">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)<br>
<br>
Or is this not actually a new/separate codepath? In which case do we really need the test?<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal" style="margin-bottom:12.0pt">Index: test/Sema/attr-nodebug.c<br>
===================================================================<br>
--- test/Sema/attr-nodebug.c<br>
+++ test/Sema/attr-nodebug.c<br>
@@ -3,7 +3,7 @@<br>
int a __attribute__((nodebug));<br>
<br>
void b() {<br>
- int b __attribute__((nodebug)); // expected-warning {{'nodebug' only applies to variables with static storage duration and functions}}<br>
+ int b __attribute__((nodebug)); // expected-warning {{'nodebug' attribute only applies to functions and global variables}}<br>
}<br>
<br>
void t1() __attribute__((nodebug));<br>
Index: lib/Sema/SemaDeclAttr.cpp<br>
===================================================================<br>
--- lib/Sema/SemaDeclAttr.cpp<br>
+++ lib/Sema/SemaDeclAttr.cpp<br>
@@ -3572,18 +3572,6 @@<br>
}<br>
<br>
static void handleNoDebugAttr(Sema &S, Decl *D, const AttributeList &Attr) {<br>
- if (const VarDecl *VD = dyn_cast<VarDecl>(D)) {<br>
- if (!VD->hasGlobalStorage())<br>
- S.Diag(Attr.getLoc(),<br>
- diag::warn_attribute_requires_functions_or_static_globals)<br>
- << Attr.getName();<br>
- } else if (!isFunctionOrMethod(D)) {<br>
- S.Diag(Attr.getLoc(),<br>
- diag::warn_attribute_requires_functions_or_static_globals)<br>
- << Attr.getName();<br>
- return;<br>
- }<br>
-<br>
D->addAttr(::new (S.Context)<br>
NoDebugAttr(Attr.getRange(), S.Context,<br>
Attr.getAttributeSpellingListIndex()));<br>
Index: include/clang/Basic/DiagnosticSemaKinds.td<br>
===================================================================<br>
--- include/clang/Basic/DiagnosticSemaKinds.td<br>
+++ include/clang/Basic/DiagnosticSemaKinds.td<br>
@@ -2504,9 +2504,6 @@<br>
def warn_incomplete_encoded_type : Warning<<br>
"encoding of %0 type is incomplete because %1 component has unknown encoding">,<br>
InGroup<DiagGroup<"encode-type">>;<br>
-def warn_attribute_requires_functions_or_static_globals : Warning<<br>
- "%0 only applies to variables with static storage duration and functions">,<br>
- InGroup<IgnoredAttributes>;<br>
def warn_gnu_inline_attribute_requires_inline : Warning<<br>
"'gnu_inline' attribute requires function to be marked 'inline',"<br>
" attribute ignored">,<br>
Index: include/clang/Basic/Attr.td<br>
===================================================================<br>
--- include/clang/Basic/Attr.td<br>
+++ include/clang/Basic/Attr.td<br>
@@ -973,6 +973,8 @@<br>
<br>
def NoDebug : InheritableAttr {<br>
let Spellings = [GCC<"nodebug">];<br>
+ let Subjects = SubjectList<[FunctionLike, ObjCMethod, GlobalVar], WarnDiag,<br>
+ "ExpectedFunctionGlobalVarMethodOrProperty">;<br>
let Documentation = [NoDebugDocs];<br>
}<br>
<br>
<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><o:p></o:p></p>
</blockquote>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</div>
</div>
</body>
</html>