[cfe-commits] r150846 - in /cfe/trunk: lib/StaticAnalyzer/Checkers/CStringChecker.cpp lib/StaticAnalyzer/Checkers/InterCheckerAPI.h lib/StaticAnalyzer/Checkers/MallocChecker.cpp test/Analysis/malloc.c

[Anna Zaks]

Jordy, 

I appreciate your input. 

Below are the comments which, hopefully, clarify my thinking.

Anna.

On Feb 25, 2012, at 2:40 AM, Jordy Rose wrote:

>>> Yes, I see that there was a false positive here. But it seems that MallocChecker directly depends on the evaluation of /every/ alias-returning function. The string functions happen to be fairly ubiquitous and present in the same library, so I can see that it might be worth special-casing them, but I personally don't consider understanding strcpy to be an /inherent/ feature of MallocChecker. If the analyzer in general doesn't know about string functions, you shouldn't be surprised when MallocChecker doesn't know about the string functions either.
>> 
>> I don't fully get your point here.. I think implementing domain specific knowledge in the checkers rather then the analyzer core is the right solution. Reasoning about non-malloc related functions makes the Malloc checker more powerful. -> Setting the dependency between the two is the way to solve the problem.
>> 
>> Ideally, CStringBasic should only include the function evaluation logic (and skip the CStringNullArg warning). It's a cleanup, which we should definitely do. But other then that, I do not see issues with this solution.
> 
> My issue (and really, I don't care so strongly in this case; I just want to make it clear) is that let's say someday we model the /entire/ C standard library in the analyzer, and we do it so well that we're basically inlining every call. A lot of those calls do or don't invalidate regions and may or may not return aliases of their arguments. Will MallocChecker then require /every/ C-library-related checker to be turned on when we run it?
> 
> As a concrete example, the pthread_setspecific whitelist entry could be fixed by modelling pthread_getspecific as well. That probably goes into another checker, the ThreadSpecificData checker or something. Does enabling MallocChecker automatically enable the ThreadSpecificData checker, even though a program may never use pthread_*specific or even threads at all?
> 

Whitelisting the 'pthread_getspecific' call is a hack. The right solution would be to create the ThreadSpecificData checker.

> Implementing domain-specific knowledge about the string functions should indeed go in the CStringChecker. Implementing domain-specific knowledge about malloc functions should go in the MallocChecker. I don't consider knowledge about string functions to be part of MallocChecker, and so I don't know if enabling MallocChecker should automatically include the evaluation of string functions.
> 

In general, I think it's worthwhile to separate the idea of domain specific checks from domain specific function evaluation. Currently, the CString checker does both, but it's only the function evaluation part that the Malloc depends on.

Further, many checkers would depend on evaluation of the standard functions (done by other checkers). This is a form of IPA (checkers constructing summaries of functions). So every checker, which wants to take advantage of IPA would want to depend on these evaluations. I think it might make sense to enable those always by default as more checkers rely on the fact that we evaluate standard functions. Currently, there is only one checker that does this, thus the explicit dependency. We are still in the early experimental stage with this. 

To address performance concerns, we can implement optimizations in the analyzer. It is not something which should/needs to be tuned by the user. For example, if we scan the code and detect that 'pthread' APIs are not used, we could just disable the checker responsible for evaluating the APIs. 

> Perhaps the right long-term solution is to make this a "soft" dependency -- if the user explicitly disables the C string checks, MallocChecker won't re-enable them. (A "hard" dependency would print an error message and exit if the checker's not available.) Currently we don't have the infrastructure for that, though, and it's true that the quality of results /is/ better with CStringChecker's participation, so I guess I'll just leave it at this.
> 

Disabling CString API evaluation is not an option even when the user disables CString checks. There are two issues here:
 1) I don't think we should provide an option to disable generation of summaries for CString functions - it's an implementation detail of the analyzer's IPA.
 2) If we happen to run malloc checking on code that does contain calls to strcpy and such, malloc would just produce a ton of false positives. The users of the tool do not have the expertise to make the connection.


> Jordy
>