[PATCH] D81745: [analyzer][MallocChecker] PR46253: Correctly recognize standard realloc

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1071-1072
                                  bool ShouldFreeOnFail) const {
+  // HACK: CallDescription currently recognizes non-standard realloc functions
+  // as standard because it doesn't check the type, or wether its a non-method
+  // function. This should be solved by making CallDescription smarter.
----------------
Indeed. CallDescrtiption could be improved to do precise type checking. Also it could be matching FunctionDecls instead of names of functions (strings), we see how error prone this can be.

I think, the mechanisms in StdCLibraryFunctionChecker could be integrated into CallDescription, so all other checkers could benefit.

See also the discussion we had on this with @xazax.hun :
>>! In D80016#2063978, @martong wrote:
>>>! In D80016#2063234, @xazax.hun wrote:
>> A high level comment. 
>> 
>> Trying to match function signatures is not a unique problem! In fact, almost all of the checks the analyzer have is trying to solve the very some problem.
>> One of the methods we have at this point is called CallDescription, see here: https://github.com/llvm/llvm-project/blob/master/clang/lib/StaticAnalyzer/Core/CallEvent.cpp#L358
>> 
>> Moreover, I would assume something similar needs to be done for APINotes. 
>> 
>> Do you think it would be possible to extend the CallDescription interface to match your needs? In that case all of the checks could profit from this work.
>> What do you think? 
> 
> Yes, viewing from this angle, I am trying to solve a problem here that perhaps could be handled by CallDescription and CallDescriptionMap with some extensions.
> 
> Here are the differences between the two approaches so far:
> - CallDescription is evaluated for every call event (e.g. checkPreCall), the names - strings - are compared; containing declaration contexts are compared by names (see CallEvent::isCalled). Contrary to this, summaries are associated directly to FunctionDecls, so during a call event we compare two FunctionDecl pointers.
> - A CallDescription does not support overloading on different types if the number of parameters are the same. With summaries this works.
> - A CallDescriptionMap is a static map with fixed number of entries. Contrary to this, the FunctionSummaryMap contains an entry (a summary) for a function only if we can lookup a matching FunctionDecl in the TU. The initialization of the FunctionSummaryMap happens lazily during the first call event.
> 
> Except the lack of proper overloading support, these differences are in the implementation. So, yes, giving it more thought, maybe we could refactor CallDescriptionMap to behave this way, but that would be some very heavy refactoring :) Still, would be possible, I guess.
> 
>> Moreover, I would assume something similar needs to be done for APINotes.
> Yes, I agree, I could not find out (yet) how the do it, but somehow they must match a given FunctionDecl to the Name in the YAML.
> 
> -----------------------------
> I was thinking about an alternative method for a long time now.
> Making one step backwards: what we need is to be able to associate some data to a given function. And we can perfectly identify the function with it's prototype written in C/C++.
> So, what if we'd be able to write a CallDescriptionMap (or a FunctionSummaryMap) like this:
> ```
>   CallDescriptionMap<FnCheck> Callbacks = {
>       {{"void *memcpy(void *dest, const void *src, size_t n)"}, &CStringChecker::evalMemcpy},
>       ...
> ```
> Actually, we could reuse the Parser and the Sema itself to solve this issue. In fact, we could achieve this by reusing the ExternalASTSource together with the ASTImporter to find precisely the existing redecl chain in the TU for memcpy. 




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81745/new/

https://reviews.llvm.org/D81745