Anna,
OK, I'll start a work.
On 25.06.2013 01:35, Anna Zaks wrote:
Alexey,
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.
Sorry for the delay,
Anna.
On Jun 19, 2013, at 9:05 AM, Jordan Rose <[email protected]
<mailto:[email protected]>> wrote:
Hi, Adam. Here're some more comments:
+ typedef std::pair<const ExplodedNode*, const MemRegion*> AllocSite;
+
+ /// \brief This utility function finds the location of the
allocation of
+ /// Sym on the path leading to the exploded node N.
+ template <typename T>
+ AllocSite getAllocationSite(const ExplodedNode *N, SymbolRef Sym)
const {
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.
(Anna: this is why it has to go in a header—it's templated on the GDM
key.)
+ N = N->pred_empty() ? NULL : *(N->pred_begin());
Again, not your fault, but there's a short accessor for this now,
N->getFirstPred(), that includes the empty check.
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.
+//===----------------------------------------------------------------------===//
+/// Defines a checker for the proper use of stream APIs.
+//===----------------------------------------------------------------------===//
This should be marked as \file.
+typedef SmallVector<SymbolRef, 2> SymbolVector;
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.
+class StopTrackingCallback : public SymbolVisitor {
*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.
+ StreamMapTy TrackedStreams = State->get<StreamMap>();
+ StreamMapTy::Factory &F = State->get_context<StreamMap>();
Anna: this is more efficient if there are a lot of things to remove,
because it doesn't canonicalize the state with each removal.
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).
Thanks for working on this!
Jordan
On Jun 17, 2013, at 14:40 , Anna Zaks <[email protected]
<mailto:[email protected]>> wrote:
Adam,
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.
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.
Review for StreamCheckerV2.cpp:
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!
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.
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.
Is there a reason to go through the factory
in StreamCheckerV2::checkDeadSymbols? The original code was simpler.
399 + if (Optional<StmtPoint> SP = P.getAs<StmtPoint>())
400 + AllocationStmt = SP->getStmt();
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.
463 + IIfopen = &Ctx.Idents.get("fopen");
Extra space before '='.
Thanks for working on this!
Anna.
On Apr 28, 2013, at 10:52 PM, Adam Schnitzer <[email protected]
<mailto:[email protected]>> wrote:
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.
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.
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:
int checkLeak(int *Data) {
FILE *F = fopen("myfile.txt", "w");
if (F != 0) {
fputs ("fopen example", F); // expected-warning {{Opened file
is never closed; potential resource leak of 'F'}}
}
if (Data)
return *Data;
else
return 0;
}
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.
Adam
On Mon, Apr 22, 2013 at 3:10 PM, Adam Schnitzer<[email protected]
<mailto:[email protected]>>wrote:
Anna,
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.
Adam
On Mon, Apr 22, 2013 at 1:17 PM, Anna Zaks<[email protected]
<mailto:[email protected]>>wrote:
Adam,
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.
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 "getAllocationSite" from
mallocChecker. Let's see if we can factor it out so that we
do not continue with copying and pasting of that code.
Thanks!
Anna.
On Apr 21, 2013, at 10:56 PM, Adam Schnitzer
<[email protected] <mailto:[email protected]>> wrote:
Anna,
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?
Adam
On Mon, Apr 22, 2013 at 1:20 AM, Anna Zaks<[email protected]
<mailto:[email protected]>>wrote:
Adam,
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.
Thanks,
Anna.
On Apr 20, 2013, at 11:34 PM, Adam Schnitzer
<[email protected] <mailto:[email protected]>> wrote:
> 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.
>
> Any feedback would be great.
>
> Adam
> <SimpleStreamChecker.patch>
<StreamCheckerV2.patch>
--
Best regards,
Alexey Sidorin
Software Engineer,
SWL-CSWG, SMRC, Samsung Electronics
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
