[PATCH] D77641: [analyzer] StdLibraryFunctionsChecker: Associate summaries to FunctionDecls

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D77641#2004273 , @Szelethus wrote:

> I think you can go ahead and commit. You seem to have a firm grasp on this 
> project, I believe your experience with ASTImporter has more then prepared 
> you for digging functions out of the `std` namespace, or whatever else that 
> might come up :^)


Alright, thanks, just did the commit. Let me know guys if you find anything 
else and I'll address the concerns. Also, if there is any regression I'll do 
the revert, just leave a note here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77641



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

[PATCH] D77641: [analyzer] StdLibraryFunctionsChecker: Associate summaries to FunctionDecls

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG62e747f61729: [analyzer] StdLibraryFunctionsChecker: 
Associate summaries to FunctionDecls (authored by martong).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77641

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
@@ -266,21 +266,15 @@
   return T;
 }
 
-/// Try our best to figure out if the call expression is the call of
+/// Try our best to figure out if the summary's signature matches
 /// *the* library function to which this specification applies.
-bool matchesCall(const FunctionDecl *FD) const;
+bool matchesSignature(const FunctionDecl *FD) const;
   };
 
-  // The same function (as in, function identifier) may have different
-  // summaries assigned to it, with different argument and return value types.
-  // We call these "variants" of the function. This can be useful for handling
-  // C++ function overloads, and also it can be used when the same function
-  // may have different definitions on different platforms.
-  typedef std::vector Summaries;
-
   // The map of all functions supported by the checker. It is initialized
   // lazily, and it doesn't change after initialization.
-  mutable llvm::StringMap FunctionSummaryMap;
+  using FunctionSummaryMapType = llvm::DenseMap;
+  mutable FunctionSummaryMapType FunctionSummaryMap;
 
   mutable std::unique_ptr BT_InvalidArg;
 
@@ -289,14 +283,6 @@
   static QualType getArgType(const Summary , ArgNo ArgN) {
 return Summary.getArgType(ArgN);
   }
-  static QualType getArgType(const CallEvent , ArgNo ArgN) {
-return ArgN == Ret ? Call.getResultType().getCanonicalType()
-   : Call.getArgExpr(ArgN)->getType().getCanonicalType();
-  }
-  static QualType getArgType(const CallExpr *CE, ArgNo ArgN) {
-return ArgN == Ret ? CE->getType().getCanonicalType()
-   : CE->getArg(ArgN)->getType().getCanonicalType();
-  }
   static SVal getArgSVal(const CallEvent , ArgNo ArgN) {
 return ArgN == Ret ? Call.getReturnValue() : Call.getArgSVal(ArgN);
   }
@@ -440,7 +426,7 @@
   BinaryOperator::Opcode Op = getOpcode();
   ArgNo OtherArg = getOtherArgNo();
   SVal OtherV = getArgSVal(Call, OtherArg);
-  QualType OtherT = getArgType(Call, OtherArg);
+  QualType OtherT = getArgType(Summary, OtherArg);
   // Note: we avoid integral promotion for comparison.
   OtherV = SVB.evalCast(OtherV, T, OtherT);
   if (auto CompV = SVB.evalBinOp(State, Op, V, OtherV, CondT)
@@ -530,7 +516,7 @@
   llvm_unreachable("Unknown invalidation kind!");
 }
 
-bool StdLibraryFunctionsChecker::Summary::matchesCall(
+bool StdLibraryFunctionsChecker::Summary::matchesSignature(
 const FunctionDecl *FD) const {
   // Check number of arguments:
   if (FD->param_size() != ArgTys.size())
@@ -565,28 +551,10 @@
 
   initFunctionSummaries(C);
 
-  IdentifierInfo *II = FD->getIdentifier();
-  if (!II)
-return None;
-  StringRef Name = II->getName();
-  if (Name.empty() || !C.isCLibraryFunction(FD, Name))
-return None;
-
-  auto FSMI = FunctionSummaryMap.find(Name);
+  auto FSMI = FunctionSummaryMap.find(FD->getCanonicalDecl());
   if (FSMI == FunctionSummaryMap.end())
 return None;
-
-  // Verify that function signature matches the spec in advance.
-  // Otherwise we might be modeling the wrong function.
-  // Strict checking is important because we will be conducting
-  // very integral-type-sensitive operations on arguments and
-  // return values.
-  const Summaries  = FSMI->second;
-  for (const Summary  : SpecVariants)
-if (Spec.matchesCall(FD))
-  return Spec;
-
-  return None;
+  return FSMI->second;
 }
 
 Optional
@@ -598,6 +566,21 @@
   return findFunctionSummary(FD, C);
 }
 
+llvm::Optional
+lookupGlobalCFunction(StringRef Name, const ASTContext ) {
+  IdentifierInfo  = ACtx.Idents.get(Name);
+  auto LookupRes = ACtx.getTranslationUnitDecl()->lookup();
+  if (LookupRes.size() == 0)
+return None;
+
+  assert(LookupRes.size() == 1 && "In C, identifiers should be unique");
+  Decl *D = LookupRes.front()->getCanonicalDecl();
+  auto *FD = dyn_cast(D);
+  if (!FD)
+return None;
+  return FD->getCanonicalDecl();
+}
+
 void StdLibraryFunctionsChecker::initFunctionSummaries(
 CheckerContext ) const {
   if (!FunctionSummaryMap.empty())
@@ -652,6 +635,38 @@
 return -1;
   }();
 
