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

[Artem Dergachev via Phabricator via cfe-commits]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D70158.228999.patch
Type: text/x-patch
Size: 10559 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20191113/480aa7ec/attachment-0001.bin>