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

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko marked 2 inline comments as done.
vsavchenko added a comment.

> Let's land this, I guess? Fantastic work!

Thanks :-)



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


================
Comment at: clang/lib/StaticAnalyzer/Core/DynamicType.cpp:147
+static bool isLive(SymbolReaper &SR, const MemRegion *MR) {
+  return SR.isLiveRegion(MR);
+}
----------------
NoQ wrote:
> vsavchenko wrote:
> > NoQ wrote:
> > > Feel free to rename `isLiveRegion` instead if it helps :)
> > Unfortunately, it is named a bit differently for a reason, - compiler couldn't decide which function to use `bool isLive(const MemRegion *region)` or `bool isLive(const VarRegion *VR, bool includeStoreBindings = false) const` for a call `isLive(VarRegion *)` (even though it's pretty obvious). Splitting `isLive` for `VarRegion` into two functions doesn't seem like an easy one. The only option I can see is the introduction of `isLiveImpl` (not very pretty in a header file) and two functions `isLive(const VarRegion*) const ` and `isLiveIncludingStoreBindings(const VarRegion *) const`.
> > 
> Or rename `isLive(const VarRegion *, bool includeStoreBindings)`? (I suspect this function should really be private anyway)
I don't know about that, it doesn't seem like a good change. I think that all of them should be `isLive`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78286