[PATCH] CSA's StreamChecker - additional functionality and fix

[Anna Zaks]

Aleksei,

Thanks for sending in the patch and adding a lot of improvements to the checker.

Here are some higher level issues I’ve noticed:

 - We strongly prefer not to bifurcate the state whenever possible since it might introduce a lot of performance overhead (you essentially double the work from that point on). The general strategy is to check if the symbol is in valid state before reporting an error. In some cases, you might need to associate another symbol that denotes success or failure of an operation with the primary tracked symbol (in case they are different).  Part of the problem could be caused by StreamChecker performing eval::Call instead of registering for these:
  void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
  void checkPreCall(const CallEvent &Call, CheckerContext &C) const;

 - How do you deal with pointer escape? Looks like StreamChecker does not register for that callback. (See SimpleStreamChecker.cpp for how it should be used.)

 - As I’ve mentioned in one of our previous communications, the improvements to the StreamChecker should be based on SimpleStreamChecker since there are a few subtle issues with the old checker. I would advise to replace the existing StreamChecker with SimpleStreamChecker as the base and add the improvements you’ve added to Stream Checker to it one by one. (Also, this patch is quite large so that would be a way to break it up a bit.)

Sorry it took us a while to start on the review!

Cheers,
Anna.


> On Apr 28, 2014, at 9:35 PM, Aleksei Sidorin <a.sidorin at samsung.com> wrote:
> 
> Hello,
> 
> This is an another version of patch for StreamChecker.
> Improvements:
> 1. open/close Unix API support
> 2. BugVisitor with open/close messages (based on MallocChecker's one)
> And a question: is it OK to use StringSwitch for function names in evalCall or it is too slow?
> 
 
We prefer to use matching on identifiers instead of string compare. You can initialize them once for all functions you are about. You can use SimpleStreamChecker.cpp for an example.

> Message: 1
> Date: Fri, 07 Jun 2013 16:05:19 +0400
> From: Alexey Sidorin <a.sidorin at samsung.com>
> To: cfe-commits at cs.uiuc.edu
> Subject: [PATCH] CSA's StreamChecker - additional functionality and
> 	fix
> Message-ID: <51B1CC7F.4020202 at samsung.com>
> Content-Type: text/plain; charset="utf-8"; Format="flowed"
> 
> Hi,
> 
> I wrote a patch for Clang Static Analyzer's StreamChecker. This patch:
> 1. adds a check for descriptor access (read/write) after it being closed
> 2. adds support of directory operations (opendir/closedir and some other)
> 3. fixes issue in double close check: descriptor that was not tracked 
> before was not marked as closed while calling a close function
> 4. adds fprintf and fscanf functions to track.
> 
> Is it OK to commit or have I done something wrong?
> 
> 
> 
> -- 
> Best regards,
> Aleksei Sidorin
> Software Engineer, 
> IMSWL-IMCG, SRR, Samsung Electronics
> <StreamCheckerv2.patch>_______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140706/0db1988b/attachment.html>