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 <[email protected]> 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 <[email protected]> > To: [email protected] > Subject: [PATCH] CSA's StreamChecker - additional functionality and > fix > Message-ID: <[email protected]> > 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 > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