+  // Auxiliary class to aid adding summaries to the summary map.
+  struct AddToFunctionSummaryMap {
+const ASTContext 
+FunctionSummaryMapType 
+AddToFunctionSummaryMap(const ASTContext , FunctionSummaryMapType )
+: 

[PATCH] D77641: [analyzer] StdLibraryFunctionsChecker: Associate summaries to FunctionDecls

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

Szelethus added a comment.

I think you can go ahead and commit. You seem to have a firm grasp on this 
project, I believe your experience with ASTImporter has more then prepared you 
for digging functions out of the `std` namespace, or whatever else that might 
come up :^)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77641



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

[PATCH] D77641: [analyzer] StdLibraryFunctionsChecker: Associate summaries to FunctionDecls

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77641



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

[PATCH] D77641: [analyzer] StdLibraryFunctionsChecker: Associate summaries to FunctionDecls

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

@NoQ Do these changes look okay to land? Thanks :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77641



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

[PATCH] D77641: [analyzer] StdLibraryFunctionsChecker: Associate summaries to FunctionDecls

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D77641#1969412 , @NoQ wrote:

> So you only do the lookup in the global scope? What about `namespace std`?
>
> Do you also plan to support class methods by looking up the class first and 
> then looking up the method in the class?


Yes, I had been planning with those. When it comes to the point where we'd like 
to add summaries for C++ functions (in namespaces or member functions) then we 
can add a new overloaded `operator()` to `AddToFunctionSummaryMap`. In this new 
function we could handle a fully qualified name (e.g. `std::map::insert`) by 
doing the lookup in each found `DeclContext` starting from the 
`TranslationUnitDecl`. (Naturally, we'd not support ADL or `using` declaration 
kind of lookup modifications, only fully qualified names would be allowed.) 
Note that we could give a summary only to those functions that can be found by 
qualified [Obj]C/C++ lookup. So, e.g. functions in unnamed namespaces or 
in-class defined friend functions are out of this game.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77641



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

[PATCH] D77641: [analyzer] StdLibraryFunctionsChecker: Associate summaries to FunctionDecls

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D77641#1969291 , @Szelethus wrote:

> I suspect your change made compiler errors a bit nicer as well, so you don't 
> get one giant "Well, this huge single argument doesn't match any of the 
> assignment operators".


The reason why I added `addToFunctionSummaryMap` calls is because we have to do 
a lookup and based on the lookup result we either map the summary or not. (I 
did not have any problems with the compiler diagnostics regarding the 
initializer list.)

> We don't have any function names that have multiple summaries just yet, 
> correct?

No, we already have a few: e.g. `read`, `write`. The reason is that we may not 
know what is the real type of `ssize_t` in different platforms, so rather we 
add `int`, `long` and `long long` return type variants. (An alternative 
solution could be to support lookup for types as well, so we'd lookup `ssize_t` 
in this case.)

> Also, this change is NFC, right? Could you make a tag about that to the 
> revision title in the future?

Well, there are functional changes. We exercise the lookup mechanism that we 
had not done before. We do not add a summary to the map if we cannot lookup the 
matching prototype. This could lead to different behavior in some cases, hard 
to find one, but at least in CTU if the ASTImporter assembled an AST where the 
Decls are not added properly to their DeclContext, then lookup would not find a 
function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77641



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

[PATCH] D77641: [analyzer] StdLibraryFunctionsChecker: Associate summaries to FunctionDecls

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

So you only do the lookup in the global scope? What about `namespace std`?

Do you also plan to support class methods by looking up the class first and 
then looking up the method in the class?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77641



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

[PATCH] D77641: [analyzer] StdLibraryFunctionsChecker: Associate summaries to FunctionDecls

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

Szelethus added a comment.

Also, if you gave a bit of time for anyone else to have a say, that might be 
nice. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77641



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

[PATCH] D77641: [analyzer] StdLibraryFunctionsChecker: Associate summaries to FunctionDecls

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

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

LGTM! Its always a joy to see you do C++. I suspect your change made compiler 
errors a bit nicer as well, so you don't get one giant "Well, this huge 
single argument doesn't look match any of the assignment operators".

We don't have any function names that have multiple summaries just yet, 
correct? Also, this change is NFC, right? Could you make a tag about that to 
the revision title in the future?




Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:554
 
-  IdentifierInfo *II = FD->getIdentifier();
-  if (!II)
-return None;
-  StringRef Name = II->getName();
-  if (Name.empty() || !C.isCLibraryFunction(FD, Name))
-return None;
-
-  auto FSMI = FunctionSummaryMap.find(Name);
+  auto FSMI = FunctionSummaryMap.find(FD->getCanonicalDecl());
   if (FSMI == FunctionSummaryMap.end())

