On Jun 19, 2013, at 9:05 AM, Jordan Rose <[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.
>
The chance of removing more than one tracked symbol here are very low and this
does make code less readable. If we think it's a better practice to remove a
bunch of symbols and canonize afterwards in all the checkers, we could provide
the API for it.
>
> 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]> 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]> 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]> 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]> 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]> 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]> 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]> 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>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
