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

Thu Nov 14 19:13:18 PST 2019

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D70158.229437.patch
Type: text/x-patch
Size: 16592 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20191115/4a6a450c/attachment-0001.bin>