[analyzer] Fix Objective-C accessor body farms after D68108.

[NoQ]

Summary

In the affected test 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. 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 ParmVarDecls (or ImplicitParamDecls) that belong to the correct function/method decl.

[NoQ]

clang/test/Analysis/Inputs/expected-plists/nullability-notes.m.plist

225

26 is the new 10.

clang/test/Analysis/nullability-notes.m

31

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.

[aprantl]

In the affected test 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.

The "stub" is really only a forward declaration marked as isPropertyAccessorStub() and it's injected into the various implementations to get more precise debug info attribution for it. The forward decl in the interface is still there, thus the method appearing twice now.

[NoQ]

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).

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, 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").

[NoQ]

I'll commit in order to get rid of the crash, but i'm open for discussions :)

clang/lib/Analysis/BodyFarm.cpp

static Stmt *createObjCPropertyGetter(ASTContext &Ctx,
   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 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;
 
   Expr *loadedIVar =
     M.makeObjCIvarRef(
       M.makeLvalueToRvalue(
         M.makeDeclRefExpr(selfVar),
         selfVar->getType()),

clang/lib/StaticAnalyzer/Core/CallEvent.cpp

if (ReceiverT)
                   }
                 }
                 if (!*Val)
                   Val = IDecl->lookupInstanceMethod(Sel);
               }
         }
 
         const ObjCMethodDecl *MD = Val.getValue();
+        if (MD && !MD->hasBody())
+          MD = MD->getCanonicalDecl();
         if (CanBeSubClassed)
           return RuntimeDefinition(MD, Receiver);
         else
           return RuntimeDefinition(MD, nullptr);
       }
   } else {
     // This is a class method.
     // If we have type info for the receiver class, we are calling via

clang/test/Analysis/Inputs/expected-plists/nullability-notes.m.plist

<array>
     <dict>
      <key>kind</key><string>control</string>
      <key>edges</key>
       <array>
        <dict>
         <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>
          </array>
         <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>
          </array>
        </dict>
       </array>
     </dict>
     <dict>
      <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>
      <key>ranges</key>
      <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>
        </array>
      </array>
      <key>depth</key><integer>1</integer>
      <key>extended_message</key>
      <string>Nullability &apos;nullable&apos; is inferred</string>
      <key>message</key>
      <string>Nullability &apos;nullable&apos; is inferred</string>
     </dict>
     <dict>
      <key>kind</key><string>control</string>
      <key>edges</key>
       <array>
        <dict>
         <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>
          </array>
         <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>
          </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>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>
          </array>
         <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>
          </array>
        </dict>
       </array>
     </dict>
     <dict>
      <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>
      <key>ranges</key>
      <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>
        </array>
      </array>
      <key>depth</key><integer>0</integer>
      <key>extended_message</key>
      <string>Nullable pointer is passed to a callee that requires a non-null 1st parameter</string>
      <key>message</key>
      <string>Nullable pointer is passed to a callee that requires a non-null 1st parameter</string>
     </dict>
    </array>
    <key>description</key><string>Nullable pointer is passed to a callee that requires a non-null 1st parameter</string>
    <key>category</key><string>Memory error</string>
    <key>type</key><string>Nullability</string>
    <key>check_name</key><string>nullability.NullabilityBase</string>
    <!-- This hash is experimental and going to change! -->
    <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>
   <key>ExecutedLines</key>
   <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>
  </array>
  <key>files</key>
  <array>
  </array>
 </dict>
 </plist>

clang/test/Analysis/nullability-notes.m

-// 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"
 
 void takesNonnull(NSObject *_Nonnull y);
 
 @interface ClassWithProperties: NSObject
 @property(copy, nullable) NSObject *x; // plist check ensures no control flow piece from here to 'self.x'.
 -(void) method;
 @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}}
 }
 @end