NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.
Let's land this, I guess? Fantastic work!
================
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:
> > 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()`?
================
Comment at: clang/lib/StaticAnalyzer/Core/DynamicType.cpp:147
+static bool isLive(SymbolReaper &SR, const MemRegion *MR) {
+ return SR.isLiveRegion(MR);
+}
----------------
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)
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
