<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
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}.<br>
<br>
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}).<br>
<br>
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.<br>
<br>
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.<br>
<br>
A similar situation can be encountered with uninitialized variables:<br>
<br>
void bar() {<br>
}<br>
<br>
void foo() {<br>
int x;<br>
bar(&x); // Display path in bar() because no other function
could initialize 'x'<br>
return x; // Warning: Use of uninitialized variable<br>
}<br>
<br>
So I guess the question is:<br>
- We clearly need the text of the note to be configurable by the
checker<br>
- We clearly need the definition of ${Something} to be configurable
by the checker<br>
- But do we need the inner workings of this whole auxiliary analysis
to be configurable by the checker?<br>
<br>
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.<br>
<br>
<div class="moz-cite-prefix">On 6/30/21 8:05 AM, Kristóf Umann via
cfe-dev wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAGcXOD6fe6tijF_pa=ALVNGdorp3RdWSqw9S7-xOYcQOOnqmTw@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr">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:
<div><br>
</div>
<div><font face="monospace">void sink(int *P) {} // no notes<br>
<br>
void f() {<br>
sink(new int(5)); // note: Memory is allocated</font></div>
<div><font face="monospace"> // Well hold
on, sink() was supposed to deal with</font></div>
<div><font face="monospace"> //</font><span
style="font-family:monospace"> </span><span
style="font-family:monospace">that,</span><span
style="font-family:monospace"> this must be a false
positive...</span></div>
<div><font face="monospace">} // warning: Potential memory
leak [cplusplus.NewDeleteLeaks]</font><br>
</div>
<div><font face="monospace"><br>
</font></div>
<div><font face="arial, sans-serif">The fact that the
allocated memory leaks is the property of the symbol
being dead</font><span
style="font-family:arial,sans-serif"> </span><span
style="font-family:arial,sans-serif">(SymbolReaper::isDead)</span><span
style="font-family:arial,sans-serif"> 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:</span></div>
<div><font face="arial, sans-serif"><br>
* No region with a lifetime longer then the call to f
holds the value of this symbol</font></div>
<div><font face="arial, sans-serif">* The RefState hasn't
changed (from Kind::Allocated or Kind::</font>AllocatedOfSizeZero<span
style="font-family:arial,sans-serif">)</span></div>
<div><span style="font-family:arial,sans-serif"><br>
</span></div>
<div><font face="arial, sans-serif">...and to me, it seems
like both of these should be checked at bugreporting,
not analysis time.</font></div>
<div><span style="font-family:arial,sans-serif"><br>
</span></div>
<div><font face="arial, sans-serif">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.</font></div>
</div>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Wed, 30 Jun 2021 at 15:17,
Valeriy Savchenko <<a href="mailto:vsavchenko@apple.com"
moz-do-not-send="true">vsavchenko@apple.com</a>> wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi
Kristóf,<br>
<br>
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.<br>
<br>
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.<br>
<br>
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.<br>
<br>
What do you think about this?<br>
<br>
Valeriy<br>
<br>
> On 30 Jun 2021, at 15:57, Kristóf Umann <<a
href="mailto:dkszelethus@gmail.com" target="_blank"
moz-do-not-send="true">dkszelethus@gmail.com</a>> wrote:<br>
> <br>
> Hi!<br>
> <br>
> 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.<br>
> <br>
> Are there any similar ongoing efforts or shall I proceed?<br>
> <br>
> Cheers,<br>
> Kristóf<br>
<br>
</blockquote>
</div>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<pre class="moz-quote-pre" wrap="">_______________________________________________
cfe-dev mailing list
<a class="moz-txt-link-abbreviated" href="mailto:cfe-dev@lists.llvm.org">cfe-dev@lists.llvm.org</a>
<a class="moz-txt-link-freetext" href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a>
</pre>
</blockquote>
<br>
</body>
</html>