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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In 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(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  : Summary.CaseConstraints) {
+  for (const auto  : 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

`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.))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77658



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG16506d789084: [analyzer] StdLibraryFunctionsChecker: Add 
sanity checks for constraints (authored by martong).

Changed prior to commit:
  https://reviews.llvm.org/D77658?vs=267177=267243#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77658

Files:
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -64,10 +64,8 @@
 namespace {
 class StdLibraryFunctionsChecker
 : public Checker {
-  /// Below is a series of typedefs necessary to define function specs.
-  /// We avoid nesting types here because each additional qualifier
-  /// would need to be repeated in every function spec.
-  struct Summary;
+
+  class Summary;
 
   /// Specify how much the analyzer engine should entrust modeling this function
   /// to us. If he doesn't, he performs additional invalidations.
@@ -114,10 +112,27 @@
 virtual ValueConstraintPtr negate() const {
   llvm_unreachable("Not implemented");
 };
+
+// Check 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 argument list).
+bool checkValidity(const FunctionDecl *FD) const {
+  const bool ValidArg = ArgN == Ret || ArgN < FD->getNumParams();
+  assert(ValidArg && "Arg out of range!");
+  if (!ValidArg)
+return false;
+  // Subclasses may further refine the validation.
+  return checkSpecificValidity(FD);
+}
 ArgNo getArgNo() const { return ArgN; }
 
   protected:
 ArgNo ArgN; // Argument to which we apply the constraint.
+
+/// Do polymorphic sanity check on the constraint.
+virtual bool checkSpecificValidity(const FunctionDecl *FD) const {
+  return true;
+}
   };
 
   /// Given a range, should the argument stay inside or outside this range?
@@ -168,6 +183,14 @@
   }
   return std::make_shared(Tmp);
 }
+
+bool checkSpecificValidity(const FunctionDecl *FD) const override {
+  const bool ValidArg =
+  getArgType(FD, ArgN)->isIntegralType(FD->getASTContext());
+  assert(ValidArg &&
+ "This constraint should be applied on an integral type");
+  return ValidArg;
+}
   };
 
   class ComparisonConstraint : public ValueConstraint {
@@ -210,6 +233,13 @@
   Tmp.CannotBeNull = !this->CannotBeNull;
   return std::make_shared(Tmp);
 }
+
+bool checkSpecificValidity(const FunctionDecl *FD) const override {
+  const bool ValidArg = getArgType(FD, ArgN)->isPointerType();
+  assert(ValidArg &&
+ "This constraint should be applied only on a pointer type");
+  return ValidArg;
+}
   };
 
   // Represents a buffer argument with an additional size argument.
@@ -278,11 +308,52 @@
   typedef std::vector ConstraintSet;
 
   using ArgTypes = std::vector;
+
+  // A placeholder type, we use it whenever we do not care about the concrete
+  // type in a Signature.
+  const QualType Irrelevant{};
+  bool static isIrrelevant(QualType T) { return T.isNull(); }
+
+  // The signature of a function we want to describe with a summary. This is a
+  // concessive signature, meaning there may be irrelevant types in the
+  // signature which we do not check against a function with concrete types.
+  struct Signature {
+const ArgTypes ArgTys;
+const QualType RetTy;
+Signature(ArgTypes ArgTys, QualType RetTy) : ArgTys(ArgTys), RetTy(RetTy) {
+  assertRetTypeSuitableForSignature(RetTy);
+  for (size_t I = 0, E = ArgTys.size(); I != E; ++I) {
+QualType ArgTy = ArgTys[I];
+assertArgTypeSuitableForSignature(ArgTy);
+  }
+}
+bool matches(const FunctionDecl *FD) const;
+
+  private:
+static void assertArgTypeSuitableForSignature(QualType T) {
+  assert((T.isNull() || !T->isVoidType()) &&
+ "We should have no void types in the spec");
+  assert((T.isNull() || T.isCanonical()) &&
+ "We should only have canonical types in the spec");
+}
+static void assertRetTypeSuitableForSignature(QualType T) {
+  assert((T.isNull() || T.isCanonical()) &&
+ "We should only have canonical types in the spec");
+}
+  };
+
+  static QualType getArgType(const FunctionDecl *FD, ArgNo ArgN) {
+assert(FD && "Function must be set");
+QualType T = (ArgN == Ret)
+ ? FD->getReturnType().getCanonicalType()
+ : FD->getParamDecl(ArgN)->getType().getCanonicalType();
+return T;
+  }
+
   using Cases = std::vector;

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

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 267177.
martong marked 8 inline comments as done.
martong added a comment.

- Add comments, remove auto from 'for' loops, matches -> matchesAndSet


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77658

Files:
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -63,10 +63,8 @@
 namespace {
 class StdLibraryFunctionsChecker
 : public Checker {
-  /// Below is a series of typedefs necessary to define function specs.
-  /// We avoid nesting types here because each additional qualifier
-  /// would need to be repeated in every function spec.
-  struct Summary;
+
+  class Summary;
 
   /// Specify how much the analyzer engine should entrust modeling this function
   /// to us. If he doesn't, he performs additional invalidations.
@@ -112,10 +110,27 @@
 virtual ValueConstraintPtr negate() const {
   llvm_unreachable("Not implemented");
 };
+
+// Check 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 argument list).
+bool checkValidity(const FunctionDecl *FD) const {
+  const bool ValidArg = ArgN == Ret || ArgN < FD->getNumParams();
+  assert(ValidArg && "Arg out of range!");
+  if (!ValidArg)
+return false;
+  // Subclasses may further refine the validation.
+  return checkSpecificValidity(FD);
+}
 ArgNo getArgNo() const { return ArgN; }
 
   protected:
 ArgNo ArgN; // Argument to which we apply the constraint.
+
+/// Do polymorphic sanity check on the constraint.
+virtual bool checkSpecificValidity(const FunctionDecl *FD) const {
+  return true;
+}
   };
 
   /// Given a range, should the argument stay inside or outside this range?
@@ -165,6 +180,14 @@
   }
   return std::make_shared(Tmp);
 }
+
+bool checkSpecificValidity(const FunctionDecl *FD) const override {
+  const bool ValidArg =
+  getArgType(FD, ArgN)->isIntegralType(FD->getASTContext());
+  assert(ValidArg &&
+ "This constraint should be applied on an integral type");
+  return ValidArg;
+}
   };
 
   class ComparisonConstraint : public ValueConstraint {
@@ -205,17 +228,65 @@
   Tmp.CannotBeNull = !this->CannotBeNull;
   return std::make_shared(Tmp);
 }
+
+bool checkSpecificValidity(const FunctionDecl *FD) const override {
+  const bool ValidArg = getArgType(FD, ArgN)->isPointerType();
+  assert(ValidArg &&
+ "This constraint should be applied only on a pointer type");
+  return ValidArg;
+}
   };
 
   /// The complete list of constraints that defines a single branch.
   typedef std::vector ConstraintSet;
 
   using ArgTypes = std::vector;
+
+  // A placeholder type, we use it whenever we do not care about the concrete
+  // type in a Signature.
+  const QualType Irrelevant{};
+  bool static isIrrelevant(QualType T) { return T.isNull(); }
+
+  // The signature of a function we want to describe with a summary. This is a
+  // concessive signature, meaning there may be irrelevant types in the
+  // signature which we do not check against a function with concrete types.
+  struct Signature {
+const ArgTypes ArgTys;
+const QualType RetTy;
+Signature(ArgTypes ArgTys, QualType RetTy) : ArgTys(ArgTys), RetTy(RetTy) {
+  assertRetTypeSuitableForSignature(RetTy);
+  for (size_t I = 0, E = ArgTys.size(); I != E; ++I) {
+QualType ArgTy = ArgTys[I];
+assertArgTypeSuitableForSignature(ArgTy);
+  }
+}
+bool matches(const FunctionDecl *FD) const;
+
+  private:
+static void assertArgTypeSuitableForSignature(QualType T) {
+  assert((T.isNull() || !T->isVoidType()) &&
+ "We should have no void types in the spec");
+  assert((T.isNull() || T.isCanonical()) &&
+ "We should only have canonical types in the spec");
+}
+static void assertRetTypeSuitableForSignature(QualType T) {
+  assert((T.isNull() || T.isCanonical()) &&
+ "We should only have canonical types in the spec");
+}
+  };
+
+  static QualType getArgType(const FunctionDecl *FD, ArgNo ArgN) {
+assert(FD && "Function must be set");
+QualType T = (ArgN == Ret)
+ ? FD->getReturnType().getCanonicalType()
+ : FD->getParamDecl(ArgN)->getType().getCanonicalType();
+return T;
+  }
+
   using Cases = std::vector;
 
-  /// Includes information about
-  ///   * function prototype (which is necessary to
-  /// ensure we're 

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

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

The code looks basically good to me, some documentations can be improved.




Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:697
 if (auto *FD = dyn_cast(D)) {
-  if (S.matchesSignature(FD)) {
+  if (S.Sign.matches(FD) && S.validate(FD)) {
 auto Res = Map.insert({FD->getCanonicalDecl(), S});

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.



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;

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"). 



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:116
+
+/// Do sanity check on the constraint.
+bool checkValidity(const FunctionDecl *FD) const {

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.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:571
   // Apply case/branch specifications.
-  for (const auto  : Summary.CaseConstraints) {
+  for (const auto  : Summary.getCaseConstraints()) {
 ProgramStateRef NewState = State;

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77658



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 266918.
martong marked 4 inline comments as done.
martong added a comment.

- Rename: validate -> checkValidity, sanityCheck -> checkSpecificValidity
- Rename: Summary::checkValidity -> validateByConstraints
- Add more docs to the Summary class
- Rename matchesSignature -> matches


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77658

Files:
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -66,7 +66,7 @@
   /// Below is a series of typedefs necessary to define function specs.
   /// We avoid nesting types here because each additional qualifier
   /// would need to be repeated in every function spec.
-  struct Summary;
+  class Summary;
 
   /// Specify how much the analyzer engine should entrust modeling this function
   /// to us. If he doesn't, he performs additional invalidations.
@@ -112,10 +112,25 @@
 virtual ValueConstraintPtr negate() const {
   llvm_unreachable("Not implemented");
 };
+
+/// Do sanity check on the constraint.
+bool checkValidity(const FunctionDecl *FD) const {
+  const bool ValidArg = ArgN == Ret || ArgN < FD->getNumParams();
+  assert(ValidArg && "Arg out of range!");
+  if (!ValidArg)
+return false;
+  // Subclasses may further refine the validation.
+  return checkSpecificValidity(FD);
+}
 ArgNo getArgNo() const { return ArgN; }
 
   protected:
 ArgNo ArgN; // Argument to which we apply the constraint.
+
+/// Do polymorphic sanity check on the constraint.
+virtual bool checkSpecificValidity(const FunctionDecl *FD) const {
+  return true;
+}
   };
 
   /// Given a range, should the argument stay inside or outside this range?
@@ -165,6 +180,14 @@
   }
   return std::make_shared(Tmp);
 }
+
+bool checkSpecificValidity(const FunctionDecl *FD) const override {
+  const bool ValidArg =
+  getArgType(FD, ArgN)->isIntegralType(FD->getASTContext());
+  assert(ValidArg &&
+ "This constraint should be applied on an integral type");
+  return ValidArg;
+}
   };
 
   class ComparisonConstraint : public ValueConstraint {
@@ -205,17 +228,65 @@
   Tmp.CannotBeNull = !this->CannotBeNull;
   return std::make_shared(Tmp);
 }
+
+bool checkSpecificValidity(const FunctionDecl *FD) const override {
+  const bool ValidArg = getArgType(FD, ArgN)->isPointerType();
+  assert(ValidArg &&
+ "This constraint should be applied only on a pointer type");
+  return ValidArg;
+}
   };
 
   /// The complete list of constraints that defines a single branch.
   typedef std::vector ConstraintSet;
 
   using ArgTypes = std::vector;
+
+  // A placeholder type, we use it whenever we do not care about the concrete
+  // type in a Signature.
+  const QualType Irrelevant{};
+  bool static isIrrelevant(QualType T) { return T.isNull(); }
+
+  // The signature of a function we want to describe with a summary. This is a
+  // concessive signature, meaning there may be irrelevant types in the
+  // signature which we do not check against a function with concrete types.
+  struct Signature {
+const ArgTypes ArgTys;
+const QualType RetTy;
+Signature(ArgTypes ArgTys, QualType RetTy) : ArgTys(ArgTys), RetTy(RetTy) {
+  assertRetTypeSuitableForSignature(RetTy);
+  for (size_t I = 0, E = ArgTys.size(); I != E; ++I) {
+QualType ArgTy = ArgTys[I];
+assertArgTypeSuitableForSignature(ArgTy);
+  }
+}
+bool matches(const FunctionDecl *FD) const;
+
+  private:
+static void assertArgTypeSuitableForSignature(QualType T) {
+  assert((T.isNull() || !T->isVoidType()) &&
+ "We should have no void types in the spec");
+  assert((T.isNull() || T.isCanonical()) &&
+ "We should only have canonical types in the spec");
+}
+static void assertRetTypeSuitableForSignature(QualType T) {
+  assert((T.isNull() || T.isCanonical()) &&
+ "We should only have canonical types in the spec");
+}
+  };
+
+  static QualType getArgType(const FunctionDecl *FD, ArgNo ArgN) {
+assert(FD && "Function must be set");
+QualType T = (ArgN == Ret)
+ ? FD->getReturnType().getCanonicalType()
+ : FD->getParamDecl(ArgN)->getType().getCanonicalType();
+return T;
+  }
+
   using Cases = std::vector;
 
-  /// Includes information about
-  ///   * function prototype (which is necessary to
-  /// ensure we're modeling the right function and casting values properly),
+  /// A summary includes information about
+  ///   * function 

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

[Gabor Marton via Phabricator via cfe-commits]

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



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:697
 if (auto *FD = dyn_cast(D)) {
-  if (S.matchesSignature(FD)) {
+  if (S.Sign.matches(FD) && S.validate(FD)) {
 auto Res = Map.insert({FD->getCanonicalDecl(), S});

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);
}
```



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:131
+/// Do polymorphic sanity check on the constraint.
+virtual bool sanityCheck(const FunctionDecl *FD) const { return true; }
   };

balazske wrote:
> Function names should be verb phrases. Maybe `checkSanity` is good or 
> `validateInternal`, `validateMore` (this function relates to `validate`, not 
> something different from it as "sanity").
> 
> `validate` can be called `isValid` or `checkValidity`, so we can not think 
> (from the name) that the function changes something on the object. 
> (`checkValidity` is good, then `sanityCheck` can be renamed to 
> `checkSpecificValidity` or like this.)
Thanks for the naming suggestions, they are definitely better than my naming!



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:304
   /// If these constraints are not satisfied that means a fatal error
   /// usually resulting in undefined behaviour.
+  class Summary {

balazske wrote:
> I would extend the documentation with information about that the signature 
> and constraints together contain information about what functions are 
> accepted by the summary. The signature can use "wildcards" (the irrelevant 
> types) and the constraints may specify additional rules for that type (that 
> comes from the kind of constraint).
OK, I added some more docs to the class.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:345
+// the given constraints.
+bool validate(const FunctionDecl *FD) const {
+  for (const auto  : CaseConstraints)

balazske wrote:
> Again `isValid` or `checkValidity` is a better name, or 
> `validateByConstraints`, `matchesConstraints`. Additionally 
> `matchesSignature` checks in its current form not only for signature, so its 
> name is not correct too.
Ok, renamed to `validateByConstraints`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77658



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 266879.
martong marked an inline comment as done.
martong added a comment.

- Rebase to master


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77658

Files:
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -66,7 +66,7 @@
   /// Below is a series of typedefs necessary to define function specs.
   /// We avoid nesting types here because each additional qualifier
   /// would need to be repeated in every function spec.
-  struct Summary;
+  class Summary;
 
   /// Specify how much the analyzer engine should entrust modeling this function
   /// to us. If he doesn't, he performs additional invalidations.
@@ -112,10 +112,23 @@
 virtual ValueConstraintPtr negate() const {
   llvm_unreachable("Not implemented");
 };
+
+/// Do sanity check on the constraint.
+bool validate(const FunctionDecl *FD) const {
+  const bool ValidArg = ArgN == Ret || ArgN < FD->getNumParams();
+  assert(ValidArg && "Arg out of range!");
+  if (!ValidArg)
+return false;
+  // Subclasses may further refine the validation.
+  return sanityCheck(FD);
+}
 ArgNo getArgNo() const { return ArgN; }
 
   protected:
 ArgNo ArgN; // Argument to which we apply the constraint.
+
+/// Do polymorphic sanity check on the constraint.
+virtual bool sanityCheck(const FunctionDecl *FD) const { return true; }
   };
 
   /// Given a range, should the argument stay inside or outside this range?
@@ -165,6 +178,14 @@
   }
   return std::make_shared(Tmp);
 }
