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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D77658#2065174 <https://reviews.llvm.org/D77658#2065174>, @MaskRay wrote:

> `arc` adds many unneeded tags from Phabricator. You can drop `Reviewers:` `Subscribers:` `Tags:` and the text `Summary:` with the following script:
>
>   arcfilter () {
>     arc amend
>     git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
>   }
>   
>
> `Reviewed By: ` is considered important by some people (https://lists.llvm.org/pipermail/llvm-dev/2020-January/137889.html). You should keep the tag. (I started to use `--date=now` because some people find author date != committer date annoying. The committer date is usually what people care.))


Thanks for pointing this out and for the script! I'll try to omit unnecessary tags from arc in the future.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:697
         if (auto *FD = dyn_cast<FunctionDecl>(D)) {
-          if (S.matchesSignature(FD)) {
+          if (S.Sign.matches(FD) && S.validate(FD)) {
             auto Res = Map.insert({FD->getCanonicalDecl(), S});
----------------
balazske wrote:
> martong wrote:
> > balazske wrote:
> > > martong wrote:
> > > > Szelethus wrote:
> > > > > This looks a bit odd, we're checking whether the function matches, and than we validate right after? Shouldn't we just not match the `FD` if it isn't valid?
> > > > Yeah, ok, I moved the validation back into `matchesSignature`.
> > > I think these are not related, there should be signature match and validness check, but it is natural to check first the signature, then the validness.
> > @Szelethus, @balazske : I agree with both of you so I renamed `matchesSignature` to `matches`, which is a shorthand to the the signature match and then the validity check (and added a comment):
> > ```
> >     // Returns true if the summary should be applied to the given function.
> >     bool matches(const FunctionDecl *FD) const {
> >       return Sign.matches(FD) && validateByConstraints(FD);
> >     }
> > ```
> Suggestion: Maybe we can have one function for `matches` and `setFunctionDecl` (set it if matches). We do not want to change the function decl later anyway, and not to something that does not match.
Alright, I renamed `matches` to `matchesAndSet` and we set the FD in this function now. Also `setFunctionDecl` is removed.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:68
   /// We avoid nesting types here because each additional qualifier
   /// would need to be repeated in every function spec.
+  class Summary;
----------------
balazske wrote:
> This text above is not the documentation of `Summary` (is it attached to `Summary` by doxygen?). Probably not `///` is needed, only `//`. And it is probably out of date now, at least I can not understand immediately what is it about (what typedef's are these, what kind of "nesting types"). 
Ok, I removed this outdated comment.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:116
+
+    /// Do sanity check on the constraint.
+    bool checkValidity(const FunctionDecl *FD) const {
----------------
balazske wrote:
> We check here that a function is a suitable candidate to be used with the constraint? (We select a function for the constraint, not a constraint for a function.) Maybe a better description would help to clarify this.
We check here, whether the constraint is malformed or not. It is malformed if the specified argument has a mismatch with the given FunctionDecl (e.g. the arg number is out-of-range of the function's arguments list).

Added a comment in the code about that.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:571
   // Apply case/branch specifications.
-  for (const auto &VRS : Summary.CaseConstraints) {
+  for (const auto &VRS : Summary.getCaseConstraints()) {
     ProgramStateRef NewState = State;
----------------
balazske wrote:
> It may be better to see type of `VRS` instead of `auto` (and know what the `VRS` abbrevation means, why not `CC` for case constraint and not `VC` for `ValueConstraint`). Same for `VR` below.
Indeed, I agree. Removed `auto` and refreshed the outdated variable names.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77658