[PATCH] D78286: [analyzer] Track runtime types represented by Obj-C Class objects

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:198
+      // 'self' variable of the current class method.
+      if (ReceiverSVal == Message.getSelfSVal()) {
+        // In this case, we should return the type of the enclosing class
----------------
vsavchenko wrote:
> NoQ wrote:
> > vsavchenko wrote:
> > > NoQ wrote:
> > > > NoQ wrote:
> > > > > vsavchenko wrote:
> > > > > > NoQ wrote:
> > > > > > > vsavchenko wrote:
> > > > > > > > NoQ wrote:
> > > > > > > > > I believe this is pretty much always the case. At least whenever `getInstanceReceiver()` is available. Another exception seem to be when `ReceiverSVal` is an `UnknownVal` (in this case `self` is going to be `SymbolRegionValue` because it's never set in the Store), but that's it. I inferred this by looking at `ObjCMethodCall::getInitialStackFrameContents()`.
> > > > > > > > > 
> > > > > > > > > I think we should have used `getSelfSVal()` to begin with.
> > > > > > > > > I believe this is pretty much always the case.
> > > > > > > > 
> > > > > > > > I didn't quite get what you mean by that
> > > > > > > > 
> > > > > > > > 
> > > > > > > What i'm trying to say is that `C.getSVal(RecE)` and `Message.getSelfSVal()` and `Message.getReceiverSVal()` are basically the same `SVal`. It shouldn't be necessary to check both or check whether they're the same; you must have meant to check for something else, probably something purely syntactic.
> > > > > > > 
> > > > > > > ----
> > > > > > > 
> > > > > > > > I inferred this by looking at ObjCMethodCall::getInitialStackFrameContents().
> > > > > > > 
> > > > > > > Wait, so it's only true for inlined methods. For non-inlined methods `getSelfSVal()` will be unknown :/
> > > > > > Yeah, that might be a bit extraneous to do it with `SVal`s, but this code for sure does its job (it is definitely not a redundant check). `getSelfSVal()` returns receiver of the function //containing// the call and not the call itself. So, it does check if we the receiver of the message is `self`.
> > > > > > 
> > > > > > I changed it to this way of doing things because it is consistent with how the same thing is done in `getRuntimeDefinition`.
> > > > > > `getSelfSVal()` returns receiver of the function containing the call and not the call itself
> > > > > 
> > > > > 😱 looks broken to me.
> > > > Let's rename `getSelfSVal()` so that it screamed that it's the callee's self as opposed to the caller's self, and explain in a long comment why do we even care about the caller's self. I.e., that we heuristically assume that if a class method jumps into another class method on the same class object, it's going to be devirtualized to the same class. Which isn't always the case, hence !Precise.
> > > > 
> > > > 
> > > I don't really think that it's a good idea to mix these two worlds:
> > > 
> > >   - **world one** - interface function that allows to get an `SVal` for `self` of the containing method. It does exactly this, for no specific reason. I'm on board with renaming, but we need to come up with an appropriate name that describes what it gives and not why.
> > >   - **world two** - use-case of this interface method that tries to figure out the type of the receiver (for devirtualization purposes or not).
> > > 
> > > So, the comment can be only here. I agree, I can add more explanation about what we are doing in this particular piece of code, but it won't make any sense to add this (or similar) comment for `getSelfSVal()`.
> > Ideally i'd detach `getSelfSVal()` from `CallEvent` entirely. Like, it doesn't even depend on the call site, why would it be part of `CallEvent` to begin with? This additionally takes care of **world one**.
> > 
> > > so that it screamed that it's the callee's self as opposed to the caller's self
> > 
> > Mmm, the opposite, of course. `getCallerSelfSVal()`?
> > 
> > 
> Where do you think it should be? 
> 
> My vote goes to location context.
As of now `LocationContext` is an entity so low-level that it doesn't even live in `libStaticAnalyzer*`, so it can't be made aware of `SVal`s.

Given that it's basically a Store lookup, i'd recommend putting it into `ProgramState`. I.e., `State->getSelfSVal(LCtx);`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78286/new/

https://reviews.llvm.org/D78286