<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Apr 28, 2016 at 4:46 PM, Robinson, Paul <span dir="ltr"><<a href="mailto:paul.robinson@sony.com" target="_blank">paul.robinson@sony.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div lang="EN-US" link="blue" vlink="purple">
<div><span class="">
<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?<u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p>
</span><p class="MsoNormal"><a name="m_6347018767109698790__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.</span></a></p></div></div></blockquote><div><br></div><div>Ah, OK - thanks for the context :)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div><p class="MsoNormal"><a name="m_6347018767109698790__MailEndCompose"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u><u></u></span></a></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></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.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></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.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">Thanks,<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d">--paulr<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></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:<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>]
<br>
<b>Sent:</b> Thursday, April 28, 2016 4:26 PM<br>
<b>To:</b> <a href="mailto:reviews%2BD19689%2Bpublic%2B514682b5314c52ad@reviews.llvm.org" target="_blank">reviews+D19689+public+514682b5314c52ad@reviews.llvm.org</a>; Robinson, Paul<br>
<b>Cc:</b> Aaron Ballman; cfe-commits<br>
<b>Subject:</b> Re: [PATCH] D19689: Add Subjects to NoDebugAttr [NFC]<u></u><u></u></span></p>
</div>
</div><div><div class="h5">
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">LGTM<u></u><u></u></p>
<div>
<p class="MsoNormal"><u></u> <u></u></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:<u></u><u></u></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<u></u><u></u></p>
<div>
<p class="MsoNormal"><u></u> <u></u></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?<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"> <u></u><u></u></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" target="_blank">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><u></u><u></u></p>
</blockquote>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
</div>
</div></div></div>
</div>
</div>
</blockquote></div><br></div></div>