[cfe-dev] Any plans to rework NoStoreFuncVisitor as well?

[Artem Dergachev via cfe-dev]

Yeah, in No${Something}FuncVisitor the definition of ${Something} is 
entirely checker-specific. This means that generality is required not 
only with respect to the text of the note, but also with respect to 
defining the nature of ${Something}.

The flimsiest part of No${Something}FuncVisitor is identifying that the 
function *was supposed to* do ${Something} it didn't do. We typically 
boil this down to "there exists another execution path on which it did 
actually do ${Something}" but we can't check that either because that 
execution path isn't necessarily explored. So we dumb it down even 
further to see if there are syntactic constructs in that function that 
would have accomplished ${Something} (such as assignment to the given 
pointee in case of ${Something}=Store) - which is not only very 
imprecise without flow sensitivity but also fails when 
inter-procedural-ness is involved (i.e., the function was supposed to 
accomplish ${Something} by calling another function that actually does 
${Something}).

I think one TODO here is that we could still take advantage of path 
exploration by having it signal us when it found a delete on a different 
path. It would help us with inter-procedural-ness and possibly with some 
flow-sensitivity as well.

Kristof's case is interesting in a different manner. If taken literally, 
it doesn't satisfy our criteria of "there exists another execution path 
on which it did actually do ${Something}". We probably shouldn't emit a 
note every time the function simply accepts the value of interest by 
pointer, because this doesn't necessarily prove the intent to delete the 
pointer; there are a million other reasons to accept a pointer. However, 
Kristof's case does still deserve a note because sink() is the *only* 
function that had a chance to delete the pointer.

A similar situation can be encountered with uninitialized variables:

void bar() {
}

void foo() {
   int x;
   bar(&x); // Display path in bar() because no other function could 
initialize 'x'
   return x; // Warning: Use of uninitialized variable
}

So I guess the question is:
- We clearly need the text of the note to be configurable by the checker
- We clearly need the definition of ${Something} to be configurable by 
the checker
- But do we need the inner workings of this whole auxiliary analysis to 
be configurable by the checker?

Or can we get away with simply supplying a definition of ${Something} 
as, say, an ASTMatcher with which the auxiliary analysis would scan the 
function? Because, damn, such analysis can get very advanced and 
complicated, so in any case we'd better provide a sane default and not 
duplicate this analysis in every checker.

On 6/30/21 8:05 AM, Kristóf Umann via cfe-dev wrote:
> I think that sounds great, and I have thought about adding this later 
> down the line to your tracking interface. The issue with the specific, 
> and rather poor memory leak report I'm investigating is that its not a 
> (lack of a) value change to a region that I want to track down, but 
> rather a change of some arbitrary property on a symbol. Take for 
> instance the following code snippet:
>
> void sink(int *P) {} // no notes
>
> void f() {
>   sink(new int(5)); // note: Memory is allocated
>                     // Well hold on, sink() was supposed to deal with
>                     //that, this must be a false positive...
> } // warning: Potential memory leak [cplusplus.NewDeleteLeaks]
>
> The fact that the allocated memory leaks is the property of the symbol 
> being dead(SymbolReaper::isDead) despite still being marked as 
> allocated after the call to sink(). The fact that sink() didn't help 
> to prevent this error is a property of:
>
> * No region with a lifetime longer then the call to f holds the value 
> of this symbol
> * The RefState hasn't changed (from Kind::Allocated or 
> Kind::AllocatedOfSizeZero)
>
> ...and to me, it seems like both of these should be checked at 
> bugreporting, not analysis time.
>
> As these properties could only be checked during tracking, I think a 
> handler the way you describe it (as I understand) would be 
> insufficient. While this really is a MallocChecker-specific problem, I 
> think generalizing NoStoreFuncVisitor to have an overridable "did 
> anything change about this entity during this function call?" would be 
> beneficial for many other similar checkers. That might as well end up 
> as an extension to the existing StoreHandler interface.
>
> On Wed, 30 Jun 2021 at 15:17, Valeriy Savchenko <vsavchenko at apple.com 
> <mailto:vsavchenko at apple.com>> wrote:
>
>     Hi Kristóf,
>
>     In my opinion, this is the checker’s logic and it shouldn’t live
>     inside of `NoStoreFuncVisitor`.  However, I think that
>     `NoStateChangeFuncVisitor` can require an extra step for the users.
>
>     During my big discussion with Artem in the patchset, where I
>     introduced the `Tracker` interface, I suggested to extract
>     different bits of what I called “events” from different parts of
>     `trackExpressionValue`, so that checkers can react on those
>     events.  At first, we make it part of the tracker mechanism, so we
>     detect them during this stage, and later we can put special tags
>     on the nodes.
>
>     Long story short, I think that `NoStore` is event and with that it
>     is similar to `Store`.  We can introduce `NoStoreHandler`
>     (probably we can come up with a better name than that), that
>     produces the note, just like `StoreHandler` does.  And the user
>     can plug in their own handler to produce customized note.
>
>     What do you think about this?
>
>     Valeriy
>
>     > On 30 Jun 2021, at 15:57, Kristóf Umann <dkszelethus at gmail.com
>     <mailto:dkszelethus at gmail.com>> wrote:
>     >
>     > Hi!
>     >
>     > Just a quick one -- I want to generalize NoStoreFuncVisitor to
>     be able to construct messages such as "Returning without
>     deallocating or changing the ownership of allocated memory". The
>     grand idea is to create a NoStateChangeFuncVisitor base class that
>     can be specialized for what a (lack of a) state change is.
>     >
>     > Are there any similar ongoing efforts or shall I proceed?
>     >
>     > Cheers,
>     > Kristóf
>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20210630/b68a8184/attachment.html>