[PATCH] D77658: [analyzer] StdLibraryFunctionsChecker: Add sanity checks for constraints

[Gabor Marton via Phabricator via cfe-commits]

martong marked 2 inline comments as done.
martong added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:116
+    /// Do sanity check on the constraint.
+    virtual bool validate(const FunctionDecl *) const { return true; }
     ArgNo getArgNo() const { return ArgN; }
----------------
balazske wrote:
> Is it possible to have the validate on the signature (not function decl)? Because the function decl (where the rule is applied) should match the signature already. For example, if the signature contains an irrelevant type it is not good to have a rule that expects a `void*` at that place. After these rules are validated with the signature, they should be applicable on a (any) function that matches the signature.
`void*` would not be a problem. However, there are more intricate pointer types like `FILE *`. To get that type from the `ASTContext` we'd have to do another lookup for "FILE" which would complicate things. So, rather we mark that as `Irrelevant` in the signature. That's what we do in case of `fread` for instance. And after lookup we will have the concrete type in our hands, and that is the moment where we can do all the validation. 

Perhaps we could alternatively have different kind of `Irrelevant` types like `IrrelevantPtr`, etc. But we'd forget to enumerate all of them.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:306
+    // Once we know the exact type of the function then do sanity check on all
+    // the given constraints.
+    bool validate(const FunctionDecl *FD) {
----------------
balazske wrote:
> So this is a check to be used at debug build only to check for programming errors?
> It could be better to separate this task from setting the `FD` itself. Or this function can be called `setFD` and contain a validation if in debug mode (or always). Probably it is better to do the validation always to filter out unexpected found functions (if some unusual API is encountered).
The goal here is to avoid inadvertently adding a summary to a wrong function. This is all because of irrelevant types. The summary as described in the form of a signature and with cases and argument constraints has several presumptions on the types. And the type may be not concrete (irrelevant) so we cannot check the presumptions. We can validate those presumptions once we have the full and complete type.

> It could be better to separate this task from setting the FD itself.
Yes, you are right, I am going to separate that.

> Probably it is better to do the validation always to filter out unexpected found functions
Yes, I agree. I am not sure about the assertions though. Right now the programmer writes the summaries (e.g. me). But once in the future, a library author could write summaries in JSON format. Perhaps a warning diagnostic would be more appropriate that time. Or we could have a checker option that logs out if a function was added to the summary map.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77658