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