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

[Kristóf Umann via cfe-dev]

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>
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> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20210630/31874a95/attachment-0001.html>