How about `FSMIt`? :^)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77641



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

[PATCH] D77641: [analyzer] StdLibraryFunctionsChecker: Associate summaries to FunctionDecls

[Gabor Marton via Phabricator via cfe-commits]

martong created this revision.
martong added reviewers: NoQ, Szelethus, balazske.
Herald added subscribers: cfe-commits, ASDenysPetrov, uenoku, steakhal, 
Charusso, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, 
szepet, baloghadamsoftware, xazax.hun, whisperity.
Herald added a reviewer: jdoerfert.
Herald added a reviewer: sstefan1.
Herald added a reviewer: uenoku.
Herald added a project: clang.
martong marked an inline comment as done.
martong added inline comments.
martong marked an inline comment as not done.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:570
+llvm::Optional
+lookupGlobalCFunction(StringRef Name, const ASTContext ) {
+  IdentifierInfo  = ACtx.Idents.get(Name);

Ups, this function is unused I'll remove.


Currently we map function summaries to names (i.e. strings). We can
associate more summaries with different signatures to one name, this way
we support overloading. During a call event we check whether the
signature of the summary matches the signature of the callee and we
apply the summary only in that case.

In this patch we change this mapping to associate a summary to a
FunctionDecl. We do lookup operations when the summary map is
initialized. We lookup the given name and we match the signature of the
given summary against the lookup results. If the summary matches the
FunctionDecl (got from the lookup result) then we add that to the
summary map. During a call event we compare FunctionDecl pointers.
Advantages of this new refactor:

- Cleaner mapping and structure for the checker.
- Possibly way more efficient handling of call events.
- A summary is added only if that is relevant for the given TU.
- We can get the concrete FunctionDecl by the time when we create the summary, 
this opens up possibilities of further sanity checks regarding the summary 
cases and argument constraints.
- Opens up to future work when we'd like to store summaries from IR to a 
FunctionDecl (or from the Attributor results of the given FunctionDecl).

Note, we cannot support old C functions without prototypes.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77641

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
@@ -266,21 +266,15 @@
   return T;
 }
 
-/// Try our best to figure out if the call expression is the call of
+/// Try our best to figure out if the summary's signature matches
 /// *the* library function to which this specification applies.
-bool matchesCall(const FunctionDecl *FD) const;
+bool matchesSignature(const FunctionDecl *FD) const;
   };
 
-  // The same function (as in, function identifier) may have different
-  // summaries assigned to it, with different argument and return value types.
-  // We call these "variants" of the function. This can be useful for handling
-  // C++ function overloads, and also it can be used when the same function
-  // may have different definitions on different platforms.
-  typedef std::vector Summaries;
-
   // The map of all functions supported by the checker. It is initialized
   // lazily, and it doesn't change after initialization.
-  mutable llvm::StringMap FunctionSummaryMap;
+  using FunctionSummaryMapType = llvm::DenseMap;
+  mutable FunctionSummaryMapType FunctionSummaryMap;
 
   mutable std::unique_ptr BT_InvalidArg;
 
@@ -289,14 +283,6 @@
   static QualType getArgType(const Summary , ArgNo ArgN) {
 return Summary.getArgType(ArgN);
   }
-  static QualType getArgType(const CallEvent , ArgNo ArgN) {
-return ArgN == Ret ? Call.getResultType().getCanonicalType()
-   : Call.getArgExpr(ArgN)->getType().getCanonicalType();
-  }
-  static QualType getArgType(const CallExpr *CE, ArgNo ArgN) {
-return ArgN == Ret ? CE->getType().getCanonicalType()
-   : CE->getArg(ArgN)->getType().getCanonicalType();
-  }
   static SVal getArgSVal(const CallEvent , ArgNo ArgN) {
 return ArgN == Ret ? Call.getReturnValue() : Call.getArgSVal(ArgN);
   }
@@ -440,7 +426,7 @@
   BinaryOperator::Opcode Op = getOpcode();
   ArgNo OtherArg = getOtherArgNo();
   SVal OtherV = getArgSVal(Call, OtherArg);
-  QualType OtherT = getArgType(Call, OtherArg);
+  QualType OtherT = getArgType(Summary, OtherArg);
   // Note: we avoid integral promotion for comparison.
   OtherV = SVB.evalCast(OtherV, T, OtherT);
   if (auto CompV = SVB.evalBinOp(State, Op, V, OtherV, CondT)
@@ -530,7 +516,7 @@
   llvm_unreachable("Unknown invalidation kind!");
 }
 
-bool StdLibraryFunctionsChecker::Summary::matchesCall(
+bool StdLibraryFunctionsChecker::Summary::matchesSignature(
 const FunctionDecl *FD) const {
   // 

[PATCH] D77641: [analyzer] StdLibraryFunctionsChecker: Associate summaries to FunctionDecls

[Gabor Marton via Phabricator via cfe-commits]

martong marked an inline comment as done.
martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:570
+llvm::Optional
+lookupGlobalCFunction(StringRef Name, const ASTContext ) {
+  IdentifierInfo  = ACtx.Idents.get(Name);

Ups, this function is unused I'll remove.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77641



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