+
+bool sanityCheck(const FunctionDecl *FD) const override {
+  const bool ValidArg =
+  getArgType(FD, ArgN)->isIntegralType(FD->getASTContext());
+  assert(ValidArg &&
+ "This constraint should be applied on an integral type");
+  return ValidArg;
+}
   };
 
   class ComparisonConstraint : public ValueConstraint {
@@ -205,12 +226,61 @@
   Tmp.CannotBeNull = !this->CannotBeNull;
   return std::make_shared(Tmp);
 }
+
+bool sanityCheck(const FunctionDecl *FD) const override {
+  const bool ValidArg = getArgType(FD, ArgN)->isPointerType();
+  assert(ValidArg &&
+ "This constraint should be applied only on a pointer type");
+  return ValidArg;
+}
   };
 
   /// The complete list of constraints that defines a single branch.
   typedef std::vector ConstraintSet;
 
   using ArgTypes = std::vector;
+
+  // A placeholder type, we use it whenever we do not care about the concrete
+  // type in a Signature.
+  const QualType Irrelevant{};
+  bool static isIrrelevant(QualType T) { return T.isNull(); }
+
+  // The signature of a function we want to describe with a summary. This is a
+  // concessive signature, meaning there may be irrelevant types in the
+  // signature which we do not check against a function with concrete types.
+  struct Signature {
+const ArgTypes ArgTys;
+const QualType RetTy;
+Signature(ArgTypes ArgTys, QualType RetTy) : ArgTys(ArgTys), RetTy(RetTy) {
+  assertRetTypeSuitableForSignature(RetTy);
+  for (size_t I = 0, E = ArgTys.size(); I != E; ++I) {
+QualType ArgTy = ArgTys[I];
+assertArgTypeSuitableForSignature(ArgTy);
+  }
+}
+bool matches(const FunctionDecl *FD) const;
+
+  private:
+static void assertArgTypeSuitableForSignature(QualType T) {
+  assert((T.isNull() || !T->isVoidType()) &&
+ "We should have no void types in the spec");
+  assert((T.isNull() || T.isCanonical()) &&
+ "We should only have canonical types in the spec");
+}
+static void assertRetTypeSuitableForSignature(QualType T) {
+  assert((T.isNull() || T.isCanonical()) &&
+ "We should only have canonical types in the spec");
+}
+  };
+
+  static QualType getArgType(const FunctionDecl *FD, ArgNo ArgN) {
+assert(FD && "Function must be set");
+QualType T = (ArgN == Ret)
+ ? FD->getReturnType().getCanonicalType()
+ : FD->getParamDecl(ArgN)->getType().getCanonicalType();
+return T;
+  }
+
   using Cases = std::vector;
 
   /// Includes information about
