Re: [PATCH] D19754: Allow 'nodebug' on local variables
This revision was automatically updated to reflect the committed changes.
Closed by commit rL272859: Allow 'nodebug' on local variables. (authored by
probinson).
Changed prior to commit:
http://reviews.llvm.org/D19754?vs=60930&id=60940#toc
Repository:
rL LLVM
http://reviews.llvm.org/D19754
Files:
cfe/trunk/include/clang/Basic/Attr.td
cfe/trunk/include/clang/Basic/AttrDocs.td
cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
cfe/trunk/test/CodeGenCXX/debug-info-nodebug.cpp
cfe/trunk/test/CodeGenObjC/debug-info-nodebug.m
cfe/trunk/test/Sema/attr-nodebug.c
Index: cfe/trunk/include/clang/Basic/AttrDocs.td
===
--- cfe/trunk/include/clang/Basic/AttrDocs.td
+++ cfe/trunk/include/clang/Basic/AttrDocs.td
@@ -524,8 +524,8 @@
let Category = DocCatVariable;
let Content = [{
The ``nodebug`` attribute allows you to suppress debugging information for a
-function, or for a variable declared with static storage duration, such as
-globals, class static data members, and static locals.
+function or method, or for a variable that is not a parameter or a non-static
+data member.
}];
}
Index: cfe/trunk/include/clang/Basic/Attr.td
===
--- cfe/trunk/include/clang/Basic/Attr.td
+++ cfe/trunk/include/clang/Basic/Attr.td
@@ -82,6 +82,8 @@
S->getKind() != Decl::ImplicitParam &&
S->getKind() != Decl::ParmVar &&
S->getKind() != Decl::NonTypeTemplateParm}]>;
+def NonParmVar : SubsetSubjectgetKind() != Decl::ParmVar}]>;
def NonBitField : SubsetSubjectisBitField()}]>;
@@ -994,8 +996,8 @@
def NoDebug : InheritableAttr {
let Spellings = [GCC<"nodebug">];
- let Subjects = SubjectList<[FunctionLike, ObjCMethod, GlobalVar], WarnDiag,
- "ExpectedFunctionGlobalVarMethodOrProperty">;
+ let Subjects = SubjectList<[FunctionLike, ObjCMethod, NonParmVar], WarnDiag,
+ "ExpectedVariableOrFunction">;
let Documentation = [NoDebugDocs];
}
Index: cfe/trunk/test/Sema/attr-nodebug.c
===
--- cfe/trunk/test/Sema/attr-nodebug.c
+++ cfe/trunk/test/Sema/attr-nodebug.c
@@ -2,8 +2,8 @@
int a __attribute__((nodebug));
-void b() {
- int b __attribute__((nodebug)); // expected-warning {{'nodebug' attribute only applies to functions and global variables}}
+void b(int p __attribute__((nodebug))) { // expected-warning {{'nodebug' attribute only applies to variables and functions}}
+ int b __attribute__((nodebug));
}
void t1() __attribute__((nodebug));
Index: cfe/trunk/test/CodeGenObjC/debug-info-nodebug.m
===
--- cfe/trunk/test/CodeGenObjC/debug-info-nodebug.m
+++ cfe/trunk/test/CodeGenObjC/debug-info-nodebug.m
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple arm-apple-ios -emit-llvm -debug-info-kind=limited -fblocks %s -o - | FileCheck %s
+// Objective-C code cargo-culted from debug-info-lifetime-crash.m.
+@protocol NSObject
+- (id)copy;
+@end
+@class W;
+@interface View1
+@end
+@implementation Controller {
+void (^Block)(void);
+}
+- (void)View:(View1 *)View foo:(W *)W
+{
+ // The reference from inside the block implicitly creates another
+ // local variable for the referenced member. That is what gets
+ // suppressed by the attribute. It still gets debug info as a
+ // member, though.
+ // CHECK-NOT: !DILocalVariable(name: "weakSelf"
+ // CHECK: !DIDerivedType({{.*}} name: "weakSelf"
+ // CHECK-NOT: !DILocalVariable(name: "weakSelf"
+ __attribute__((nodebug)) __typeof(self) weakSelf = self;
+ Block = [^{
+__typeof(self) strongSelf = weakSelf;
+} copy];
+}
+@end
Index: cfe/trunk/test/CodeGenCXX/debug-info-nodebug.cpp
===
--- cfe/trunk/test/CodeGenCXX/debug-info-nodebug.cpp
+++ cfe/trunk/test/CodeGenCXX/debug-info-nodebug.cpp
@@ -44,9 +44,12 @@
// YESINFO-DAG: !DIDerivedType({{.*}} name: "static_const_member"
// NOINFO-NOT: !DIDerivedType({{.*}} name: "static_const_member"
-// Function-local static variable.
+// Function-local static and auto variables.
void func4() {
NODEBUG static int static_local = 6;
+ NODEBUGint normal_local = 7;
}
// YESINFO-DAG: !DIGlobalVariable(name: "static_local"
// NOINFO-NOT: !DIGlobalVariable(name: "static_local"
+// YESINFO-DAG: !DILocalVariable(name: "normal_local"
+// NOINFO-NOT: !DILocalVariable(name: "normal_local"
Index: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
===
--- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
+++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
@@ -3019,6 +3019,8 @@
CGBuilderTy &Builder) {
assert(DebugKind >= codegenoptions::LimitedDebugInfo);
assert(!LexicalBlockStack.em
Re: [PATCH] D19754: Allow 'nodebug' on local variables
dblaikie accepted this revision. dblaikie added a comment. Sure, looks good - thanks. Will discuss the broader test issues later/separately. http://reviews.llvm.org/D19754 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19754: Allow 'nodebug' on local variables
probinson marked an inline comment as done. Comment at: test/CodeGenCXX/debug-info-nodebug.cpp:50 @@ -49,2 +49,3 @@ NODEBUG static int static_local = 6; + NODEBUGint normal_local = 7; } const case removed. http://reviews.llvm.org/D19754 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19754: Allow 'nodebug' on local variables
probinson updated this revision to Diff 60930.
probinson added a comment.
Removed the apparently redundant test for a const local variable.
Yes, I'm back to this after way longer than expected. I believe, for this
patch specifically, the extra const local variable was the only identifiable
problem. There were other issues regarding the overall test, which were partly
discussed here and partly in http://reviews.llvm.org/D19567, but those were not
specific to this patch.
@dblaikie, I propose that with the 'const' test removed, we allow this patch to
proceed (which completes the implementation, something I would dearly like to
have done before 3.9 branches) with the promise that I'll come back around to
the broader topic of the test file in general as a follow-up.
http://reviews.llvm.org/D19754
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
lib/CodeGen/CGDebugInfo.cpp
test/CodeGenCXX/debug-info-nodebug.cpp
test/CodeGenObjC/debug-info-nodebug.m
test/Sema/attr-nodebug.c
Index: test/Sema/attr-nodebug.c
===
--- test/Sema/attr-nodebug.c
+++ test/Sema/attr-nodebug.c
@@ -2,8 +2,8 @@
int a __attribute__((nodebug));
-void b() {
- int b __attribute__((nodebug)); // expected-warning {{'nodebug' attribute only applies to functions and global variables}}
+void b(int p __attribute__((nodebug))) { // expected-warning {{'nodebug' attribute only applies to variables and functions}}
+ int b __attribute__((nodebug));
}
void t1() __attribute__((nodebug));
Index: test/CodeGenObjC/debug-info-nodebug.m
===
--- test/CodeGenObjC/debug-info-nodebug.m
+++ test/CodeGenObjC/debug-info-nodebug.m
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple arm-apple-ios -emit-llvm -debug-info-kind=limited -fblocks %s -o - | FileCheck %s
+// Objective-C code cargo-culted from debug-info-lifetime-crash.m.
+@protocol NSObject
+- (id)copy;
+@end
+@class W;
+@interface View1
+@end
+@implementation Controller {
+void (^Block)(void);
+}
+- (void)View:(View1 *)View foo:(W *)W
+{
+ // The reference from inside the block implicitly creates another
+ // local variable for the referenced member. That is what gets
+ // suppressed by the attribute. It still gets debug info as a
+ // member, though.
+ // CHECK-NOT: !DILocalVariable(name: "weakSelf"
+ // CHECK: !DIDerivedType({{.*}} name: "weakSelf"
+ // CHECK-NOT: !DILocalVariable(name: "weakSelf"
+ __attribute__((nodebug)) __typeof(self) weakSelf = self;
+ Block = [^{
+__typeof(self) strongSelf = weakSelf;
+} copy];
+}
+@end
Index: test/CodeGenCXX/debug-info-nodebug.cpp
===
--- test/CodeGenCXX/debug-info-nodebug.cpp
+++ test/CodeGenCXX/debug-info-nodebug.cpp
@@ -44,9 +44,12 @@
// YESINFO-DAG: !DIDerivedType({{.*}} name: "static_const_member"
// NOINFO-NOT: !DIDerivedType({{.*}} name: "static_const_member"
-// Function-local static variable.
+// Function-local static and auto variables.
void func4() {
NODEBUG static int static_local = 6;
+ NODEBUGint normal_local = 7;
}
// YESINFO-DAG: !DIGlobalVariable(name: "static_local"
// NOINFO-NOT: !DIGlobalVariable(name: "static_local"
+// YESINFO-DAG: !DILocalVariable(name: "normal_local"
+// NOINFO-NOT: !DILocalVariable(name: "normal_local"
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -3019,6 +3019,8 @@
CGBuilderTy &Builder) {
assert(DebugKind >= codegenoptions::LimitedDebugInfo);
assert(!LexicalBlockStack.empty() && "Region stack mismatch, stack empty!");
+ if (VD->hasAttr())
+return;
bool Unwritten =
VD->isImplicit() || (isa(VD->getDeclContext()) &&
@@ -3163,6 +3165,8 @@
if (Builder.GetInsertBlock() == nullptr)
return;
+ if (VD->hasAttr())
+return;
bool isByRef = VD->hasAttr();
Index: include/clang/Basic/AttrDocs.td
===
--- include/clang/Basic/AttrDocs.td
+++ include/clang/Basic/AttrDocs.td
@@ -524,8 +524,8 @@
let Category = DocCatVariable;
let Content = [{
The ``nodebug`` attribute allows you to suppress debugging information for a
-function, or for a variable declared with static storage duration, such as
-globals, class static data members, and static locals.
+function or method, or for a variable that is not a parameter or a non-static
+data member.
}];
}
Index: include/clang/Basic/Attr.td
===
--- include/clang/Basic/Attr.td
+++ include/clang/Basic/Attr.td
@@ -82,6 +82,8 @@
S->getKind() != Decl::ImplicitParam &&
S->getKind() != Decl::ParmVar &&
Re: [PATCH] D19754: Allow 'nodebug' on local variables
On Tue, May 17, 2016 at 2:30 PM, Robinson, Paul wrote: > What you are describing is what testing literature refers to as criteria > for equivalence classes. There is some level of judgment to that, yes. > > > > Yep yep, to be sure. I'm just generally trying to encourage the community > behavior towards being both selective & thorough about testing. > > > > I have noticed you doing this (not just in this review) and I am very > appreciative of the principles; when it comes to understanding what a test > is trying to do, keeping the unnecessary fluff out is very helpful. You > have no idea how many times I've had to suss out the intent of a (usually > comment-free) test after it broke when we merged it into our tree. > Fortunately that sort of thing has been happening less often, now that more > of our changes have been integrated upstream, but still, it's great to have > tests that are very focused…. > > > > ….when they are tests for a bugfix or other comparatively small change. I > have to say when it comes to a new-feature kind of patch, I would rather > have the test err on the side of completeness. > I'm not intending to argue against completeness, to be sure. > This is partly based on the experience of introducing the 'optnone' > attribute to Clang, which IIRC popped up with new and surprising cases two > or three times after its introduction. > For the broader discussion about test strategy, I'd love to hear more about these cases (perhaps on another thread) to make sure I/we/community take the right lessons from them to improve the process and provide review feedback that pushes us in a good direction. > More thorough tests up front could easily have prevented those > surprises. Now here I am again, not with a new attribute but seriously > expanding the applicability of an attribute, and would like to apply > previous experience and start out with what I think should be a moderately > complete test. > > > > If you're unwilling to accept that argument and insist on minimal upstream > tests, okay; I can take what I've done and migrate it into our private > tests, and leave behind only the minimal upstream test. It will leave me > with the test I think the feature needs, leaves upstream with the minimal > test you prefer, and if something breaks it will just take a little longer > to get that feedback. > I think that'd be best, if you've got the option to do so/find it necessary. Thanks! - Dave > Let me know. > > Thanks, > > --paulr > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
RE: [PATCH] D19754: Allow 'nodebug' on local variables
What you are describing is what testing literature refers to as criteria for equivalence classes. There is some level of judgment to that, yes. Yep yep, to be sure. I'm just generally trying to encourage the community behavior towards being both selective & thorough about testing. I have noticed you doing this (not just in this review) and I am very appreciative of the principles; when it comes to understanding what a test is trying to do, keeping the unnecessary fluff out is very helpful. You have no idea how many times I've had to suss out the intent of a (usually comment-free) test after it broke when we merged it into our tree. Fortunately that sort of thing has been happening less often, now that more of our changes have been integrated upstream, but still, it's great to have tests that are very focused…. ….when they are tests for a bugfix or other comparatively small change. I have to say when it comes to a new-feature kind of patch, I would rather have the test err on the side of completeness. This is partly based on the experience of introducing the 'optnone' attribute to Clang, which IIRC popped up with new and surprising cases two or three times after its introduction. More thorough tests up front could easily have prevented those surprises. Now here I am again, not with a new attribute but seriously expanding the applicability of an attribute, and would like to apply previous experience and start out with what I think should be a moderately complete test. If you're unwilling to accept that argument and insist on minimal upstream tests, okay; I can take what I've done and migrate it into our private tests, and leave behind only the minimal upstream test. It will leave me with the test I think the feature needs, leaves upstream with the minimal test you prefer, and if something breaks it will just take a little longer to get that feedback. Let me know. Thanks, --paulr ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19754: Allow 'nodebug' on local variables
On Thu, May 5, 2016 at 8:50 AM, Robinson, Paul
wrote:
> This would be a great conversation to have at the social, sadly I will
> have to miss it this month.
>
Yeah, I don't often make it along to them, unfortunately.
>
> >> dblaikie wrote:
> >>> Doesn't look like the const case is any different from the non-const
> >>> case, is it?
> >> Given a white-box analysis of the compiler internals, you are correct;
> >> this is in contrast to the static const/non-const handling, which *does*
> >> use different paths.
> >> I am unwilling to trust that the const/non-const paths for locals will
> >> forever use the same path. You could also look at it as "the test is
> >> the spec."
> >
> > But even then - what about any other property of a variable? What if it's
> > in a nested scope instead of a top level scope? What if it's declared in
> > a condition (if (int x = ...)). What if it's volatile? We could pick
> > arbitrary properties that /could/ have an effect on debug info but we
> > generally believe to be orthogonal to the feature at hand
>
> What you are describing is what testing literature refers to as criteria
> for equivalence classes. There is some level of judgment to that, yes.
>
Yep yep, to be sure. I'm just generally trying to encourage the community
behavior towards being both selective & thorough about testing.
> > & I'd say 'const' is in the same group of things.
>
> I would have thought exactly that for the static-storage case, but it is
> demonstrably not true.
Could you provide the example you had in mind there? I suspect there's a
bit of a false equivalence there - that, once we understand the language
rules/constructs better, we'll see it doesn't apply here to the local
variable case & wouldn't be used to inform our equivalence classification.
(I'll skip other parts where you refer to that issue in this email until we
discuss it further here)
> Therefore, const/not is a valid distinction for
> the equivalence classes in the static case. Needing a separate const
> test for the static case, it seems completely appropriate to have the
> same for the auto case. In other words, in my judgment the storage-class
> doesn't seem relevant to the equivalence class criteria for the test.
>
> > (a const local variable still needs storage, etc, - the address can be
> > escaped and accessed from elsewhere and determined to be unique
>
> All of those objections apply equally to the static case, and yet the
> static case must have separate tests.
>
> > - and both variables in this example could be optimized away entirely
> > by the compiler because they're unused, so the const is no worse off
> > there in that theoretical concern)
>
> Again not different for the (file-)static case. If a file-static
> variable is not used in the CU there's no reason for it to be emitted.
> As it happens I *did* need uses to get the static cases to work, and
> (currently) don't need uses to get the local cases to work, so in the
> interest of not including elements in the test case that are irrelevant
> to the feature-under-test, I didn't add uses of the locals.
>
But I think that points out that the parallel doesn't apply here - uses are
critical to the static variable case (well, as you saw - uses of the type,
at least, and separately, definitions of the static members are also
necessary to test that codepath too). I don't think there's an equivalence
between this and the local variable case until we see the variable
disappear due to lack of use. Then we'll get into the territory of "what
kind of use is enough" to preserve the variable to demonstrate the debug
info is present/not present - but even then, the const isn't so relevant.
The const would just allow the variable to be optimized away more
frequently & thus not be present to demonstrate the attribute's
functionality.
Sorry, I'm perhaps having a hard time explaining this well.
>
> This is an argument for doing the test exactly as I did: first run it
> to prove debug info IS emitted in the absence of the attribute, then
> again to prove debug info is suppressed in the presence of the attribute.
> That way if optimization or lack-of-use means the variable is not emitted,
> the test can be adjusted to make sure that condition does not apply,
> proving that the lack of debug info is properly because of the attribute
> and not because of some other irrelevant circumstance.
>
Yeah, I think that's a separate and interesting part of testing too -
making the test case resilient to other changes (in this case - the
possibility of frontend optimizations that might remove the variable
entirely before we have a chance to test the attribute).
Another way I would suggest to approach this would be to make it impossible
for the compiler to mess this up - but that also "complicates" the test a
bit too:
void f1(int&);
void f2() {
...attribute... int x;
f1(x);
}
That way the compiler can't optimize away x, has to put it somewhere in
memory, etc.
>
> --paulr
>
>
RE: [PATCH] D19754: Allow 'nodebug' on local variables
This would be a great conversation to have at the social, sadly I will have to miss it this month. >> dblaikie wrote: >>> Doesn't look like the const case is any different from the non-const >>> case, is it? >> Given a white-box analysis of the compiler internals, you are correct; >> this is in contrast to the static const/non-const handling, which *does* >> use different paths. >> I am unwilling to trust that the const/non-const paths for locals will >> forever use the same path. You could also look at it as "the test is >> the spec." > > But even then - what about any other property of a variable? What if it's > in a nested scope instead of a top level scope? What if it's declared in > a condition (if (int x = ...)). What if it's volatile? We could pick > arbitrary properties that /could/ have an effect on debug info but we > generally believe to be orthogonal to the feature at hand What you are describing is what testing literature refers to as criteria for equivalence classes. There is some level of judgment to that, yes. > & I'd say 'const' is in the same group of things. I would have thought exactly that for the static-storage case, but it is demonstrably not true. Therefore, const/not is a valid distinction for the equivalence classes in the static case. Needing a separate const test for the static case, it seems completely appropriate to have the same for the auto case. In other words, in my judgment the storage-class doesn't seem relevant to the equivalence class criteria for the test. > (a const local variable still needs storage, etc, - the address can be > escaped and accessed from elsewhere and determined to be unique All of those objections apply equally to the static case, and yet the static case must have separate tests. > - and both variables in this example could be optimized away entirely > by the compiler because they're unused, so the const is no worse off > there in that theoretical concern) Again not different for the (file-)static case. If a file-static variable is not used in the CU there's no reason for it to be emitted. As it happens I *did* need uses to get the static cases to work, and (currently) don't need uses to get the local cases to work, so in the interest of not including elements in the test case that are irrelevant to the feature-under-test, I didn't add uses of the locals. This is an argument for doing the test exactly as I did: first run it to prove debug info IS emitted in the absence of the attribute, then again to prove debug info is suppressed in the presence of the attribute. That way if optimization or lack-of-use means the variable is not emitted, the test can be adjusted to make sure that condition does not apply, proving that the lack of debug info is properly because of the attribute and not because of some other irrelevant circumstance. --paulr ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19754: Allow 'nodebug' on local variables
On Tue, May 3, 2016 at 4:38 PM, Paul Robinson via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
> probinson marked 2 inline comments as done.
>
>
> Comment at: include/clang/Basic/Attr.td:86-88
> @@ -85,1 +85,5 @@
> +def NonParmVar : SubsetSubject + [{S->getKind() != Decl::ImplicitParam &&
> + S->getKind() != Decl::ParmVar &&
> + S->getKind() !=
> Decl::NonTypeTemplateParm}]>;
> def NonBitField : SubsetSubject
> probinson wrote:
> > aaron.ballman wrote:
> > > Can you add tests for each of these cases to ensure that the
> diagnostic fires on all of them?
> > Actually not sure how to apply an attribute to an ImplicitParam
> Well, this is all very exciting. I tried
> ```
> template<__attribute__((nodebug)) int i> int t() { return i; }
> int g() { return t<2>(); }
> ```
> but got no diagnostic. In fact. putting assert(false) in
> handleNoDebugAttr shows that it's never called. Unsurprisingly, debug info
> for the template parameter still appears.
> Maybe nobody has ever been so foolish as to try to put an attribute on a
> template parameter before? (It is mildly disturbing that putting an
> attribute in an apparently impossible place yields no diagnostic and no
> semantic effect.)
> I was obviously modeling this check on NormalVar, which (it turns out) is
> never used. And if you can't put attributes on template parameters or (it
> would seem likely) implicit parameters, then NonParmVar should check for
> nothing more than ParmVar.
> And that's what I'll do.
>
> (Marking as Done because the set of tests now matches the specified
> condition.)
>
>
> Comment at: test/CodeGenCXX/debug-info-nodebug.cpp:50
> @@ -49,1 +49,3 @@
>NODEBUG static int static_local = 6;
> + NODEBUG const int const_local = 7;
> + NODEBUGint normal_local = 8;
>
> dblaikie wrote:
> > Doesn't look like the const case is any different from the non-const
> case, is it?
> Given a white-box analysis of the compiler internals, you are correct;
> this is in contrast to the static const/non-const handling, which *does*
> use different paths.
> I am unwilling to trust that the const/non-const paths for locals will
> forever use the same path. You could also look at it as "the test is the
> spec."
>
But even then - what about any other property of a variable? What if it's
in a nested scope instead of a top level scope? What if it's declared in a
condition (if (int x = ...)). What if it's volatile? We could pick
arbitrary properties that /could/ have an effect on debug info but we
generally believe to be orthogonal to the feature at hand & I'd say 'const'
is in the same group of things. (a const local variable still needs
storage, etc, - the address can be escaped and accessed from elsewhere and
determined to be unique - and both variables in this example could be
optimized away entirely by the compiler because they're unused, so the
const is no worse off there in that theoretical concern)
>
>
> Comment at: test/CodeGenObjC/debug-info-nodebug.m:17
> @@ +16,3 @@
> + // CHECK-NOT: !DILocalVariable(name: "strongSelf"
> + __attribute__((nodebug)) __typeof(self) weakSelf = self;
> + Block = [^{
>
> dblaikie wrote:
> > Is this case outside of the block interesting in some way? It doesn't
> look like it.
> The attribute on "weakSelf" is what triggers the second modified path in
> CGDebugInfo and suppresses the DILocalVariable for that name.
> The attribute on "strongSelf" goes through the normal EmitDeclare path.
> So in that sense, it is not interesting.
>
> I should not have been so hesitant to work out what was going on here,
> sorry about that. My cluelessness about Objective-C knows no bounds.
> I'm changing the test to verify that the DILocalVariable does not appear,
> while the member info does still appear, and updating the comment to
> reflect this new knowledge.
>
>
> http://reviews.llvm.org/D19754
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19754: Allow 'nodebug' on local variables
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! http://reviews.llvm.org/D19754 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19754: Allow 'nodebug' on local variables
probinson updated this revision to Diff 56079.
probinson marked an inline comment as done.
probinson added a comment.
Correct the attribute condition, and refine the Objective-C test.
http://reviews.llvm.org/D19754
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
lib/CodeGen/CGDebugInfo.cpp
test/CodeGenCXX/debug-info-nodebug.cpp
test/CodeGenObjC/debug-info-nodebug.m
test/Sema/attr-nodebug.c
Index: test/Sema/attr-nodebug.c
===
--- test/Sema/attr-nodebug.c
+++ test/Sema/attr-nodebug.c
@@ -2,8 +2,8 @@
int a __attribute__((nodebug));
-void b() {
- int b __attribute__((nodebug)); // expected-warning {{'nodebug' attribute only applies to functions and global variables}}
+void b(int p __attribute__((nodebug))) { // expected-warning {{'nodebug' attribute only applies to variables and functions}}
+ int b __attribute__((nodebug));
}
void t1() __attribute__((nodebug));
Index: test/CodeGenObjC/debug-info-nodebug.m
===
--- test/CodeGenObjC/debug-info-nodebug.m
+++ test/CodeGenObjC/debug-info-nodebug.m
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -triple arm-apple-ios -emit-llvm -debug-info-kind=limited -fblocks %s -o - | FileCheck %s
+// Objective-C code cargo-culted from debug-info-lifetime-crash.m.
+@protocol NSObject
+- (id)copy;
+@end
+@class W;
+@interface View1
+@end
+@implementation Controller {
+void (^Block)(void);
+}
+- (void)View:(View1 *)View foo:(W *)W
+{
+ // The reference from inside the block implicitly creates another
+ // local variable for the referenced member. That is what gets
+ // suppressed by the attribute. It still gets debug info as a
+ // member, though.
+ // CHECK-NOT: !DILocalVariable(name: "weakSelf"
+ // CHECK: !DIDerivedType({{.*}} name: "weakSelf"
+ // CHECK-NOT: !DILocalVariable(name: "weakSelf"
+ __attribute__((nodebug)) __typeof(self) weakSelf = self;
+ Block = [^{
+__typeof(self) strongSelf = weakSelf;
+} copy];
+}
+@end
Index: test/CodeGenCXX/debug-info-nodebug.cpp
===
--- test/CodeGenCXX/debug-info-nodebug.cpp
+++ test/CodeGenCXX/debug-info-nodebug.cpp
@@ -44,9 +44,15 @@
// YESINFO-DAG: !DIDerivedType({{.*}} name: "static_const_member"
// NOINFO-NOT: !DIDerivedType({{.*}} name: "static_const_member"
-// Function-local static variable.
+// Function-local static, const, and normal variables.
void func4() {
NODEBUG static int static_local = 6;
+ NODEBUG const int const_local = 7;
+ NODEBUGint normal_local = 8;
}
// YESINFO-DAG: !DIGlobalVariable(name: "static_local"
// NOINFO-NOT: !DIGlobalVariable(name: "static_local"
+// YESINFO-DAG: !DILocalVariable(name: "const_local"
+// NOINFO-NOT: !DILocalVariable(name: "const_local"
+// YESINFO-DAG: !DILocalVariable(name: "normal_local"
+// NOINFO-NOT: !DILocalVariable(name: "normal_local"
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -2974,6 +2974,8 @@
CGBuilderTy &Builder) {
assert(DebugKind >= codegenoptions::LimitedDebugInfo);
assert(!LexicalBlockStack.empty() && "Region stack mismatch, stack empty!");
+ if (VD->hasAttr())
+return;
bool Unwritten =
VD->isImplicit() || (isa(VD->getDeclContext()) &&
@@ -3118,6 +3120,8 @@
if (Builder.GetInsertBlock() == nullptr)
return;
+ if (VD->hasAttr())
+return;
bool isByRef = VD->hasAttr();
Index: include/clang/Basic/AttrDocs.td
===
--- include/clang/Basic/AttrDocs.td
+++ include/clang/Basic/AttrDocs.td
@@ -498,8 +498,8 @@
let Category = DocCatVariable;
let Content = [{
The ``nodebug`` attribute allows you to suppress debugging information for a
-function, or for a variable declared with static storage duration, such as
-globals, class static data members, and static locals.
+function or method, or for a variable that is not a parameter or a non-static
+data member.
}];
}
Index: include/clang/Basic/Attr.td
===
--- include/clang/Basic/Attr.td
+++ include/clang/Basic/Attr.td
@@ -82,6 +82,8 @@
S->getKind() != Decl::ImplicitParam &&
S->getKind() != Decl::ParmVar &&
S->getKind() != Decl::NonTypeTemplateParm}]>;
+def NonParmVar : SubsetSubjectgetKind() != Decl::ParmVar}]>;
def NonBitField : SubsetSubjectisBitField()}]>;
@@ -973,8 +975,8 @@
def NoDebug : InheritableAttr {
let Spellings = [GCC<"nodebug">];
- let Subjects = SubjectList<[FunctionLike, ObjCMethod, GlobalVar], WarnDiag,
- "ExpectedFunctionGlobalVarMethodOrProperty">;
+ let Subjects = SubjectList<[Fu
Re: [PATCH] D19754: Allow 'nodebug' on local variables
probinson marked 2 inline comments as done.
Comment at: include/clang/Basic/Attr.td:86-88
@@ -85,1 +85,5 @@
+def NonParmVar : SubsetSubjectgetKind() != Decl::ImplicitParam &&
+ S->getKind() != Decl::ParmVar &&
+ S->getKind() != Decl::NonTypeTemplateParm}]>;
def NonBitField : SubsetSubject aaron.ballman wrote:
> > Can you add tests for each of these cases to ensure that the diagnostic
> > fires on all of them?
> Actually not sure how to apply an attribute to an ImplicitParam
Well, this is all very exciting. I tried
```
template<__attribute__((nodebug)) int i> int t() { return i; }
int g() { return t<2>(); }
```
but got no diagnostic. In fact. putting assert(false) in handleNoDebugAttr
shows that it's never called. Unsurprisingly, debug info for the template
parameter still appears.
Maybe nobody has ever been so foolish as to try to put an attribute on a
template parameter before? (It is mildly disturbing that putting an attribute
in an apparently impossible place yields no diagnostic and no semantic effect.)
I was obviously modeling this check on NormalVar, which (it turns out) is never
used. And if you can't put attributes on template parameters or (it would seem
likely) implicit parameters, then NonParmVar should check for nothing more than
ParmVar.
And that's what I'll do.
(Marking as Done because the set of tests now matches the specified condition.)
Comment at: test/CodeGenCXX/debug-info-nodebug.cpp:50
@@ -49,1 +49,3 @@
NODEBUG static int static_local = 6;
+ NODEBUG const int const_local = 7;
+ NODEBUGint normal_local = 8;
dblaikie wrote:
> Doesn't look like the const case is any different from the non-const case, is
> it?
Given a white-box analysis of the compiler internals, you are correct; this is
in contrast to the static const/non-const handling, which *does* use different
paths.
I am unwilling to trust that the const/non-const paths for locals will forever
use the same path. You could also look at it as "the test is the spec."
Comment at: test/CodeGenObjC/debug-info-nodebug.m:17
@@ +16,3 @@
+ // CHECK-NOT: !DILocalVariable(name: "strongSelf"
+ __attribute__((nodebug)) __typeof(self) weakSelf = self;
+ Block = [^{
dblaikie wrote:
> Is this case outside of the block interesting in some way? It doesn't look
> like it.
The attribute on "weakSelf" is what triggers the second modified path in
CGDebugInfo and suppresses the DILocalVariable for that name.
The attribute on "strongSelf" goes through the normal EmitDeclare path. So in
that sense, it is not interesting.
I should not have been so hesitant to work out what was going on here, sorry
about that. My cluelessness about Objective-C knows no bounds.
I'm changing the test to verify that the DILocalVariable does not appear, while
the member info does still appear, and updating the comment to reflect this new
knowledge.
http://reviews.llvm.org/D19754
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19754: Allow 'nodebug' on local variables
dblaikie added inline comments.
Comment at: test/CodeGenCXX/debug-info-nodebug.cpp:50
@@ -49,1 +49,3 @@
NODEBUG static int static_local = 6;
+ NODEBUG const int const_local = 7;
+ NODEBUGint normal_local = 8;
Doesn't look like the const case is any different from the non-const case, is
it?
Comment at: test/CodeGenObjC/debug-info-nodebug.m:17
@@ +16,3 @@
+ // CHECK-NOT: !DILocalVariable(name: "strongSelf"
+ __attribute__((nodebug)) __typeof(self) weakSelf = self;
+ Block = [^{
Is this case outside of the block interesting in some way? It doesn't look like
it.
http://reviews.llvm.org/D19754
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19754: Allow 'nodebug' on local variables
probinson added inline comments. Comment at: include/clang/Basic/Attr.td:86-88 @@ -85,1 +85,5 @@ +def NonParmVar : SubsetSubjectgetKind() != Decl::ImplicitParam && + S->getKind() != Decl::ParmVar && + S->getKind() != Decl::NonTypeTemplateParm}]>; def NonBitField : SubsetSubject Can you add tests for each of these cases to ensure that the diagnostic fires > on all of them? Actually not sure how to apply an attribute to an ImplicitParam http://reviews.llvm.org/D19754 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19754: Allow 'nodebug' on local variables
aaron.ballman added inline comments. Comment at: include/clang/Basic/Attr.td:86-88 @@ -85,1 +85,5 @@ +def NonParmVar : SubsetSubjectgetKind() != Decl::ImplicitParam && + S->getKind() != Decl::ParmVar && + S->getKind() != Decl::NonTypeTemplateParm}]>; def NonBitField : SubsetSubjecthttp://reviews.llvm.org/D19754 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
