NoQ updated this revision to Diff 229437.
NoQ added a comment.
I discovered another problem related to this patch, which turned out to be a
crash (serious!), so here's an update.
We're crashing on roughly the following idiom:
@interface I : NSObject
@property NSObject *o;
@end
@implementation I
@end
@interface J : I
@end
@implementation J
@synthesize o;
@end
void foo(I *i) {
i.o; // crash
}
In this code we're using `@synthesize` to synthesize a property that was
already declared in our superclass, without re-declaring it in our class.
Here's the respective AST:
ObjCInterfaceDecl 0x115653598 <test.m:3:1, line:5:2> line:3:12 I
|-super ObjCInterface 0x7fa75fb54f78 'NSObject'
|-ObjCImplementation 0x1156538c8 'I'
|-ObjCPropertyDecl 0x1156536c8 <line:4:1, col:21> col:21 o 'NSObject *'
assign readwrite atomic unsafe_unretained
|-ObjCMethodDecl 0x115653748 <col:21> col:21 implicit - o 'NSObject *'
`-ObjCMethodDecl 0x1156537d0 <col:21> col:21 implicit - setO: 'void'
`-ParmVarDecl 0x115653858 <col:21> col:21 o 'NSObject *'
ObjCImplementationDecl 0x1156538c8 <line:7:1, line:8:1> line:7:17 I
|-ObjCInterface 0x115653598 'I'
|-ObjCIvarDecl 0x115653950 <line:4:21> col:21 implicit _o 'NSObject *'
synthesize private
`-ObjCPropertyImplDecl 0x1156539b0 <<invalid sloc>, col:21> <invalid sloc> o
synthesize
|-ObjCProperty 0x1156536c8 'o'
`-ObjCIvar 0x115653950 '_o' 'NSObject *'
ObjCInterfaceDecl 0x115653bc8 <line:10:1, line:11:2> line:10:12 J
|-super ObjCInterface 0x115653598 'I'
`-ObjCImplementation 0x115653ce0 'J'
ObjCImplementationDecl 0x115653ce0 <line:13:1, line:15:1> line:13:17 J
|-ObjCInterface 0x115653bc8 'J'
|-ObjCIvarDecl 0x115653d68 <line:14:13> col:13 o 'NSObject *' synthesize
private
`-ObjCPropertyImplDecl 0x115653dc8 <col:1, col:13> col:13 o synthesize
|-ObjCProperty 0x1156536c8 'o'
`-ObjCIvar 0x115653d68 'o' 'NSObject *'
- In the interface `I` there is an `ObjCPropertyDecl` for `o` and a pair of
accessors. Nothing new here.
- In the implementation `I` there's an `ObjCIvarDecl` for `_o`. Nothing new
here either.
- In the interface `J` there's basically nothing apart from the inheritance
from `I`. In particular, there's no `ObjCPropertyDecl`.
- In the implementation `J` there's:
- An `ObjCPropertyImplDecl` for `o`
- An `ObjCIvarDecl` for `o` which seems to be the new ivar backing the
synthesized property (btw why not `_o`?), and
- A pair of accessor stubs (introduced in D68108
<https://reviews.llvm.org/D68108>).
So, basically, 2 pairs of accessors, 2 ivars, but only one property.
This roughly corresponds to the run-time behavior of this code: you can still
access the old ivar with the ivar syntax from a method in `I`, but you cannot
access the old ivar with property syntax even in a method of `I` - it'll
redirect you to the new ivar instead.
In particular, invoking `ObjCMethodDecl::findPropertyDecl()` on the new stab
causes a crash, because it's //a property accessor that is not backed by a
property decl//.
I changed the code to try to avoid relying on the existence of a property decl
for synthesized accessor stubs, instead attempting a brute-force lookup by name
first.
The question still stands about how exactly do we want the
`ObjCMethodDecl::findPropertyDecl()` method to behave in this scenario (as
opposed to crashing). I could easily modify it to try to find the old property
decl in the superclass, but it isn't particularly helpful for me, because it
wouldn't allow me to find the ivar i'm looking for. I would prefer it to give
me the `ObjCPropertyImplDecl` but it is unfortunately not an
`ObjCPropertyDecl`. It looks like a similar problem has also arised here
<https://reviews.llvm.org/D68108?id=222056#inline-612878>, do we want a more
principled solution?
P.S.
> Moving that code from the static analyzer into Sema may be a good idea though.
Note that the analyzer's body farm doesn't intend to be precise; instead, it's
expected to be a simplified AST that's easy for the static analyzer to
understand. For example, the current body farm ignores //atomicity// because
the static analyzer doesn't care about mutli-threaded environments; even if the
body farm is improved, the analyzer will need to be separately taught to
understand that more complicated AST, but the body farm was our way to avoid
teaching the analyzer about the more complicated ASTs to begin with ("atomics
are just functions, let's farm simpler bodies for them").
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70158/new/
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
clang/test/Analysis/properties.m
Index: clang/test/Analysis/properties.m
===================================================================
--- clang/test/Analysis/properties.m
+++ clang/test/Analysis/properties.m
@@ -3,6 +3,8 @@
void clang_analyzer_eval(int);
+#define nil ((id)0)
+
typedef const void * CFTypeRef;
extern CFTypeRef CFRetain(CFTypeRef cf);
void CFRelease(CFTypeRef cf);
@@ -1040,3 +1042,48 @@
clang_analyzer_eval(self.still_no_custom_accessor == self.still_no_custom_accessor); // expected-warning{{TRUE}}
}
@end
+
+@interface Shadowed
+@property (assign) NSObject *o;
+- (NSObject *)getShadowedIvar;
+- (void)clearShadowedIvar;
+- (NSObject *)getShadowedProp;
+- (void)clearShadowedProp;
+@end
+
+@implementation Shadowed
+- (NSObject *)getShadowedIvar {
+ return self->_o;
+}
+- (void)clearShadowedIvar {
+ self->_o = nil;
+}
+- (NSObject *)getShadowedProp {
+ return self.o;
+}
+- (void)clearShadowedProp {
+ self.o = nil;
+}
+@end
+
+@interface Shadowing : Shadowed
+@end
+
+@implementation Shadowing
+// Property 'o' is declared in the superclass but synthesized here.
+// This creates a separate ivar that shadows the superclass's ivar,
+// but the old ivar is still accessible from the methods of the superclass.
+// The old property, however, is not accessible with the property syntax
+// even from the superclass methods.
+@synthesize o;
+
+-(void)testPropertyShadowing {
+ NSObject *oo = self.o;
+ clang_analyzer_eval(self.o == oo); // expected-warning{{TRUE}}
+ clang_analyzer_eval([self getShadowedIvar] == oo); // expected-warning{{UNKNOWN}}
+ [self clearShadowedIvar];
+ clang_analyzer_eval(self.o == oo); // expected-warning{{TRUE}}
+ clang_analyzer_eval([self getShadowedIvar] == oo); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval([self getShadowedIvar] == nil); // expected-warning{{TRUE}}
+}
+@end
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
@@ -737,50 +737,65 @@
}
static Stmt *createObjCPropertyGetter(ASTContext &Ctx,
- const ObjCPropertyDecl *Prop) {
- // First, find the backing ivar.
- const ObjCIvarDecl *IVar = findBackingIvar(Prop);
- if (!IVar)
- return nullptr;
+ const ObjCMethodDecl *MD) {
+ // First, find the backing ivar.
+ const ObjCIvarDecl *IVar = nullptr;
+
+ // Property accessor stubs sometimes do not correspond to any property.
+ if (MD->isSynthesizedAccessorStub()) {
+ const ObjCInterfaceDecl *IntD = MD->getClassInterface();
+ const ObjCImplementationDecl *ImpD = IntD->getImplementation();
+ for (const auto *V: ImpD->ivars()) {
+ if (V->getName() == MD->getSelector().getNameForSlot(0))
+ IVar = V;
+ }
+ }
- // Ignore weak variables, which have special behavior.
- if (Prop->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_weak)
- return nullptr;
+ if (!IVar) {
+ const ObjCPropertyDecl *Prop = MD->findPropertyDecl();
+ IVar = findBackingIvar(Prop);
+ if (!IVar)
+ return nullptr;
+
+ // Ignore weak variables, which have special behavior.
+ if (Prop->getPropertyAttributes() & ObjCPropertyDecl::OBJC_PR_weak)
+ return nullptr;
- // Look to see if Sema has synthesized a body for us. This happens in
- // Objective-C++ because the return value may be a C++ class type with a
- // non-trivial copy constructor. We can only do this if we can find the
- // @synthesize for this property, though (or if we know it's been auto-
- // synthesized).
- const ObjCImplementationDecl *ImplDecl =
- IVar->getContainingInterface()->getImplementation();
- if (ImplDecl) {
- for (const auto *I : ImplDecl->property_impls()) {
- if (I->getPropertyDecl() != Prop)
- continue;
-
- if (I->getGetterCXXConstructor()) {
- ASTMaker M(Ctx);
- return M.makeReturn(I->getGetterCXXConstructor());
+ // Look to see if Sema has synthesized a body for us. This happens in
+ // Objective-C++ because the return value may be a C++ class type with a
+ // non-trivial copy constructor. We can only do this if we can find the
+ // @synthesize for this property, though (or if we know it's been auto-
+ // synthesized).
+ const ObjCImplementationDecl *ImplDecl =
+ IVar->getContainingInterface()->getImplementation();
+ if (ImplDecl) {
+ for (const auto *I : ImplDecl->property_impls()) {
+ if (I->getPropertyDecl() != Prop)
+ continue;
+
+ if (I->getGetterCXXConstructor()) {
+ ASTMaker M(Ctx);
+ return M.makeReturn(I->getGetterCXXConstructor());
+ }
}
}
- }
- // Sanity check that the property is the same type as the ivar, or a
- // reference to it, and that it is either an object pointer or trivially
- // copyable.
- if (!Ctx.hasSameUnqualifiedType(IVar->getType(),
- Prop->getType().getNonReferenceType()))
- return nullptr;
- if (!IVar->getType()->isObjCLifetimeType() &&
- !IVar->getType().isTriviallyCopyableType(Ctx))
- return nullptr;
+ // Sanity check that the property is the same type as the ivar, or a
+ // reference to it, and that it is either an object pointer or trivially
+ // copyable.
+ if (!Ctx.hasSameUnqualifiedType(IVar->getType(),
+ Prop->getType().getNonReferenceType()))
+ return nullptr;
+ if (!IVar->getType()->isObjCLifetimeType() &&
+ !IVar->getType().isTriviallyCopyableType(Ctx))
+ return nullptr;
+ }
// Generate our body:
// return self->_ivar;
ASTMaker M(Ctx);
- const VarDecl *selfVar = Prop->getGetterMethodDecl()->getSelfDecl();
+ const VarDecl *selfVar = MD->getSelfDecl();
if (!selfVar)
return nullptr;
@@ -791,7 +806,7 @@
selfVar->getType()),
IVar);
- if (!Prop->getType()->isReferenceType())
+ if (!MD->getReturnType()->isReferenceType())
loadedIVar = M.makeLvalueToRvalue(loadedIVar, IVar->getType());
return M.makeReturn(loadedIVar);
@@ -814,10 +829,6 @@
return Val.getValue();
Val = nullptr;
- const ObjCPropertyDecl *Prop = D->findPropertyDecl();
- if (!Prop)
- return nullptr;
-
// For now, we only synthesize getters.
// Synthesizing setters would cause false negatives in the
// RetainCountChecker because the method body would bind the parameter
@@ -840,7 +851,7 @@
return nullptr;
}
- Val = createObjCPropertyGetter(C, Prop);
+ Val = createObjCPropertyGetter(C, D);
return Val.getValue();
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
