[Analyzer] [PATCH] Adding file name to SimpleStreamChecker

Anna Zaks ganna at apple.com
Wed Jun 19 09:32:15 PDT 2013


On Jun 19, 2013, at 9:05 AM, Jordan Rose <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.
> 

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 <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> 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> 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> 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> 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> 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> 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>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130619/43c85452/attachment.html>


More information about the cfe-commits mailing list