NoQ created this revision.
NoQ added reviewers: dcoughlin, aprantl.
Herald added subscribers: cfe-commits, Charusso, dkrupp, donat.nagy, Szelethus,
mikhail.ramalho, a.sidorin, JDevlieghere, szepet, baloghadamsoftware,
kristof.beyls, xazax.hun.
Herald added a project: clang.
NoQ marked 2 inline comments as done.
NoQ added inline comments.
================
Comment at:
clang/test/Analysis/Inputs/expected-plists/nullability-notes.m.plist:225
<array>
- <integer>14</integer>
- <integer>16</integer>
- <integer>17</integer>
+ <integer>26</integer>
+ <integer>30</integer>
----------------
`26` is the new `10`.
================
Comment at: clang/test/Analysis/nullability-notes.m:31
-(void) method {
+ clang_analyzer_warnOnDeadSymbol(self);
// no-crash
----------------
D68108 caused this symbol to never die because in the inlined stack frame of
the accessor it was bound to implicit parameter variable `self` in the Store,
and that variable was put into `UnknownSpaceRegion` due to not being part of
its stack frame's declaration, so it was held alive forever by the Store as if
it's a static variable.
In the affected test D68108 <https://reviews.llvm.org/D68108> causes stubs of
getter and setter methods for property 'x' appear in both `ObjCInterfaceDecl`
and `ObjCImplementationDecl` for our toy `ClassWithProperties`. Previously they
were only present in `ObjCInterfaceDecl`. I guess that's the intended behavior.
`ObjCInterfaceDecl::lookupPrivateMethod()` preferes looking up the method in
the implementation. `CallEvent::getRuntimeDefinition()` trusts it and starts
using the implementation stub instead of the interface stub. This explains the
disappearance of "executed line" 10: implementation stubs, unlike interface
stubs, have invalid source locations.
On the other hand, bodyFarm's `createObjCPropertyGetter()` function queries
`ObjCPropertyDecl` when it is looking for the declaration of variable '`self`'
within the method. This is no longer valid, because the accessor declaration
for `ObjCPropertyDecl` is the interface stub, while the newly farmed body is
going to be attached to the implementation stub. This explains the change in
dead symbol elimination: basically, the 'self' variable is from a different
Decl, so liveness analysis becomes confused. If we are to keep inlining and
farming the implementation stub, we should use the 'self' from the
implementation stub, not the 'self' from the interface stub.
In this patch i basically change `CallEvent` back to use the interface stub.
Generally, i believe that `CallEvent::getRuntimeDefinition()` should always
return either the declaration that has a body (if the body is at all present),
or the canonical declaration (in all other cases), but i didn't verify that. In
any case, body farms always farm the body for the canonical declaration.
I think i made the opposite choice in my previous patch on the subject, D60899
<https://reviews.llvm.org/D60899>. I guess i'll need to make up my mind. I'll
think a bit more about this.
Another useful assertion to add would be to always check that the farmed body
only refers to `ParmVarDecl`s (or `ImplicitParamDecl`s) that belong to the
correct function/method decl.
Repository:
rC Clang
https://reviews.llvm.org/D70158
Files:
clang/lib/Analysis/BodyFarm.cpp
clang/lib/StaticAnalyzer/Core/CallEvent.cpp
clang/test/Analysis/Inputs/expected-plists/nullability-notes.m.plist
clang/test/Analysis/nullability-notes.m
Index: clang/test/Analysis/nullability-notes.m
===================================================================
--- clang/test/Analysis/nullability-notes.m
+++ clang/test/Analysis/nullability-notes.m
@@ -1,6 +1,22 @@
-// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability.NullPassedToNonnull,nullability.NullReturnedFromNonnull,nullability.NullablePassedToNonnull,nullability.NullableReturnedFromNonnull,nullability.NullableDereferenced -analyzer-output=text -verify %s
-// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core,nullability.NullPassedToNonnull,nullability.NullReturnedFromNonnull,nullability.NullablePassedToNonnull,nullability.NullableReturnedFromNonnull,nullability.NullableDereferenced -analyzer-output=plist -o %t.plist %s
-// RUN: %normalize_plist <%t.plist | diff -ub %S/Inputs/expected-plists/nullability-notes.m.plist -
+// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core \
+// RUN: -analyzer-checker=nullability.NullPassedToNonnull \
+// RUN: -analyzer-checker=nullability.NullReturnedFromNonnull \
+// RUN: -analyzer-checker=nullability.NullablePassedToNonnull \
+// RUN: -analyzer-checker=nullability.NullableReturnedFromNonnull \
+// RUN: -analyzer-checker=nullability.NullableDereferenced \
+// RUN: -analyzer-checker=debug.ExprInspection \
+// RUN: -analyzer-output=text -verify %s
+// RUN: %clang_analyze_cc1 -fblocks -analyzer-checker=core \
+// RUN: -analyzer-checker=nullability.NullPassedToNonnull \
+// RUN: -analyzer-checker=nullability.NullReturnedFromNonnull \
+// RUN: -analyzer-checker=nullability.NullablePassedToNonnull \
+// RUN: -analyzer-checker=nullability.NullableReturnedFromNonnull \
+// RUN: -analyzer-checker=nullability.NullableDereferenced \
+// RUN: -analyzer-output=plist -o %t.plist %s
+// RUN: %normalize_plist <%t.plist \
+// RUN: | diff -ub %S/Inputs/expected-plists/nullability-notes.m.plist -
+
+void clang_analyzer_warnOnDeadSymbol(id);
#include "Inputs/system-header-simulator-for-nullability.h"
@@ -12,8 +28,11 @@
@end;
@implementation ClassWithProperties
-(void) method {
+ clang_analyzer_warnOnDeadSymbol(self);
// no-crash
NSObject *x = self.x; // expected-note{{Nullability 'nullable' is inferred}}
+ // expected-warning@-1{{SYMBOL DEAD}}
+ // expected-note@-2 {{SYMBOL DEAD}}
takesNonnull(x); // expected-warning{{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
// expected-note@-1{{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
}
Index: clang/test/Analysis/Inputs/expected-plists/nullability-notes.m.plist
===================================================================
--- clang/test/Analysis/Inputs/expected-plists/nullability-notes.m.plist
+++ clang/test/Analysis/Inputs/expected-plists/nullability-notes.m.plist
@@ -16,12 +16,46 @@
<key>start</key>
<array>
<dict>
- <key>line</key><integer>16</integer>
+ <key>line</key><integer>31</integer>
<key>col</key><integer>3</integer>
<key>file</key><integer>0</integer>
</dict>
<dict>
- <key>line</key><integer>16</integer>
+ <key>line</key><integer>31</integer>
+ <key>col</key><integer>33</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ </array>
+ <key>end</key>
+ <array>
+ <dict>
+ <key>line</key><integer>33</integer>
+ <key>col</key><integer>3</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ <dict>
+ <key>line</key><integer>33</integer>
+ <key>col</key><integer>10</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ </array>
+ </dict>
+ </array>
+ </dict>
+ <dict>
+ <key>kind</key><string>control</string>
+ <key>edges</key>
+ <array>
+ <dict>
+ <key>start</key>
+ <array>
+ <dict>
+ <key>line</key><integer>33</integer>
+ <key>col</key><integer>3</integer>
+ <key>file</key><integer>0</integer>
+ </dict>
+ <dict>
+ <key>line</key><integer>33</integer>
<key>col</key><integer>10</integer>
<key>file</key><integer>0</integer>
</dict>
@@ -29,12 +63,12 @@
<key>end</key>
<array>
<dict>
- <key>line</key><integer>16</integer>
+ <key>line</key><integer>33</integer>
<key>col</key><integer>22</integer>
<key>file</key><integer>0</integer>
</dict>
<dict>
- <key>line</key><integer>16</integer>
+ <key>line</key><integer>33</integer>
<key>col</key><integer>22</integer>
<key>file</key><integer>0</integer>
</dict>
@@ -46,7 +80,7 @@
<key>kind</key><string>event</string>
<key>location</key>
<dict>
- <key>line</key><integer>16</integer>
+ <key>line</key><integer>33</integer>
<key>col</key><integer>22</integer>
<key>file</key><integer>0</integer>
</dict>
@@ -54,12 +88,12 @@
<array>
<array>
<dict>
- <key>line</key><integer>16</integer>
+ <key>line</key><integer>33</integer>
<key>col</key><integer>22</integer>
<key>file</key><integer>0</integer>
</dict>
<dict>
- <key>line</key><integer>16</integer>
+ <key>line</key><integer>33</integer>
<key>col</key><integer>22</integer>
<key>file</key><integer>0</integer>
</dict>
@@ -79,12 +113,12 @@
<key>start</key>
<array>
<dict>
- <key>line</key><integer>16</integer>
+ <key>line</key><integer>33</integer>
<key>col</key><integer>22</integer>
<key>file</key><integer>0</integer>
</dict>
<dict>
- <key>line</key><integer>16</integer>
+ <key>line</key><integer>33</integer>
<key>col</key><integer>22</integer>
<key>file</key><integer>0</integer>
</dict>
@@ -92,12 +126,12 @@
<key>end</key>
<array>
<dict>
- <key>line</key><integer>16</integer>
+ <key>line</key><integer>33</integer>
<key>col</key><integer>3</integer>
<key>file</key><integer>0</integer>
</dict>
<dict>
- <key>line</key><integer>16</integer>
+ <key>line</key><integer>33</integer>
<key>col</key><integer>10</integer>
<key>file</key><integer>0</integer>
</dict>
@@ -113,12 +147,12 @@
<key>start</key>
<array>
<dict>
- <key>line</key><integer>16</integer>
+ <key>line</key><integer>33</integer>
<key>col</key><integer>3</integer>
<key>file</key><integer>0</integer>
</dict>
<dict>
- <key>line</key><integer>16</integer>
+ <key>line</key><integer>33</integer>
<key>col</key><integer>10</integer>
<key>file</key><integer>0</integer>
</dict>
@@ -126,12 +160,12 @@
<key>end</key>
<array>
<dict>
- <key>line</key><integer>17</integer>
+ <key>line</key><integer>36</integer>
<key>col</key><integer>3</integer>
<key>file</key><integer>0</integer>
</dict>
<dict>
- <key>line</key><integer>17</integer>
+ <key>line</key><integer>36</integer>
<key>col</key><integer>14</integer>
<key>file</key><integer>0</integer>
</dict>
@@ -143,7 +177,7 @@
<key>kind</key><string>event</string>
<key>location</key>
<dict>
- <key>line</key><integer>17</integer>
+ <key>line</key><integer>36</integer>
<key>col</key><integer>3</integer>
<key>file</key><integer>0</integer>
</dict>
@@ -151,12 +185,12 @@
<array>
<array>
<dict>
- <key>line</key><integer>17</integer>
+ <key>line</key><integer>36</integer>
<key>col</key><integer>16</integer>
<key>file</key><integer>0</integer>
</dict>
<dict>
- <key>line</key><integer>17</integer>
+ <key>line</key><integer>36</integer>
<key>col</key><integer>16</integer>
<key>file</key><integer>0</integer>
</dict>
@@ -177,10 +211,10 @@
<key>issue_hash_content_of_line_in_context</key><string>ff735bea0eb12d4d172b139143c32365</string>
<key>issue_context_kind</key><string>Objective-C method</string>
<key>issue_context</key><string>method</string>
- <key>issue_hash_function_offset</key><string>3</string>
+ <key>issue_hash_function_offset</key><string>6</string>
<key>location</key>
<dict>
- <key>line</key><integer>17</integer>
+ <key>line</key><integer>36</integer>
<key>col</key><integer>3</integer>
<key>file</key><integer>0</integer>
</dict>
@@ -188,9 +222,11 @@
<dict>
<key>0</key>
<array>
- <integer>14</integer>
- <integer>16</integer>
- <integer>17</integer>
+ <integer>26</integer>
+ <integer>30</integer>
+ <integer>31</integer>
+ <integer>33</integer>
+ <integer>36</integer>
</array>
</dict>
</dict>
Index: clang/lib/StaticAnalyzer/Core/CallEvent.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -1309,6 +1309,8 @@
}
const ObjCMethodDecl *MD = Val.getValue();
+ if (MD && !MD->hasBody())
+ MD = MD->getCanonicalDecl();
if (CanBeSubClassed)
return RuntimeDefinition(MD, Receiver);
else
Index: clang/lib/Analysis/BodyFarm.cpp
===================================================================
--- clang/lib/Analysis/BodyFarm.cpp
+++ clang/lib/Analysis/BodyFarm.cpp
@@ -780,7 +780,11 @@
// return self->_ivar;
ASTMaker M(Ctx);
- const VarDecl *selfVar = Prop->getGetterMethodDecl()->getSelfDecl();
+ const ObjCMethodDecl *MD = Prop->getGetterMethodDecl();
+ assert(MD == MD->getCanonicalDecl() &&
+ "The declaration of 'self' must belong to the method we're farming!");
+
+ const VarDecl *selfVar = MD->getSelfDecl();
if (!selfVar)
return nullptr;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