@@ -232,15 +302,19 @@
   ///   * a list of argument constraints, that must be true on every branch.
   /// If these constraints are not satisfied that means a fatal error
   /// usually resulting in undefined behaviour.
-  struct Summary {
-const ArgTypes ArgTys;
-const QualType RetTy;
+  class Summary {
+const Signature Sign;
 const InvalidationKind InvalidationKd;
 Cases CaseConstraints;
 ConstraintSet 

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

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:697
 if (auto *FD = dyn_cast(D)) {
-  if (S.matchesSignature(FD)) {
+  if (S.Sign.matches(FD) && S.validate(FD)) {
 auto Res = Map.insert({FD->getCanonicalDecl(), S});

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.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:131
+/// Do polymorphic sanity check on the constraint.
+virtual bool sanityCheck(const FunctionDecl *FD) const { return true; }
   };

Function names should be verb phrases. Maybe `checkSanity` is good or 
`validateInternal`, `validateMore` (this function relates to `validate`, not 
something different from it as "sanity").

`validate` can be called `isValid` or `checkValidity`, so we can not think 
(from the name) that the function changes something on the object. 
(`checkValidity` is good, then `sanityCheck` can be renamed to 
`checkSpecificValidity` or like this.)



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:304
   /// If these constraints are not satisfied that means a fatal error
   /// usually resulting in undefined behaviour.
+  class Summary {

I would extend the documentation with information about that the signature and 
constraints together contain information about what functions are accepted by 
the summary. The signature can use "wildcards" (the irrelevant types) and the 
constraints may specify additional rules for that type (that comes from the 
kind of constraint).



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:345
+// the given constraints.
+bool validate(const FunctionDecl *FD) const {
+  for (const auto  : CaseConstraints)

Again `isValid` or `checkValidity` is a better name, or 
`validateByConstraints`, `matchesConstraints`. Additionally `matchesSignature` 
checks in its current form not only for signature, so its name is not correct 
too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77658



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 261855.
martong marked 2 inline comments as done.
martong added a comment.

- Better encapsulate the data in a 'Summary'


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77658

Files:
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -66,7 +66,7 @@
   /// Below is a series of typedefs necessary to define function specs.
   /// We avoid nesting types here because each additional qualifier
   /// would need to be repeated in every function spec.
-  struct Summary;
+  class Summary;
 
   /// Specify how much the analyzer engine should entrust modeling this function
   /// to us. If he doesn't, he performs additional invalidations.
@@ -112,10 +112,23 @@
 virtual ValueConstraintPtr negate() const {
   llvm_unreachable("Not implemented");
 };
+
+/// Do sanity check on the constraint.
+bool validate(const FunctionDecl *FD) const {
+  const bool ValidArg = ArgN == Ret || ArgN < FD->getNumParams();
+  assert(ValidArg && "Arg out of range!");
+  if (!ValidArg)
+return false;
+  // Subclasses may further refine the validation.
+  return sanityCheck(FD);
+}
 ArgNo getArgNo() const { return ArgN; }
 
   protected:
 ArgNo ArgN; // Argument to which we apply the constraint.
+
+/// Do polymorphic sanity check on the constraint.
+virtual bool sanityCheck(const FunctionDecl *FD) const { return true; }
   };
 
   /// Given a range, should the argument stay inside or outside this range?
@@ -165,6 +178,14 @@
   }
   return std::make_shared(Tmp);
 }
+
+bool sanityCheck(const FunctionDecl *FD) const override {
+  const bool ValidArg =
+  getArgType(FD, ArgN)->isIntegralType(FD->getASTContext());
+  assert(ValidArg &&
+ "This constraint should be applied on an integral type");
+  return ValidArg;
+}
   };
 
   class ComparisonConstraint : public ValueConstraint {
@@ -205,12 +226,61 @@
   Tmp.CannotBeNull = !this->CannotBeNull;
   return std::make_shared(Tmp);
 }
+
+bool sanityCheck(const FunctionDecl *FD) const override {
+  const bool ValidArg = getArgType(FD, ArgN)->isPointerType();
+  assert(ValidArg &&
+ "This constraint should be applied only on a pointer type");
+  return ValidArg;
+}
   };
 
   /// The complete list of constraints that defines a single branch.
   typedef std::vector ConstraintSet;
 
   using ArgTypes = std::vector;
+
+  // A placeholder type, we use it whenever we do not care about the concrete
+  // type in a Signature.
+  const QualType Irrelevant{};
+  bool static isIrrelevant(QualType T) { return T.isNull(); }
+
+  // The signature of a function we want to describe with a summary. This is a
+  // concessive signature, meaning there may be irrelevant types in the
+  // signature which we do not check against a function with concrete types.
+  struct Signature {
+const ArgTypes ArgTys;
+const QualType RetTy;
+Signature(ArgTypes ArgTys, QualType RetTy) : ArgTys(ArgTys), RetTy(RetTy) {
+  assertRetTypeSuitableForSignature(RetTy);
+  for (size_t I = 0, E = ArgTys.size(); I != E; ++I) {
+QualType ArgTy = ArgTys[I];
+assertArgTypeSuitableForSignature(ArgTy);
+  }
+}
+bool matches(const FunctionDecl *FD) const;
+
+  private:
+static void assertArgTypeSuitableForSignature(QualType T) {
+  assert((T.isNull() || !T->isVoidType()) &&
+ "We should have no void types in the spec");
+  assert((T.isNull() || T.isCanonical()) &&
+ "We should only have canonical types in the spec");
+}
+static void assertRetTypeSuitableForSignature(QualType T) {
+  assert((T.isNull() || T.isCanonical()) &&
+ "We should only have canonical types in the spec");
+}
+  };
+
+  static QualType getArgType(const FunctionDecl *FD, ArgNo ArgN) {
+assert(FD && "Function must be set");
+QualType T = (ArgN == Ret)
+ ? FD->getReturnType().getCanonicalType()
+ : FD->getParamDecl(ArgN)->getType().getCanonicalType();
+return T;
+  }
+
   using Cases = std::vector;
 
   /// Includes information about
@@ -232,15 +302,19 @@
   ///   * a list of argument constraints, that must be true on every branch.
   /// If these constraints are not satisfied that means a fatal error
   /// usually resulting in undefined behaviour.
-  struct Summary {
-const ArgTypes ArgTys;
-const QualType RetTy;
+  class Summary {
+const Signature Sign;
 const InvalidationKind InvalidationKd;
 Cases CaseConstraints;
 

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

[Gabor Marton via Phabricator via cfe-commits]

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



Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:234-236
+  // The signature of a function we want to describe with a summary. This is a
+  // concessive signature, meaning there may be irrelevant types in the
+  // signature which we do not check against a function with concrete types.

Szelethus wrote:
> It might be worth putting a `TODO` here to not forget the constness methods 
> :^)
Two types do not match if one of them is a const and the other is non-const. Is 
this what you're referring for?



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:697
 if (auto *FD = dyn_cast(D)) {
-  if (S.matchesSignature(FD)) {
+  if (S.Sign.matches(FD) && S.validate(FD)) {
 auto Res = Map.insert({FD->getCanonicalDecl(), S});

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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77658



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

The idea is noble with the addition of `validate` functions, assert in debug 
builds and just move on in release. However, I'd expect it to be integrated 
into the signature matching function.




Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:234-236
+  // The signature of a function we want to describe with a summary. This is a
+  // concessive signature, meaning there may be irrelevant types in the
+  // signature which we do not check against a function with concrete types.

It might be worth putting a `TODO` here to not forget the constness methods :^)



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:697
 if (auto *FD = dyn_cast(D)) {
-  if (S.matchesSignature(FD)) {
+  if (S.Sign.matches(FD) && S.validate(FD)) {
 auto Res = Map.insert({FD->getCanonicalDecl(), S});

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77658



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Kéri via Phabricator via cfe-commits]

balazske 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; }

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.



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) {

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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77658



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Gabor Marton via Phabricator via cfe-commits]

martong created this revision.
martong added reviewers: NoQ, Szelethus, balazske.
Herald added subscribers: cfe-commits, ASDenysPetrov, steakhal, Charusso, 
gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, 
baloghadamsoftware, xazax.hun, whisperity.
Herald added a project: clang.
martong added a parent revision: D77641: [analyzer] StdLibraryFunctionsChecker: 
Associate summaries to FunctionDecls.

Once we found a matching FunctionDecl for the given summary then we
validate the given constraints against that FunctionDecl. E.g. we
validate that a NotNull constraint is applied only on arguments that
have pointer types.

This is needed because when we matched the signature of the summary we
were working with incomplete function types, i.e. some intricate type
could have been marked as `Irrelevant` in the signature.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77658

Files:
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -112,6 +112,8 @@
 virtual ValueConstraintPtr negate() const {
   llvm_unreachable("Not implemented");
 };
+/// Do sanity check on the constraint.
+virtual bool validate(const FunctionDecl *) const { return true; }
 ArgNo getArgNo() const { return ArgN; }
 
   protected:
@@ -165,6 +167,12 @@
   }
   return std::make_shared(Tmp);
 }
+
+bool validate(const FunctionDecl *FD) const override {
+  assert(getArgType(FD, ArgN)->isIntegralType(FD->getASTContext()) &&
+ "This constraint should be applied on an integral type");
+  return getArgType(FD, ArgN)->isIntegralType(FD->getASTContext());
+}
   };
 
   class ComparisonConstraint : public ValueConstraint {
@@ -205,12 +213,52 @@
   Tmp.CannotBeNull = !this->CannotBeNull;
   return std::make_shared(Tmp);
 }
+
+bool validate(const FunctionDecl *FD) const override {
+  assert(getArgType(FD, ArgN)->isPointerType() &&
+ "This constraint should be applied only on a pointer type");
+  return getArgType(FD, ArgN)->isPointerType();
+}
   };
 
   /// The complete list of constraints that defines a single branch.
   typedef std::vector ConstraintSet;
 
   using ArgTypes = std::vector;
