[Analyzer] [PATCH] Adding file name to SimpleStreamChecker
Alexey Sidorin
a.sidorin at samsung.com
Tue Jun 25 00:47:10 PDT 2013
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 <jordan_rose at apple.com
> <mailto:jordan_rose at apple.com>> 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 <ganna at apple.com
>> <mailto:ganna at apple.com>> 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 <adamschn at umich.edu
>>> <mailto:adamschn at umich.edu>> 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<adamschn at umich.edu
>>>> <mailto:adamschn at umich.edu>>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<ganna at apple.com
>>>> <mailto:ganna at apple.com>>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
>>>> <adamschn at umich.edu <mailto:adamschn at umich.edu>> 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<ganna at apple.com
>>>>> <mailto:ganna at apple.com>>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
>>>>> <adamschn at umich.edu <mailto:adamschn at umich.edu>> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130625/9d0e99f5/attachment.html>
More information about the cfe-commits
mailing list