<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Anna,<br>
<br>
OK, I'll start a work.<br>
<br>
On 25.06.2013 01:35, Anna Zaks wrote:<br>
</div>
<blockquote
cite="mid:C4FE9633-CF77-4E48-9934-235C9372867B@apple.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
Alexey,
<div><br>
</div>
<div>Adam told me that he might not be able to work on this
immediately. Please, go ahead and start creating StreamChecker
(OpenCloseAPI checker) out of SimpleStream checker if you are
still interested in working on this.</div>
<div><br>
</div>
<div>Sorry for the delay,</div>
<div>Anna.</div>
<div>
<div><br>
</div>
<div><br>
<div>
<div>On Jun 19, 2013, at 9:05 AM, Jordan Rose <<a
moz-do-not-send="true"
href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<div style="letter-spacing: normal; orphans: auto;
text-align: start; text-indent: 0px; text-transform:
none; white-space: normal; widows: auto; word-spacing:
0px; -webkit-text-stroke-width: 0px; word-wrap:
break-word; -webkit-nbsp-mode: space;
-webkit-line-break: after-white-space;">
<div>Hi, Adam. Here're some more comments:</div>
<div><br>
</div>
<div>
<div>+ typedef std::pair<const ExplodedNode*,
const MemRegion*> AllocSite;</div>
<div>+</div>
<div>+ /// \brief This utility function finds the
location of the allocation of</div>
<div>+ /// Sym on the path leading to the exploded
node N.</div>
<div>+ template <typename T></div>
<div>+ AllocSite getAllocationSite(const ExplodedNode
*N, SymbolRef Sym) const {</div>
</div>
<div><br>
</div>
<div>I know this isn't your code, but that return value
is pretty idiosyncratic. I was going to ask you to add
a doc comment, but maybe you could just return a
little struct instead? Then the members are named
nicely.</div>
<div><br>
</div>
<div>(Anna: this is why it has to go in a header—it's
templated on the GDM key.)</div>
<div><br>
</div>
<div><br>
</div>
<div>+ N = N->pred_empty() ? NULL :
*(N->pred_begin());</div>
<div><br>
</div>
<div>Again, not your fault, but there's a short accessor
for this now, N->getFirstPred(), that includes the
empty check.</div>
<div><br>
</div>
<div>This also scares me because it's not robust against
paths that merge before the error. Maybe some day
we'll go back and rewrite this as a visitor, but then
we'd have to let visitors update the BugReport's
description.</div>
<div><br>
</div>
<div><br>
</div>
<div>
<div>+//===----------------------------------------------------------------------===//</div>
<div>+/// Defines a checker for the proper use of
stream APIs.</div>
<div>+//===----------------------------------------------------------------------===//</div>
</div>
<div><br>
</div>
<div>This should be marked as \file.</div>
<div><br>
</div>
<div><br>
</div>
<div>+typedef SmallVector<SymbolRef, 2>
SymbolVector;</div>
<div><br>
</div>
<div>We don't really need this. You only use it twice
(three times including the Decl), and conventionally
we use ArrayRefs or references to SmallVectorImpls to
pass around arrays. You definitely shouldn't be
passing a SmallVector by value.</div>
<div><br>
</div>
<div><br>
</div>
<div>+class StopTrackingCallback : public SymbolVisitor
{</div>
<div><br>
</div>
<div>*sigh* This should be a generic helper at some
point. We've tossed around the idea of having a
special kind of ProgramState data that does this
automatically.</div>
<div><br>
</div>
<div><br>
</div>
<div>
<div>+ StreamMapTy TrackedStreams =
State->get<StreamMap>();</div>
<div>+ StreamMapTy::Factory &F =
State->get_context<StreamMap>();</div>
</div>
<div><br>
</div>
<div>Anna: this is more efficient if there are a lot of
things to remove, because it doesn't canonicalize the
state with each removal.</div>
<div><br>
</div>
<div><br>
</div>
<div>As for the name, I'm not such a fan of
OpenCloseAPIChecker, but everything I'm coming up with
sounds worse. (ResourceManagementChecker,
CleanupChecker.) Not everything in the general checker
is "open" and "close" (locks, security authorization,
etc).</div>
<div><br>
</div>
<div>Thanks for working on this!</div>
<div>Jordan</div>
<div><br>
</div>
<br>
<div>
<div>On Jun 17, 2013, at 14:40 , Anna Zaks <<a
moz-do-not-send="true"
href="mailto:ganna@apple.com">ganna@apple.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<div style="word-wrap: break-word;
-webkit-nbsp-mode: space; -webkit-line-break:
after-white-space;">
<div>Adam, </div>
<div><br>
</div>
<div>Sorry for the delayed review. Please let us
know if you are still interested in working on
this since Alexey was interested in adding some
extra stream checks.</div>
<div><br>
</div>
<div>Moving of getAllocationSite is a good idea.
However, you should change the name to something
more generic (allocation sounds too related to
memory allocations/deallocations). Also, the
function implementation should go into the cpp
file, not into a header. Please, submit this as
a separate patch. </div>
<div><br>
</div>
<div>Review for StreamCheckerV2.cpp:</div>
<div><br>
</div>
<div>We can just delete StreamChecker.cpp. The
content will always stay in the repository, but
we could reuse the name. Another option would be
to name this checker something else, like
OpenCloseAPI checker. The up-site is that we
could reuse the code to implement other
open/close API checks (lock/unlock,...). The
downside is that the placement of non-open close
Stream API checks would be unclear. I personally
prefer the second option - OpenCloseAPI checker.
Other opinions are welcome!</div>
<div><br>
</div>
<div>We should have a first commit for just
copying the SimpleStreamChecker into another
checker and follow up commits for the specific
improvements. LLVM follows iterative development
process, which encourages small incremental
patches. This would also allow others to improve
the checker.</div>
<div><br>
</div>
<div>There are several header files that have been
added. Please, only include the ones that are
needed. For example, I don't think
"InterCheckerAPI.h" is used. I suspect that most
others are not needed either.</div>
<div><br>
</div>
<div>Is there a reason to go through the factory
in StreamCheckerV2::checkDeadSymbols? The
original code was simpler.</div>
<div><br>
</div>
<div>
<div>399 + if (Optional<StmtPoint> SP =
P.getAs<StmtPoint>())</div>
<div>400 + AllocationStmt = SP->getStmt();</div>
</div>
<div>The malloc checker also special cases the
situation where the point is CallExitEnd.
Please, test what happens when the allocation
site happens in a function called from the
function where the issue is reported.</div>
<div><br>
</div>
<div>463 + IIfopen =
&Ctx.Idents.get("fopen");</div>
<div>Extra space before '='.</div>
<div><br>
</div>
<div>Thanks for working on this!</div>
<div>Anna.</div>
<div><br>
</div>
<div>
<div>On Apr 28, 2013, at 10:52 PM, Adam
Schnitzer <<a moz-do-not-send="true"
href="mailto:adamschn@umich.edu">adamschn@umich.edu</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<div style="letter-spacing: normal; orphans:
auto; text-align: start; text-indent: 0px;
text-transform: none; white-space: normal;
widows: auto; word-spacing: 0px;
-webkit-text-stroke-width: 0px;">I had
another shot at it. I made a new checker,
StreamV2, which is based off of
SimpleStream, so that can stay as an
example.
<div><br>
</div>
<div>This patch moved the getFirstAllocation
function from the malloc checker to
CheckerContext so it can be reused. If
there's a better spot for it, let me know.</div>
<div><br>
</div>
<div>I also added the printing for the
MemRegion where the stream was opened.
This did change the location where some of
the bugs were reported. There is one
example that I don't think is quite right:</div>
</div>
</blockquote>
<blockquote type="cite">
<div style="letter-spacing: normal; orphans:
auto; text-align: start; text-indent: 0px;
text-transform: none; white-space: normal;
widows: auto; word-spacing: 0px;
-webkit-text-stroke-width: 0px;">
<div><br>
</div>
<div>
<div>int checkLeak(int *Data) {</div>
<div> FILE *F = fopen("myfile.txt", "w");</div>
<div> if (F != 0) {</div>
<div> fputs ("fopen example", F); //
expected-warning {{Opened file is never
closed; potential resource leak of 'F'}}</div>
<div> }</div>
<div><br>
</div>
<div> if (Data)</div>
<div> return *Data;</div>
<div> else</div>
<div> return 0;</div>
<div>}</div>
<div><br>
</div>
<div>It seems like the bug should be
reported on the line: if (Data). I think
this might be an error with the order in
which I'm removing nodes from the graph
and reporting bugs.</div>
<div><br>
</div>
<div>Adam</div>
<br>
<div class="gmail_quote">On Mon, Apr 22,
2013 at 3:10 PM, Adam Schnitzer<span
class="Apple-converted-space"> </span><span
dir="ltr"><<a
moz-do-not-send="true"
href="mailto:adamschn@umich.edu"
target="_blank">adamschn@umich.edu</a>></span><span
class="Apple-converted-space"> </span>wrote:<br>
<blockquote class="gmail_quote"
style="margin: 0px 0px 0px 0.8ex;
border-left-width: 1px;
border-left-color: rgb(204, 204, 204);
border-left-style: solid;
padding-left: 1ex;">Anna,
<div><br>
</div>
<div>Thanks for the feedback. I think
I have a better idea of how to go
about it now. I'll have another shot
at it.</div>
<span><font color="#888888"><br>
</font></span>
<div><span><font color="#888888">Adam</font></span>
<div><br>
<br>
<div class="gmail_quote">On Mon,
Apr 22, 2013 at 1:17 PM, Anna
Zaks<span
class="Apple-converted-space"> </span><span
dir="ltr"><<a
moz-do-not-send="true"
href="mailto:ganna@apple.com"
target="_blank">ganna@apple.com</a>></span><span
class="Apple-converted-space"> </span>wrote:<br>
<blockquote class="gmail_quote"
style="margin: 0px 0px 0px
0.8ex; border-left-width: 1px;
border-left-color: rgb(204,
204, 204); border-left-style:
solid; padding-left: 1ex;">
<div style="word-wrap:
break-word;">Adam,
<div><br>
</div>
<div>
<div>
<div>The warning looks
good for the listed
test cases (though the
column seems
redundant). However,
it might not be a good
fit when the file name
is not a string
literal. In other
checkers, we often
pretty print the
region instead; for
example, see
reportLeak in the
MallocChecker. </div>
<div><br>
</div>
<div>The second issue is
storing the string in
the state. It should
be possible to get the
file info only at the
point of a leak
report, not when
processing 'fopen'.
Specifically, you
would go up the path
when reporting the
leak and find the
statement that opened
the file. That logic
would be very similar
to "<span
style="color:
rgb(49, 89, 93);
font-family: Menlo;">getAllocationSite"</span> from
mallocChecker. Let's
see if we can factor
it out so that we do
not continue with
copying and pasting of
that code.</div>
<div><br>
</div>
<div>Thanks!</div>
<span><font
color="#888888">Anna.</font></span>
<div>
<div>On Apr 21, 2013,
at 10:56 PM, Adam
Schnitzer <<a
moz-do-not-send="true"
href="mailto:adamschn@umich.edu" target="_blank">adamschn@umich.edu</a>>
wrote:</div>
<br>
<blockquote
type="cite">
<div
style="letter-spacing:
normal;
text-align: start;
text-indent: 0px;
text-transform:
none; white-space:
normal;
word-spacing:
0px;">Anna,
<div><br>
</div>
<div>Got it, sorry
about the mixup.
I will go ahead
and work in
a separate file.
But did it look
like I was on
the right track
for the
diagnostics?
<div><br>
</div>
<div>Adam<br>
<br>
<div
class="gmail_quote">On
Mon, Apr 22,
2013 at 1:20
AM, Anna Zaks<span> </span><span
dir="ltr"><<a
moz-do-not-send="true" href="mailto:ganna@apple.com" target="_blank">ganna@apple.com</a>></span><span> </span>wrote:<br>
<blockquote
class="gmail_quote"
style="margin:
0px 0px 0px
0.8ex;
border-left-width:
1px;
border-left-color:
rgb(204, 204,
204);
border-left-style:
solid;
padding-left:
1ex;">Adam,<br>
<br>
Sorry if I was
not 100%
clear. We'd
like to leave
the
SimpleStreamChecker.cpp
file as is for
reference
purposes. You
can either
create a new
file or
replace
StreamChecker.cpp
with your
checker.<br>
<br>
Thanks,<br>
Anna.<br>
<div>On Apr
20, 2013, at
11:34 PM, Adam
Schnitzer <<a
moz-do-not-send="true" href="mailto:adamschn@umich.edu" target="_blank">adamschn@umich.edu</a>>
wrote:<br>
<br>
> This is
my first patch
for the
SimpleStreamChecker.
It improves
diagnostics by
adding the
file name in
the case of a
resource leak.
I did so by
adding a
std::string to
the
StreamState to
hold the file
name.<br>
><br>
> Any
feedback would
be great.<br>
><br>
> Adam<br>
</div>
>
<SimpleStreamChecker.patch></blockquote>
</div>
</div>
</div>
</div>
</blockquote>
</div>
</div>
<br>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
</div>
<br>
</div>
<span><StreamCheckerV2.patch></span></div>
</blockquote>
</div>
</div>
</blockquote>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
<br>
<pre class="moz-signature" cols="72">--
Best regards,
Alexey Sidorin
Software Engineer,
SWL-CSWG, SMRC, Samsung Electronics
</pre>
</body>
</html>