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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