+
+  // A placeholder type, we use it whenever we do not care about the concrete
+  // type in a Signature.
+  const QualType Irrelevant{};
+  bool static isIrrelevant(QualType T) { return T.isNull(); }
+
+  // The signature of a function we want to describe with a summary. This is a
+  // concessive signature, meaning there may be irrelevant types in the
+  // signature which we do not check against a function with concrete types.
+  struct Signature {
+const ArgTypes ArgTys;
+const QualType RetTy;
+Signature(ArgTypes ArgTys, QualType RetTy) : ArgTys(ArgTys), RetTy(RetTy) {
+  assertRetTypeSuitableForSignature(RetTy);
+  for (size_t I = 0, E = ArgTys.size(); I != E; ++I) {
+QualType ArgTy = ArgTys[I];
+assertArgTypeSuitableForSignature(ArgTy);
+  }
+}
+bool matches(const FunctionDecl *FD) const;
+
+  private:
+static void assertArgTypeSuitableForSignature(QualType T) {
+  assert((T.isNull() || !T->isVoidType()) &&
+ "We should have no void types in the spec");
+  assert((T.isNull() || T.isCanonical()) &&
+ "We should only have canonical types in the spec");
+}
+static void assertRetTypeSuitableForSignature(QualType T) {
+  assert((T.isNull() || T.isCanonical()) &&
+ "We should only have canonical types in the spec");
+}
+  };
+
   using Cases = std::vector;
 
   /// Includes information about
@@ -233,14 +281,17 @@
   /// If these constraints are not satisfied that means a fatal error
   /// usually resulting in undefined behaviour.
   struct Summary {
-const ArgTypes ArgTys;
-const QualType RetTy;
+const Signature Sign;
 const InvalidationKind InvalidationKd;
 Cases CaseConstraints;
 ConstraintSet ArgConstraints;
 
+// The function to which the summary applies. This is set after lookup and
+// match to the signature.
+const FunctionDecl *FD = nullptr;
+
 Summary(ArgTypes ArgTys, QualType RetTy, InvalidationKind InvalidationKd)
-: ArgTys(ArgTys), RetTy(RetTy), InvalidationKd(InvalidationKd) {}
+: Sign(ArgTys, RetTy), InvalidationKd(InvalidationKd) {}
 
 Summary (ConstraintSet&& CS) {
   CaseConstraints.push_back(std::move(CS));
@@ -251,26 +302,29 @@
   return *this;
 }
 
-  private:
-static void assertTypeSuitableForSummary(QualType T) {
-  assert(!T->isVoidType() &&
- "We should have had no significant void types in the