[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2019-04-23 Thread Rafael Stahl via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rC358968: [analyzer][CrossTU] Extend CTU to VarDecls with 
initializer (authored by r.stahl, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D46421?vs=196075=196207#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D46421

Files:
  include/clang/CrossTU/CrossTranslationUnit.h
  lib/CrossTU/CrossTranslationUnit.cpp
  lib/StaticAnalyzer/Core/RegionStore.cpp
  lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  test/Analysis/Inputs/ctu-other.cpp
  test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt
  test/Analysis/ctu-main.cpp
  test/Analysis/func-mapping-test.cpp
  test/Analysis/redecl.c
  tools/clang-extdef-mapping/ClangExtDefMapGen.cpp

Index: lib/CrossTU/CrossTranslationUnit.cpp
===
--- lib/CrossTU/CrossTranslationUnit.cpp
+++ lib/CrossTU/CrossTranslationUnit.cpp
@@ -158,6 +158,27 @@
   return Result.str();
 }
 
+bool containsConst(const VarDecl *VD, const ASTContext ) {
+  CanQualType CT = ACtx.getCanonicalType(VD->getType());
+  if (!CT.isConstQualified()) {
+const RecordType *RTy = CT->getAs();
+if (!RTy || !RTy->hasConstFields())
+  return false;
+  }
+  return true;
+}
+
+static bool hasBodyOrInit(const FunctionDecl *D, const FunctionDecl *) {
+  return D->hasBody(DefD);
+}
+static bool hasBodyOrInit(const VarDecl *D, const VarDecl *) {
+  return D->getAnyInitializer(DefD);
+}
+template  static bool hasBodyOrInit(const T *D) {
+  const T *Unused;
+  return hasBodyOrInit(D, Unused);
+}
+
 CrossTranslationUnitContext::CrossTranslationUnitContext(CompilerInstance )
 : CI(CI), Context(CI.getASTContext()) {}
 
@@ -165,48 +186,50 @@
 
 std::string CrossTranslationUnitContext::getLookupName(const NamedDecl *ND) {
   SmallString<128> DeclUSR;
-  bool Ret = index::generateUSRForDecl(ND, DeclUSR); (void)Ret;
+  bool Ret = index::generateUSRForDecl(ND, DeclUSR);
+  (void)Ret;
   assert(!Ret && "Unable to generate USR");
   return DeclUSR.str();
 }
 
-/// Recursively visits the function decls of a DeclContext, and looks up a
-/// function based on USRs.
-const FunctionDecl *
-CrossTranslationUnitContext::findFunctionInDeclContext(const DeclContext *DC,
-   StringRef LookupFnName) {
+/// Recursively visits the decls of a DeclContext, and returns one with the
+/// given USR.
+template 
+const T *
+CrossTranslationUnitContext::findDefInDeclContext(const DeclContext *DC,
+  StringRef LookupName) {
   assert(DC && "Declaration Context must not be null");
   for (const Decl *D : DC->decls()) {
 const auto *SubDC = dyn_cast(D);
 if (SubDC)
-  if (const auto *FD = findFunctionInDeclContext(SubDC, LookupFnName))
-return FD;
+  if (const auto *ND = findDefInDeclContext(SubDC, LookupName))
+return ND;
 
-const auto *ND = dyn_cast(D);
-const FunctionDecl *ResultDecl;
-if (!ND || !ND->hasBody(ResultDecl))
+const auto *ND = dyn_cast(D);
+const T *ResultDecl;
+if (!ND || !hasBodyOrInit(ND, ResultDecl))
   continue;
-if (getLookupName(ResultDecl) != LookupFnName)
+if (getLookupName(ResultDecl) != LookupName)
   continue;
 return ResultDecl;
   }
   return nullptr;
 }
 
-llvm::Expected
-CrossTranslationUnitContext::getCrossTUDefinition(const FunctionDecl *FD,
-  StringRef CrossTUDir,
-  StringRef IndexName,
-  bool DisplayCTUProgress) {
-  assert(FD && "FD is missing, bad call to this function!");
-  assert(!FD->hasBody() && "FD has a definition in current translation unit!");
+template 
+llvm::Expected CrossTranslationUnitContext::getCrossTUDefinitionImpl(
+const T *D, StringRef CrossTUDir, StringRef IndexName,
+bool DisplayCTUProgress) {
+  assert(D && "D is missing, bad call to this function!");
+  assert(!hasBodyOrInit(D) &&
+ "D has a body or init in current translation unit!");
   ++NumGetCTUCalled;
-  const std::string LookupFnName = getLookupName(FD);
-  if (LookupFnName.empty())
+  const std::string LookupName = getLookupName(D);
+  if (LookupName.empty())
 return llvm::make_error(
 index_error_code::failed_to_generate_usr);
   llvm::Expected ASTUnitOrError =
-  loadExternalAST(LookupFnName, CrossTUDir, IndexName, DisplayCTUProgress);
+  loadExternalAST(LookupName, CrossTUDir, IndexName, DisplayCTUProgress);
   if (!ASTUnitOrError)
 return ASTUnitOrError.takeError();
   ASTUnit *Unit = *ASTUnitOrError;
@@ -262,12 +285,29 @@
   }
 
   TranslationUnitDecl *TU = Unit->getASTContext().getTranslationUnitDecl();
-  if (const FunctionDecl *ResultDecl =
-  findFunctionInDeclContext(TU, LookupFnName))
+  if

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2019-04-22 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added a comment.

Looks good, thanks. Can you commit this or do you need someone to commit it on 
your behalf?


Repository:
  rC Clang

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

https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2019-04-22 Thread Rafael Stahl via Phabricator via cfe-commits 
r.stahl updated this revision to Diff 196075.
r.stahl added a comment.

@xazax.hun good point and that actually fixes a bug since that branch should 
also do the deep check. Added that to the tests.


Repository:
  rC Clang

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

https://reviews.llvm.org/D46421

Files:
  include/clang/CrossTU/CrossTranslationUnit.h
  lib/CrossTU/CrossTranslationUnit.cpp
  lib/StaticAnalyzer/Core/RegionStore.cpp
  lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  test/Analysis/Inputs/ctu-other.cpp
  test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt
  test/Analysis/ctu-main.cpp
  test/Analysis/func-mapping-test.cpp
  test/Analysis/redecl.c
  tools/clang-extdef-mapping/ClangExtDefMapGen.cpp

Index: tools/clang-extdef-mapping/ClangExtDefMapGen.cpp
===
--- tools/clang-extdef-mapping/ClangExtDefMapGen.cpp
+++ tools/clang-extdef-mapping/ClangExtDefMapGen.cpp
@@ -34,20 +34,22 @@
 class MapExtDefNamesConsumer : public ASTConsumer {
 public:
   MapExtDefNamesConsumer(ASTContext )
-  : SM(Context.getSourceManager()) {}
+  : Ctx(Context), SM(Context.getSourceManager()) {}
 
   ~MapExtDefNamesConsumer() {
 // Flush results to standard output.
 llvm::outs() << createCrossTUIndexString(Index);
   }
 
-  void HandleTranslationUnit(ASTContext ) override {
-handleDecl(Ctx.getTranslationUnitDecl());
+  void HandleTranslationUnit(ASTContext ) override {
+handleDecl(Context.getTranslationUnitDecl());
   }
 
 private:
   void handleDecl(const Decl *D);
+  void addIfInMain(const DeclaratorDecl *DD, SourceLocation defStart);
 
+  ASTContext 
   SourceManager 
   llvm::StringMap Index;
   std::string CurrentFileName;
@@ -58,30 +60,13 @@
 return;
 
   if (const auto *FD = dyn_cast(D)) {
-if (FD->isThisDeclarationADefinition()) {
-  if (const Stmt *Body = FD->getBody()) {
-if (CurrentFileName.empty()) {
-  CurrentFileName =
-  SM.getFileEntryForID(SM.getMainFileID())->tryGetRealPathName();
-  if (CurrentFileName.empty())
-CurrentFileName = "invalid_file";
-}
-
-switch (FD->getLinkageInternal()) {
-case ExternalLinkage:
-case VisibleNoLinkage:
-case UniqueExternalLinkage:
-  if (SM.isInMainFile(Body->getBeginLoc())) {
-std::string LookupName =
-CrossTranslationUnitContext::getLookupName(FD);
-Index[LookupName] = CurrentFileName;
-  }
-  break;
-default:
-  break;
-}
-  }
-}
+if (FD->isThisDeclarationADefinition())
+  if (const Stmt *Body = FD->getBody())
+addIfInMain(FD, Body->getBeginLoc());
+  } else if (const auto *VD = dyn_cast(D)) {
+if (cross_tu::containsConst(VD, Ctx) && VD->hasInit())
+  if (const Expr *Init = VD->getInit())
+addIfInMain(VD, Init->getBeginLoc());
   }
 
   if (const auto *DC = dyn_cast(D))
@@ -89,6 +74,27 @@
   handleDecl(D);
 }
 
+void MapExtDefNamesConsumer::addIfInMain(const DeclaratorDecl *DD,
+ SourceLocation defStart) {
+  std::string LookupName = CrossTranslationUnitContext::getLookupName(DD);
+  if (CurrentFileName.empty()) {
+CurrentFileName =
+SM.getFileEntryForID(SM.getMainFileID())->tryGetRealPathName();
+if (CurrentFileName.empty())
+  CurrentFileName = "invalid_file";
+  }
+
+  switch (DD->getLinkageInternal()) {
+  case ExternalLinkage:
+  case VisibleNoLinkage:
+  case UniqueExternalLinkage:
+if (SM.isInMainFile(defStart))
+  Index[LookupName] = CurrentFileName;
+  default:
+break;
+  }
+}
+
 class MapExtDefNamesAction : public ASTFrontendAction {
 protected:
   std::unique_ptr CreateASTConsumer(CompilerInstance ,
Index: test/Analysis/redecl.c
===
--- /dev/null
+++ test/Analysis/redecl.c
@@ -0,0 +1,13 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+// XFAIL: *
+
+void clang_analyzer_eval(int);
+
+extern const int extInt;
+
+int main()
+{
+clang_analyzer_eval(extInt == 2); // expected-warning{{TRUE}}
+}
+
+extern const int extInt = 2;
Index: test/Analysis/func-mapping-test.cpp
===
--- test/Analysis/func-mapping-test.cpp
+++ test/Analysis/func-mapping-test.cpp
@@ -1,7 +1,43 @@
-// RUN: %clang_extdef_map %s -- | FileCheck %s
+// RUN: %clang_extdef_map %s -- | FileCheck --implicit-check-not "c:@y" --implicit-check-not "c:@z" %s
 
 int f(int) {
   return 0;
 }
+// CHECK-DAG: c:@F@f#I#
 
-// CHECK: c:@F@f#I#
+extern const int x = 5;
+// CHECK-DAG: c:@x
+
+// Non-const variables should not be collected.
+int y = 5;
+
+// In C++, const implies internal linkage, so not collected.
+const int z = 5;
+
+struct S {
+  int a;
+};
+extern S const s = {.a = 2};
+// CHECK-DAG: c:@s
+
+struct SF {
+  const int

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2019-04-11 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun accepted this revision.
xazax.hun added a comment.
This revision is now accepted and ready to land.

I have one question, once it is resolved I am fine with committing this.




Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:352
+return true;
+} else if (VD->isStaticDataMember()) {
+  // Only import if const.

Do we actually need this branch? I would expect `cross_tu::containsConst(VD, 
*Ctx)` to return true for all the cases where we would return true here.


Repository:
  rC Clang

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

https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2019-04-09 Thread Rafael Stahl via Phabricator via cfe-commits 
r.stahl updated this revision to Diff 194285.
r.stahl marked 4 inline comments as done.
r.stahl added a comment.

- addressed review comments
- created cross_tu::containsConst
- fixed bug that unions were not exported
- added TODO test for constructor case


Repository:
  rC Clang

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

https://reviews.llvm.org/D46421

Files:
  include/clang/CrossTU/CrossTranslationUnit.h
  lib/CrossTU/CrossTranslationUnit.cpp
  lib/StaticAnalyzer/Core/RegionStore.cpp
  lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  test/Analysis/Inputs/ctu-other.cpp
  test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt
  test/Analysis/ctu-main.cpp
  test/Analysis/func-mapping-test.cpp
  test/Analysis/redecl.c
  tools/clang-extdef-mapping/ClangExtDefMapGen.cpp

Index: tools/clang-extdef-mapping/ClangExtDefMapGen.cpp
===
--- tools/clang-extdef-mapping/ClangExtDefMapGen.cpp
+++ tools/clang-extdef-mapping/ClangExtDefMapGen.cpp
@@ -34,20 +34,22 @@
 class MapExtDefNamesConsumer : public ASTConsumer {
 public:
   MapExtDefNamesConsumer(ASTContext )
-  : SM(Context.getSourceManager()) {}
+  : Ctx(Context), SM(Context.getSourceManager()) {}
 
   ~MapExtDefNamesConsumer() {
 // Flush results to standard output.
 llvm::outs() << createCrossTUIndexString(Index);
   }
 
-  void HandleTranslationUnit(ASTContext ) override {
-handleDecl(Ctx.getTranslationUnitDecl());
+  void HandleTranslationUnit(ASTContext ) override {
+handleDecl(Context.getTranslationUnitDecl());
   }
 
 private:
   void handleDecl(const Decl *D);
+  void addIfInMain(const DeclaratorDecl *DD, SourceLocation defStart);
 
+  ASTContext 
   SourceManager 
   llvm::StringMap Index;
   std::string CurrentFileName;
@@ -58,30 +60,13 @@
 return;
 
   if (const auto *FD = dyn_cast(D)) {
-if (FD->isThisDeclarationADefinition()) {
-  if (const Stmt *Body = FD->getBody()) {
-if (CurrentFileName.empty()) {
-  CurrentFileName =
-  SM.getFileEntryForID(SM.getMainFileID())->tryGetRealPathName();
-  if (CurrentFileName.empty())
-CurrentFileName = "invalid_file";
-}
-
-switch (FD->getLinkageInternal()) {
-case ExternalLinkage:
-case VisibleNoLinkage:
-case UniqueExternalLinkage:
-  if (SM.isInMainFile(Body->getBeginLoc())) {
-std::string LookupName =
-CrossTranslationUnitContext::getLookupName(FD);
-Index[LookupName] = CurrentFileName;
-  }
-  break;
-default:
-  break;
-}
-  }
-}
+if (FD->isThisDeclarationADefinition())
+  if (const Stmt *Body = FD->getBody())
+addIfInMain(FD, Body->getBeginLoc());
+  } else if (const auto *VD = dyn_cast(D)) {
+if (cross_tu::containsConst(VD, Ctx) && VD->hasInit())
+  if (const Expr *Init = VD->getInit())
+addIfInMain(VD, Init->getBeginLoc());
   }
 
   if (const auto *DC = dyn_cast(D))
@@ -89,6 +74,27 @@
   handleDecl(D);
 }
 
+void MapExtDefNamesConsumer::addIfInMain(const DeclaratorDecl *DD,
+ SourceLocation defStart) {
+  std::string LookupName = CrossTranslationUnitContext::getLookupName(DD);
+  if (CurrentFileName.empty()) {
+CurrentFileName =
+SM.getFileEntryForID(SM.getMainFileID())->tryGetRealPathName();
+if (CurrentFileName.empty())
+  CurrentFileName = "invalid_file";
+  }
+
+  switch (DD->getLinkageInternal()) {
+  case ExternalLinkage:
+  case VisibleNoLinkage:
+  case UniqueExternalLinkage:
+if (SM.isInMainFile(defStart))
+  Index[LookupName] = CurrentFileName;
+  default:
+break;
+  }
+}
+
 class MapExtDefNamesAction : public ASTFrontendAction {
 protected:
   std::unique_ptr CreateASTConsumer(CompilerInstance ,
Index: test/Analysis/redecl.c
===
--- /dev/null
+++ test/Analysis/redecl.c
@@ -0,0 +1,13 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+// XFAIL: *
+
+void clang_analyzer_eval(int);
+
+extern const int extInt;
+
+int main()
+{
+clang_analyzer_eval(extInt == 2); // expected-warning{{TRUE}}
+}
+
+extern const int extInt = 2;
Index: test/Analysis/func-mapping-test.cpp
===
--- test/Analysis/func-mapping-test.cpp
+++ test/Analysis/func-mapping-test.cpp
@@ -1,7 +1,43 @@
-// RUN: %clang_extdef_map %s -- | FileCheck %s
+// RUN: %clang_extdef_map %s -- | FileCheck --implicit-check-not "c:@y" --implicit-check-not "c:@z" %s
 
 int f(int) {
   return 0;
 }
+// CHECK-DAG: c:@F@f#I#
 
-// CHECK: c:@F@f#I#
+extern const int x = 5;
+// CHECK-DAG: c:@x
+
+// Non-const variables should not be collected.
+int y = 5;
+
+// In C++, const implies internal linkage, so not collected.
+const int z = 5;
+
+struct S {
+  int a;
+};
+extern S const s =

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2019-04-09 Thread Rafael Stahl via Phabricator via cfe-commits 
r.stahl added inline comments.



Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:357
+  return true;
+if (!RTy->hasConstFields())
+  return true;

xazax.hun wrote:
> I wonder what would happen with types that has const fields and user written 
> constructors. In case we will not simulate the effect of the constructor and 
> will not be able to set the const fields maybe we should exclude those as 
> well?
I added a test for that and it doesn't seem to work. It just ends up as unknown 
which is fine, right? Eventually it would be nice to do that of course.


Repository:
  rC Clang

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

https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2019-04-05 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added a comment.

I cannot think of other users, so I would prefer to put it in the CTU lib for 
now.


Repository:
  rC Clang

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

https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2019-04-05 Thread Rafael Stahl via Phabricator via cfe-commits 
r.stahl marked 3 inline comments as done.
r.stahl added a comment.

In D46421#1456171 , @xazax.hun wrote:

> My last set of comments are also unresolved. Most of them are minor nits, but 
> I would love to get rid of the code duplication between ClangExtDefMapGen and 
> the Clang Static Analyzer regarding when do we consider a variable worth to 
> import. Otherwise the patch looks good to me.


Sorry, was too long ago, thought this was done!

What would be a good place for this? I'm thinking ExprClassification in the AST 
lib. There is a tail in `IsModifiable` that tests the `CanQualType` for const 
that could be moved out. And then make a new function `bool ContainsConst(const 
VarDecl *VD)` mabye? Shouldn't that be a new patch, because it touches the more 
critical AST lib?


Repository:
  rC Clang

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

https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2019-04-05 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added a comment.

In D46421#1456155 , @r.stahl wrote:

> Okay so I would suggest to go ahead and commit this. Rebased it succeeds 
> without modification.
>
> Still leaves the open problems with the redecls. Should I add the failing 
> test from https://reviews.llvm.org/D46421#1375147 in the same commit marked 
> as expected to fail? Or what is the procedure here?


My last set of comments are also unresolved. Most of them are minor nits, but I 
would love to get rid of the code duplication between ClangExtDefMapGen and the 
Clang Static Analyzer regarding when do we consider a variable worth to import. 
Otherwise the patch looks good to me.


Repository:
  rC Clang

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

https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2019-04-05 Thread Rafael Stahl via Phabricator via cfe-commits 
r.stahl added a comment.

Okay so I would suggest to go ahead and commit this. Rebased it succeeds 
without modification.

Still leaves the open problems with the redecls. Should I add the failing test 
from https://reviews.llvm.org/D46421#1375147 in the same commit marked as 
expected to fail? Or what is the procedure here?


Repository:
  rC Clang

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

https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2019-02-26 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added a comment.
Herald added a subscriber: Charusso.

In D46421#1382017 , @martong wrote:

> > I think you should change back to getInit()
>
> I am not entirely sure about this because the initalizer may not be attached 
> to the canonical decl. `getInit()` gives the initializer of one given 
> specific Decl, however, `getAnyInitializer()` searches through the whole 
> redecl chain.


Ugh, indeed. `getDefinition()` is also not the thing we're looking for. What a 
mess :) Tests are welcome!~

P.S. I don't intend to block this patch; you guys know your stuff, i'm just 
displaying a bit of curiosity and observing how it unfolds.


Repository:
  rC Clang

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

https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2019-02-02 Thread Gabor Marton via Phabricator via cfe-commits 
martong added a comment.

> I think you should change back to getInit()

I am not entirely sure about this because the initalizer may not be attached to 
the canonical decl. `getInit()` gives the initializer of one given specific 
Decl, however, `getAnyInitializer()` searches through the whole redecl chain.


Repository:
  rC Clang

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

https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2019-02-01 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added a comment.

Fxd the global variable problem in D57619 , 
thanks! I think you should change back to `getInit()` once D57619 
 lands.


Repository:
  rC Clang

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

https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2019-02-01 Thread Gabor Marton via Phabricator via cfe-commits 
martong added a comment.

> Previously this was not required since all VarDecls were canonical. Not sure 
> if this change was intended. I did some digging, but am not familiar enough 
> with the code base to figure out what changed. Does anyone have an idea about 
> this?

We changed the ASTImporter a few months ago to import the redecl chains 
properly. That means we do not squash ever decl into one decl. Also we link a 
newly imported decl to an already existing (found by the lookup). We allow only 
one definition of course. In case of variables we allow only one initializer. 
This way we can preserve the original structure of the AST in the most precise 
way.
I don't think this could be a problem, since we may have redecl chains of 
variables (and other kind of decls) in a single TU too.


Repository:
  rC Clang

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

https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2019-02-01 Thread Rafael Stahl via Phabricator via cfe-commits 
r.stahl added a comment.

Thanks for the comments! All good points. Nice idea with the constructor, but 
that can probably happen in a follow up patch.

In D46421#1380619 , @xazax.hun wrote:

> I know you probably tired of me making you split all the patches but I think 
> if it is independent of CTU we should submit this fix as a separate commit. 
> This way we could be more focused having one commit doing one thing.


No problem if I should do that. However I'm concerned whether this change is 
correct. Previously this was not required since all VarDecls were canonical. 
Not sure if this change was intended. I did some digging, but am not familiar 
enough with the code base to figure out what changed. Does anyone have an idea 
about this?

But I agree that if it wasn't a deliberate change, we could canonicalize in the 
DeclRegion constructor - or VarRegion since only those are redeclarable.


Repository:
  rC Clang

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

https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2019-02-01 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added a comment.

Thank you for working on this!

I have two concerns:

- The logic when should we import the initializer of a VarDecl is duplicated. I 
think to have better maintainability it would be better to have only one 
implementation of the decision in a function and call it multiple times.
- While the static analyzer is currently the only user of CTU lib it might 
change in the future. Only importing initializers of const variables is an 
analyzer specific decision which might not be good for other potential users of 
the CTU library. Maybe it would be better to be able to configure the 
ClangExtDefMapGen somehow, so it is able to both export only vardecls that are 
required by the analyzer or anything?




Comment at: lib/CrossTU/CrossTranslationUnit.cpp:163
+static bool hasBodyOrInit(const VarDecl *D, const VarDecl *) {
+  return D->getAnyInitializer(DefD) != nullptr;
+}

The `!= nullptr` part is usually not written in LLVM codebase.



Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:357
+  return true;
+if (!RTy->hasConstFields())
+  return true;

I wonder what would happen with types that has const fields and user written 
constructors. In case we will not simulate the effect of the constructor and 
will not be able to set the const fields maybe we should exclude those as well?



Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:362
+  // Only import if const.
+  if (!Ctx->getCanonicalType(VD->getType()).isConstQualified())
+return true;

Maybe this check could be hoisted so we do not need to repeat in both branches?



Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:369
+
+if (VD->getAnyInitializer() != nullptr)
+  return true;

Redundant `!= nullptr`.



Comment at: tools/clang-extdef-mapping/ClangExtDefMapGen.cpp:68
+QualType VTy = VD->getType();
+bool containsConst = VTy.isConstQualified();
+if (!containsConst && !VTy.isNull())

Variables should start with a capital letter.


Repository:
  rC Clang

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

https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2019-02-01 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added a comment.
Herald added a project: clang.

In D46421#1375147 , @r.stahl wrote:

> In D46421#1374807 , @NoQ wrote:
>
> > At the same time, i don't have any test cases for the actual change in 
> > behavior that such canonicalization causes. If the test case that you had 
> > in mind is indeed demonstrating this problem, i'd love to have it. If it 
> > turns out that your test case doesn't allow us to demonstrate the problem 
> > without CTU, then probably it has something to do with `ASTImporter` 
> > accidentally canonicalizing the the declaration in `DeclRefExpr` more 
> > rarely than the vanilla AST.
>
>
> This seems unrelated to CTU. The following subset of my test demonstrates 
> this:
>
>   // RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection 
> -verify %s
>  
>   void clang_analyzer_eval(int);
>  
>   extern const int extInt;
>  
>   int main()
>   {
>   clang_analyzer_eval(extInt == 2); // expected-warning{{TRUE}}
>   }
>  
>   extern const int extInt = 2;
>  
>
>
>
>
>   Breakpoint 1, (anonymous namespace)::RegionStoreManager::getBindingForVar 
> (this=0xa7b420, B=..., R=0xa7d348)
>   at 
> /data/work/commitllvm/llvm/tools/clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1948
>   1948if (const Expr *Init = VD->getAnyInitializer()) {
>   (gdb) p VD->getInit()
>   $1 = (const clang::Expr *) 0x0
>   (gdb) p VD->getAnyInitializer()
>   $2 = (const clang::Expr *) 0xa4b630
>  
>


I know you probably tired of me making you split all the patches but I think if 
it is independent of CTU we should submit this fix as a separate commit. This 
way we could be more focused having one commit doing one thing.


Repository:
  rC Clang

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

https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2019-01-29 Thread Rafael Stahl via Phabricator via cfe-commits 
r.stahl added a comment.

In D46421#1374807 , @NoQ wrote:

> At the same time, i don't have any test cases for the actual change in 
> behavior that such canonicalization causes. If the test case that you had in 
> mind is indeed demonstrating this problem, i'd love to have it. If it turns 
> out that your test case doesn't allow us to demonstrate the problem without 
> CTU, then probably it has something to do with `ASTImporter` accidentally 
> canonicalizing the the declaration in `DeclRefExpr` more rarely than the 
> vanilla AST.


This seems unrelated to CTU. The following subset of my test demonstrates this:

  // RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection 
-verify %s
  
  void clang_analyzer_eval(int);
  
  extern const int extInt;
  
  int main()
  {
  clang_analyzer_eval(extInt == 2); // expected-warning{{TRUE}}
  }
  
  extern const int extInt = 2;



  Breakpoint 1, (anonymous namespace)::RegionStoreManager::getBindingForVar 
(this=0xa7b420, B=..., R=0xa7d348)
  at 
/data/work/commitllvm/llvm/tools/clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1948
  1948if (const Expr *Init = VD->getAnyInitializer()) {
  (gdb) p VD->getInit()
  $1 = (const clang::Expr *) 0x0
  (gdb) p VD->getAnyInitializer()
  $2 = (const clang::Expr *) 0xa4b630


Repository:
  rC Clang

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

https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2019-01-28 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added a comment.

In D46421#1354188 , @r.stahl wrote:

> In my old version I seemed to get away with the tests, but they failed after 
> rebasing. It seems like earlier there was only a single VarDecl for the 
> imported ones with InitExpr. Now after importing there is one without init 
> and a redecl with the init. This is why I changed getInit() in RegionStore to 
> getAnyInititializer. I think these three should be enough, but I'm not sure 
> where else in the analyzer this would have to be changed.


Hmm, that is actually pretty interesting and sounds pretty bad: it seems that 
we are constructing a `VarRegion` with a non-canonical `VarDecl`. In other 
words, it turns out that `VarRegion` that is constructed for a `DeclRefExpr` 
may depend on which of the re-declarations of the variable does `DeclRefExpr` 
refer to. Which means that we potentially construct two different `VarRegion`s 
for the same variable during analysis. It should always refer to the canonical 
declaration (which should, as far as i understand, be the one that has an 
initializer on it, if any).

The problem can be easily demonstrated by adding `assert(d->isCanonicalDecl()` 
to `DeclRegion`'s constructor and running tests (a few should fail, including 
`ctu-main.c`), though i'd rather prefer the constructor to automatically 
canonicalize the declaration - which statically ensures that this sort of stuff 
doesn't happen (for a tiny performance cost). I also wonder if `DeclRefExpr` in 
a fully constructed AST should always point to the canonical declaration - 
maybe that's the thing that's broken.

At the same time, i don't have any test cases for the actual change in behavior 
that such canonicalization causes. If the test case that you had in mind is 
indeed demonstrating this problem, i'd love to have it. If it turns out that 
your test case doesn't allow us to demonstrate the problem without CTU, then 
probably it has something to do with `ASTImporter` accidentally canonicalizing 
the the declaration in `DeclRefExpr` more rarely than the vanilla AST.


Repository:
  rC Clang

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

https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2019-01-11 Thread Rafael Stahl via Phabricator via cfe-commits 
r.stahl updated this revision to Diff 181263.
r.stahl marked 12 inline comments as done.
r.stahl added a comment.

Strip name changes (see D56441 ); addressed 
review comments

In my old version I seemed to get away with the tests, but they failed after 
rebasing. It seems like earlier there was only a single VarDecl for the 
imported ones with InitExpr. Now after importing there is one without init and 
a redecl with the init. This is why I changed getInit() in RegionStore to 
getAnyInititializer. I think these three should be enough, but I'm not sure 
where else in the analyzer this would have to be changed.

I also noticed that nested struct inits don't work, but this is unrelated to 
this patch, so I commented out the test for now and will send a separate patch.


Repository:
  rC Clang

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

https://reviews.llvm.org/D46421

Files:
  include/clang/CrossTU/CrossTranslationUnit.h
  lib/CrossTU/CrossTranslationUnit.cpp
  lib/StaticAnalyzer/Core/RegionStore.cpp
  lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  test/Analysis/Inputs/ctu-other.cpp
  test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt
  test/Analysis/ctu-main.cpp
  test/Analysis/func-mapping-test.cpp
  tools/clang-extdef-mapping/ClangExtDefMapGen.cpp

Index: tools/clang-extdef-mapping/ClangExtDefMapGen.cpp
===
--- tools/clang-extdef-mapping/ClangExtDefMapGen.cpp
+++ tools/clang-extdef-mapping/ClangExtDefMapGen.cpp
@@ -48,6 +48,7 @@
 
 private:
   void handleDecl(const Decl *D);
+  void addIfInMain(const DeclaratorDecl *DD, SourceLocation defStart);
 
   SourceManager 
   llvm::StringMap Index;
@@ -59,30 +60,19 @@
 return;
 
   if (const auto *FD = dyn_cast(D)) {
-if (FD->isThisDeclarationADefinition()) {
-  if (const Stmt *Body = FD->getBody()) {
-if (CurrentFileName.empty()) {
-  CurrentFileName =
-  SM.getFileEntryForID(SM.getMainFileID())->tryGetRealPathName();
-  if (CurrentFileName.empty())
-CurrentFileName = "invalid_file";
-}
-
-switch (FD->getLinkageInternal()) {
-case ExternalLinkage:
-case VisibleNoLinkage:
-case UniqueExternalLinkage:
-  if (SM.isInMainFile(Body->getBeginLoc())) {
-std::string LookupName =
-CrossTranslationUnitContext::getLookupName(FD);
-Index[LookupName] = CurrentFileName;
-  }
-  break;
-default:
-  break;
-}
-  }
-}
+if (FD->isThisDeclarationADefinition())
+  if (const Stmt *Body = FD->getBody())
+addIfInMain(FD, Body->getBeginLoc());
+  } else if (const auto *VD = dyn_cast(D)) {
+QualType VTy = VD->getType();
+bool containsConst = VTy.isConstQualified();
+if (!containsConst && !VTy.isNull())
+  if (const RecordType *RTy = VTy->getAsStructureType())
+containsConst = RTy->hasConstFields();
+
+if (containsConst && VD->hasInit())
+  if (const Expr *Init = VD->getInit())
+addIfInMain(VD, Init->getBeginLoc());
   }
 
   if (const auto *DC = dyn_cast(D))
@@ -90,6 +80,27 @@
   handleDecl(D);
 }
 
+void MapExtDefNamesConsumer::addIfInMain(const DeclaratorDecl *DD,
+ SourceLocation defStart) {
+  std::string LookupName = CrossTranslationUnitContext::getLookupName(DD);
+  if (CurrentFileName.empty()) {
+CurrentFileName =
+SM.getFileEntryForID(SM.getMainFileID())->tryGetRealPathName();
+if (CurrentFileName.empty())
+  CurrentFileName = "invalid_file";
+  }
+
+  switch (DD->getLinkageInternal()) {
+  case ExternalLinkage:
+  case VisibleNoLinkage:
+  case UniqueExternalLinkage:
+if (SM.isInMainFile(defStart))
+  Index[LookupName] = CurrentFileName;
+  default:
+break;
+  }
+}
+
 class MapExtDefNamesAction : public ASTFrontendAction {
 protected:
   std::unique_ptr CreateASTConsumer(CompilerInstance ,
Index: test/Analysis/func-mapping-test.cpp
===
--- test/Analysis/func-mapping-test.cpp
+++ test/Analysis/func-mapping-test.cpp
@@ -1,7 +1,36 @@
-// RUN: %clang_extdef_map %s -- | FileCheck %s
+// RUN: %clang_extdef_map %s -- | FileCheck --implicit-check-not "c:@y" --implicit-check-not "c:@z" %s
 
 int f(int) {
   return 0;
 }
+// CHECK-DAG: c:@F@f#I#
 
-// CHECK: c:@F@f#I#
+extern const int x = 5;
+// CHECK-DAG: c:@x
+
+// Non-const variables should not be collected.
+int y = 5;
+
+// In C++, const implies internal linkage, so not collected.
+const int z = 5;
+
+struct S {
+  int a;
+};
+extern S const s = {.a = 2};
+// CHECK-DAG: c:@s
+
+struct SF {
+  const int a;
+};
+SF sf = {.a = 2};
+// CHECK-DAG: c:@sf
+
+struct SStatic {
+  static const int a = 4;
+};
+const int SStatic::a;
+// CHECK-DAG: c:@S@SStatic@a
+
+extern int const arr[5] = { 0, 1 };
+// CHECK-DAG: c:@arr
\ No

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2019-01-11 Thread Rafael Stahl via Phabricator via cfe-commits 
r.stahl added inline comments.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:120
+}
+template  static bool hasDefinition(const T *D) {
+  const T *Unused;

martong wrote:
> `hasDefinitionOrInit` ?
I simply made it `hasBodyOrInit` now to be as clear as possible


Repository:
  rC Clang

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

https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2018-12-14 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added a comment.

In D46421#1121080 , @r.stahl wrote:

> It seems like a good idea to not do that, since non-const values are not 
> used. It might become useful if we ever do some kind of straight line 
> execution from static initialization to main.
>  However for structs it is enough if one of their fields is declared const.


Aaand in C++ there's also the `mutable` keyword that can cancel the effect of 
the surrounding `const` keyword, at least for the purposes of precise memory 
contents modeling in `RegionStore`.

The idea looks great to me. It was far from obvious to me that importing 
variables manually was necessary, nice catch.

I definitely wish for a more direct test for this, i.e. "CTU analysis avoids 
that specific false positive due to the new functionality".


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

https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2018-12-14 Thread Gabor Marton via Phabricator via cfe-commits 
martong added a comment.

Hi Rafael,

This is a great patch and good improvement, thank you! I had a few minor 
comments.
(Also, I was thinking about what else could we import in the future, maybe 
definitions of C++11 enum classes would be also worth to be handled by CTU.)




Comment at: include/clang/CrossTU/CrossTranslationUnit.h:114
+  llvm::Expected
+  getCrossTUDefinition(const VarDecl *VD, StringRef CrossTUDir,
+   StringRef IndexName);

r.stahl wrote:
> xazax.hun wrote:
> > I wonder if we need these overloads. Maybe having only the templated 
> > version and having a static assert for the supported types is better? 
> > Asking the other reviewers as well.
> They are not required, but I thought they make the interface more clear and 
> help prevent implementation in headers.
I consider the new overload just great. Later, if we want we still can extend 
the interface with a template which would call the overloaded functions.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:117
+}
+static bool hasDefinition(const VarDecl *D, const VarDecl *) {
+  return D->getAnyInitializer(DefD) != nullptr;

The naming here is confusing for me, would be better to be `hasInit`, because 
there are cases when a VarDecl has an initializer but it is not a definition:
```
  struct A {
static const int a = 1 + 2; // VarDecl: has init, but not a definition
  };
  const int A::a; // VarDecl: definition
```
In the above case we probably want to import the initializer and we don't care 
about the definition.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:120
+}
+template  static bool hasDefinition(const T *D) {
+  const T *Unused;

`hasDefinitionOrInit` ?



Comment at: test/Analysis/Inputs/ctu-other.cpp:79
+};
+extern const S extS = {.a = 4};

Could we have another test case when `S::a` is static?



Comment at: test/Analysis/func-mapping-test.cpp:18
+struct S {
+  int a;
+};

Could we have one more test when we have a `static` member variable?


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

https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2018-12-14 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added a comment.

Sorry for the delay. The changes are looking good to me, although I think it 
might be worth to split this up into two patch, one NFC with the renaming, and 
one that actually introduces the changes.

@martong could you also take a look?

First, we would also like to test this internally. just to make sure it works 
as intended. Could you rebase the path to current top of tree?

Thanks!




Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:350
+  bool VisitVarDecl(VarDecl *VD) {
+if (!Opts->naiveCTUEnabled())
+  return true;

Maybe we should also opt out for non-const VarDecls to avoid even attempting to 
import the initializer if we will not find it anyways.


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

https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2018-12-14 Thread Rafael Stahl via Phabricator via cfe-commits 
r.stahl added a comment.
Herald added subscribers: dkrupp, donat.nagy, Szelethus, mikhail.ramalho, 
baloghadamsoftware.

Please let me know if there is anything else that should be addressed.

Note that even though this diff is quite large, most is just renaming things to 
stay consistent.


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

https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2018-06-05 Thread Rafael Stahl via Phabricator via cfe-commits 
r.stahl updated this revision to Diff 149959.
r.stahl added a comment.

- add missing AnalysisConsumer diff
- only collect const qualified vardecls in the extdef index and adjust test


https://reviews.llvm.org/D46421

Files:
  include/clang/Basic/DiagnosticCrossTUKinds.td
  include/clang/CrossTU/CrossTranslationUnit.h
  include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  lib/CrossTU/CrossTranslationUnit.cpp
  lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
  lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  test/Analysis/Inputs/ctu-other.cpp
  test/Analysis/Inputs/externalDefMap.txt
  test/Analysis/Inputs/externalFnMap.txt
  test/Analysis/ctu-main.cpp
  test/Analysis/func-mapping-test.cpp
  test/CMakeLists.txt
  test/lit.cfg.py
  tools/CMakeLists.txt
  tools/clang-extdef-mapping/CMakeLists.txt
  tools/clang-extdef-mapping/ClangExtDefMapGen.cpp
  tools/clang-func-mapping/CMakeLists.txt
  tools/clang-func-mapping/ClangFnMapGen.cpp
  tools/scan-build-py/README.md
  tools/scan-build-py/libscanbuild/__init__.py
  tools/scan-build-py/libscanbuild/analyze.py
  tools/scan-build-py/libscanbuild/arguments.py
  tools/scan-build-py/libscanbuild/clang.py
  tools/scan-build-py/tests/unit/test_analyze.py
  tools/scan-build-py/tests/unit/test_clang.py

Index: tools/scan-build-py/tests/unit/test_clang.py
===
--- tools/scan-build-py/tests/unit/test_clang.py
+++ tools/scan-build-py/tests/unit/test_clang.py
@@ -96,7 +96,7 @@
 
 class ClangIsCtuCapableTest(unittest.TestCase):
 def test_ctu_not_found(self):
-is_ctu = sut.is_ctu_capable('not-found-clang-func-mapping')
+is_ctu = sut.is_ctu_capable('not-found-clang-extdef-mapping')
 self.assertFalse(is_ctu)
 
 
Index: tools/scan-build-py/tests/unit/test_analyze.py
===
--- tools/scan-build-py/tests/unit/test_analyze.py
+++ tools/scan-build-py/tests/unit/test_analyze.py
@@ -349,14 +349,14 @@
 class MergeCtuMapTest(unittest.TestCase):
 
 def test_no_map_gives_empty(self):
-pairs = sut.create_global_ctu_function_map([])
+pairs = sut.create_global_ctu_extdef_map([])
 self.assertFalse(pairs)
 
 def test_multiple_maps_merged(self):
 concat_map = ['c:@F@fun1#I# ast/fun1.c.ast',
   'c:@F@fun2#I# ast/fun2.c.ast',
   'c:@F@fun3#I# ast/fun3.c.ast']
-pairs = sut.create_global_ctu_function_map(concat_map)
+pairs = sut.create_global_ctu_extdef_map(concat_map)
 self.assertTrue(('c:@F@fun1#I#', 'ast/fun1.c.ast') in pairs)
 self.assertTrue(('c:@F@fun2#I#', 'ast/fun2.c.ast') in pairs)
 self.assertTrue(('c:@F@fun3#I#', 'ast/fun3.c.ast') in pairs)
@@ -366,7 +366,7 @@
 concat_map = ['c:@F@fun1#I# ast/fun1.c.ast',
   'c:@F@fun2#I# ast/fun2.c.ast',
   'c:@F@fun1#I# ast/fun7.c.ast']
-pairs = sut.create_global_ctu_function_map(concat_map)
+pairs = sut.create_global_ctu_extdef_map(concat_map)
 self.assertFalse(('c:@F@fun1#I#', 'ast/fun1.c.ast') in pairs)
 self.assertFalse(('c:@F@fun1#I#', 'ast/fun7.c.ast') in pairs)
 self.assertTrue(('c:@F@fun2#I#', 'ast/fun2.c.ast') in pairs)
@@ -376,28 +376,28 @@
 concat_map = ['c:@F@fun1#I# ast/fun1.c.ast',
   'c:@F@fun2#I# ast/fun2.c.ast',
   'c:@F@fun1#I# ast/fun1.c.ast']
-pairs = sut.create_global_ctu_function_map(concat_map)
+pairs = sut.create_global_ctu_extdef_map(concat_map)
 self.assertTrue(('c:@F@fun1#I#', 'ast/fun1.c.ast') in pairs)
 self.assertTrue(('c:@F@fun2#I#', 'ast/fun2.c.ast') in pairs)
 self.assertEqual(2, len(pairs))
 
 def test_space_handled_in_source(self):
 concat_map = ['c:@F@fun1#I# ast/f un.c.ast']
-pairs = sut.create_global_ctu_function_map(concat_map)
+pairs = sut.create_global_ctu_extdef_map(concat_map)
 self.assertTrue(('c:@F@fun1#I#', 'ast/f un.c.ast') in pairs)
 self.assertEqual(1, len(pairs))
 
 
-class FuncMapSrcToAstTest(unittest.TestCase):
+class ExtdefMapSrcToAstTest(unittest.TestCase):
 
 def test_empty_gives_empty(self):
-fun_ast_lst = sut.func_map_list_src_to_ast([])
+fun_ast_lst = sut.extdef_map_list_src_to_ast([])
 self.assertFalse(fun_ast_lst)
 
 def test_sources_to_asts(self):
 fun_src_lst = ['c:@F@f1#I# ' + os.path.join(os.sep + 'path', 'f1.c'),
'c:@F@f2#I# ' + os.path.join(os.sep + 'path', 'f2.c')]
-fun_ast_lst = sut.func_map_list_src_to_ast(fun_src_lst)
+fun_ast_lst = sut.extdef_map_list_src_to_ast(fun_src_lst)
 self.assertTrue('c:@F@f1#I# ' +
 os.path.join('ast', 'path', 'f1.c.ast')
 in fun_ast_lst)
@@ -408,7 +408,7 @@
 
 def test_spaces_handled(self):
 fun_src_lst = ['c:@F@f1#I# ' +

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2018-06-04 Thread Rafael Stahl via Phabricator via cfe-commits 
r.stahl added a comment.

In https://reviews.llvm.org/D46421#1119098, @xazax.hun wrote:

> Sorry for the limited activity. Unfortunately, I have very little time 
> reviewing patches lately.


Thanks for getting around to it!

> I think we need to answer the following questions:
> 
> - Does this change affect the analysis of the constructors of global objects? 
> If so, how?

The intention was to only allow the analyzer to resolve constants of arrays, 
structs and integral values. It seems to me that construction would have to be 
symbolically executed at the correct point in time and cannot be retroactively 
looked up - as is done with the constants here.

> - Do we want to import non-const object's initializer expressions? The 
> analyzer might not end up using the value anyways.

It seems like a good idea to not do that, since non-const values are not used. 
It might become useful if we ever do some kind of straight line execution from 
static initialization to main.
However for structs it is enough if one of their fields is declared const.

> - How big can the index get with this modification for large projects?

That depends a lot on global usage of course. In LLVM for example there is 
little impact, but I don't have any numbers, it's taking too long. Taking care 
of the previous point helps here as well.

> Overall the direction looks good to me, this will be a very useful addition, 
> thanks for working on this.

I will prepare something to not store unnecessary entries in the index. I also 
noticed the AnalysisConsumer change has gone missing with my latest diff - will 
be fixed.


https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2018-06-01 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added a comment.

Sorry for the limited activity. Unfortunately, I have very little time 
reviewing patches lately.
I think we need to answer the following questions:

- Does this change affect the analysis of the constructors of global objects? 
If so, how?
- Do we want to import non-const object's initializer expressions? The analyzer 
might not end up using the value anyways.
- How big can the index get with this modification for large projects?

Overall the direction looks good to me, this will be a very useful addition, 
thanks for working on this.


https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2018-05-15 Thread Rafael Stahl via Phabricator via cfe-commits 
r.stahl added inline comments.



Comment at: include/clang/CrossTU/CrossTranslationUnit.h:114
+  llvm::Expected
+  getCrossTUDefinition(const VarDecl *VD, StringRef CrossTUDir,
+   StringRef IndexName);

xazax.hun wrote:
> I wonder if we need these overloads. Maybe having only the templated version 
> and having a static assert for the supported types is better? Asking the 
> other reviewers as well.
They are not required, but I thought they make the interface more clear and 
help prevent implementation in headers.



Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:354
+if (!VD->hasExternalStorage() ||
+VD->getAnyInitializer() != nullptr)
+  return true;

xazax.hun wrote:
> Will this work for zero initialized globals (i.e. not having an initializer 
> expression) that are defined in the current translation unit? It would be 
> great to have a test case for that.
If a variable has a definition in the current TU, it may not have another one 
else where, but you're correct that this case will continue here. It will 
however only result in a failed lookup.

Similar but backwards: If an extern var is defined in another TU with global 
zero init.

Overall I'd say that default initializing a constant to zero is pretty uncommon 
(and invalid in C++), so that in my opinion it's fine to not support it for 
now. It seems like there is no transparent way to get the initializer including 
that case or am I missing something?


https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2018-05-15 Thread Rafael Stahl via Phabricator via cfe-commits 
r.stahl updated this revision to Diff 146789.
r.stahl marked 3 inline comments as done.
r.stahl edited the summary of this revision.
r.stahl added a comment.
Herald added subscribers: whisperity, mgorny.

I looked through the original patches and found quite a few more occurrences of 
"function map" and renamed them - including the tool "clang-func-mapping". 
There is one comment "by using the 'clang-extdef-mapping' packaged with Xcode 
(on OS X)". Is this implicit from the install targets that the tool name 
changed or do I have to inform someone?


https://reviews.llvm.org/D46421

Files:
  include/clang/Basic/DiagnosticCrossTUKinds.td
  include/clang/CrossTU/CrossTranslationUnit.h
  include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  lib/CrossTU/CrossTranslationUnit.cpp
  lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
  test/Analysis/Inputs/ctu-other.cpp
  test/Analysis/Inputs/externalDefMap.txt
  test/Analysis/Inputs/externalFnMap.txt
  test/Analysis/ctu-main.cpp
  test/Analysis/func-mapping-test.cpp
  test/CMakeLists.txt
  test/lit.cfg.py
  tools/CMakeLists.txt
  tools/clang-extdef-mapping/CMakeLists.txt
  tools/clang-extdef-mapping/ClangExtDefMapGen.cpp
  tools/clang-func-mapping/CMakeLists.txt
  tools/clang-func-mapping/ClangFnMapGen.cpp
  tools/scan-build-py/README.md
  tools/scan-build-py/libscanbuild/__init__.py
  tools/scan-build-py/libscanbuild/analyze.py
  tools/scan-build-py/libscanbuild/arguments.py
  tools/scan-build-py/libscanbuild/clang.py
  tools/scan-build-py/tests/unit/test_analyze.py
  tools/scan-build-py/tests/unit/test_clang.py

Index: tools/scan-build-py/tests/unit/test_clang.py
===
--- tools/scan-build-py/tests/unit/test_clang.py
+++ tools/scan-build-py/tests/unit/test_clang.py
@@ -96,7 +96,7 @@
 
 class ClangIsCtuCapableTest(unittest.TestCase):
 def test_ctu_not_found(self):
-is_ctu = sut.is_ctu_capable('not-found-clang-func-mapping')
+is_ctu = sut.is_ctu_capable('not-found-clang-extdef-mapping')
 self.assertFalse(is_ctu)
 
 
Index: tools/scan-build-py/tests/unit/test_analyze.py
===
--- tools/scan-build-py/tests/unit/test_analyze.py
+++ tools/scan-build-py/tests/unit/test_analyze.py
@@ -349,14 +349,14 @@
 class MergeCtuMapTest(unittest.TestCase):
 
 def test_no_map_gives_empty(self):
-pairs = sut.create_global_ctu_function_map([])
+pairs = sut.create_global_ctu_extdef_map([])
 self.assertFalse(pairs)
 
 def test_multiple_maps_merged(self):
 concat_map = ['c:@F@fun1#I# ast/fun1.c.ast',
   'c:@F@fun2#I# ast/fun2.c.ast',
   'c:@F@fun3#I# ast/fun3.c.ast']
-pairs = sut.create_global_ctu_function_map(concat_map)
+pairs = sut.create_global_ctu_extdef_map(concat_map)
 self.assertTrue(('c:@F@fun1#I#', 'ast/fun1.c.ast') in pairs)
 self.assertTrue(('c:@F@fun2#I#', 'ast/fun2.c.ast') in pairs)
 self.assertTrue(('c:@F@fun3#I#', 'ast/fun3.c.ast') in pairs)
@@ -366,7 +366,7 @@
 concat_map = ['c:@F@fun1#I# ast/fun1.c.ast',
   'c:@F@fun2#I# ast/fun2.c.ast',
   'c:@F@fun1#I# ast/fun7.c.ast']
-pairs = sut.create_global_ctu_function_map(concat_map)
+pairs = sut.create_global_ctu_extdef_map(concat_map)
 self.assertFalse(('c:@F@fun1#I#', 'ast/fun1.c.ast') in pairs)
 self.assertFalse(('c:@F@fun1#I#', 'ast/fun7.c.ast') in pairs)
 self.assertTrue(('c:@F@fun2#I#', 'ast/fun2.c.ast') in pairs)
@@ -376,28 +376,28 @@
 concat_map = ['c:@F@fun1#I# ast/fun1.c.ast',
   'c:@F@fun2#I# ast/fun2.c.ast',
   'c:@F@fun1#I# ast/fun1.c.ast']
-pairs = sut.create_global_ctu_function_map(concat_map)
+pairs = sut.create_global_ctu_extdef_map(concat_map)
 self.assertTrue(('c:@F@fun1#I#', 'ast/fun1.c.ast') in pairs)
 self.assertTrue(('c:@F@fun2#I#', 'ast/fun2.c.ast') in pairs)
 self.assertEqual(2, len(pairs))
 
 def test_space_handled_in_source(self):
 concat_map = ['c:@F@fun1#I# ast/f un.c.ast']
-pairs = sut.create_global_ctu_function_map(concat_map)
+pairs = sut.create_global_ctu_extdef_map(concat_map)
 self.assertTrue(('c:@F@fun1#I#', 'ast/f un.c.ast') in pairs)
 self.assertEqual(1, len(pairs))
 
 
-class FuncMapSrcToAstTest(unittest.TestCase):
+class ExtdefMapSrcToAstTest(unittest.TestCase):
 
 def test_empty_gives_empty(self):
-fun_ast_lst = sut.func_map_list_src_to_ast([])
+fun_ast_lst = sut.extdef_map_list_src_to_ast([])
 self.assertFalse(fun_ast_lst)
 
 def test_sources_to_asts(self):
 fun_src_lst = ['c:@F@f1#I# ' + os.path.join(os.sep + 'path', 'f1.c'),
'c:@F@f2#I# ' + os.path.join(os.sep + 'path', 'f2.c')]
-fun_ast_lst = sut.func_map_list_src_to_ast(fun_src_lst)

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2018-05-15 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added inline comments.



Comment at: include/clang/CrossTU/CrossTranslationUnit.h:117
 
-  /// This function loads a function definition from an external AST
-  ///file.
+  /// \brief This function loads a definition from an external AST file.
   ///

Adding briefs are unnecessary, see https://reviews.llvm.org/rL331834


https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2018-05-14 Thread Gábor Horváth via Phabricator via cfe-commits 
xazax.hun added a comment.

The analyzer can only reason about const variables this way, right? Maybe we 
should only import the initializers for such variables to have better 
performance? What do you think?

Also, I wonder what happens with user-defined classes. Will the analyzer 
evaluate the constructor if the variable is imported?




Comment at: include/clang/CrossTU/CrossTranslationUnit.h:114
+  llvm::Expected
+  getCrossTUDefinition(const VarDecl *VD, StringRef CrossTUDir,
+   StringRef IndexName);

I wonder if we need these overloads. Maybe having only the templated version 
and having a static assert for the supported types is better? Asking the other 
reviewers as well.



Comment at: include/clang/CrossTU/CrossTranslationUnit.h:138
   llvm::Expected importDefinition(const FunctionDecl 
*FD);
+  llvm::Expected importDefinition(const VarDecl *VD);
 

Same question as above.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:114
 
+bool HasDefinition(const FunctionDecl *D, const FunctionDecl *) {
+  return D->hasBody(DefD);

Functions should start with lowercase letter. Maybe these could be static?



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:143
+CrossTranslationUnitContext::findDefInDeclContext(const DeclContext *DC,
+  StringRef LookupFnName) {
   assert(DC && "Declaration Context must not be null");

Fn in the name refers to a function, maybe that could be changed as well.



Comment at: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:354
+if (!VD->hasExternalStorage() ||
+VD->getAnyInitializer() != nullptr)
+  return true;

Will this work for zero initialized globals (i.e. not having an initializer 
expression) that are defined in the current translation unit? It would be great 
to have a test case for that.


https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2018-05-14 Thread Rafael Stahl via Phabricator via cfe-commits 
r.stahl updated this revision to Diff 146609.
r.stahl marked an inline comment as done.
r.stahl added a comment.

- added tests
- updated doc and var naming
- addressed review comment


https://reviews.llvm.org/D46421

Files:
  include/clang/CrossTU/CrossTranslationUnit.h
  lib/CrossTU/CrossTranslationUnit.cpp
  lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  test/Analysis/Inputs/ctu-other.cpp
  test/Analysis/Inputs/externalFnMap.txt
  test/Analysis/ctu-main.cpp
  test/Analysis/func-mapping-test.cpp

Index: test/Analysis/func-mapping-test.cpp
===
--- test/Analysis/func-mapping-test.cpp
+++ test/Analysis/func-mapping-test.cpp
@@ -3,5 +3,16 @@
 int f(int) {
   return 0;
 }
+// CHECK-DAG: c:@F@f#I#
 
-// CHECK: c:@F@f#I#
+int x = 5;
+// CHECK-DAG: c:@x
+
+struct S {
+  int a;
+};
+S s = {.a = 2};
+// CHECK-DAG: c:@s
+
+int arr[5] = { 0, 1 };
+// CHECK-DAG: c:@arr
Index: test/Analysis/ctu-main.cpp
===
--- test/Analysis/ctu-main.cpp
+++ test/Analysis/ctu-main.cpp
@@ -42,6 +42,15 @@
 
 int fun_using_anon_struct(int);
 
+extern const int extInt;
+namespace intns {
+extern const int extInt;
+}
+struct S {
+  int a;
+};
+extern const S extS;
+
 int main() {
   clang_analyzer_eval(f(3) == 2); // expected-warning{{TRUE}}
   clang_analyzer_eval(f(4) == 3); // expected-warning{{TRUE}}
@@ -58,4 +67,8 @@
 
   clang_analyzer_eval(chns::chf1(4) == 12); // expected-warning{{TRUE}}
   clang_analyzer_eval(fun_using_anon_struct(8) == 8); // expected-warning{{TRUE}}
+
+  clang_analyzer_eval(extInt == 2); // expected-warning{{TRUE}}
+  clang_analyzer_eval(intns::extInt == 3); // expected-warning{{TRUE}}
+  clang_analyzer_eval(extS.a == 4); // expected-warning{{TRUE}}
 }
Index: test/Analysis/Inputs/externalFnMap.txt
===
--- test/Analysis/Inputs/externalFnMap.txt
+++ test/Analysis/Inputs/externalFnMap.txt
@@ -12,3 +12,6 @@
 c:@N@chns@S@chcls@F@chf4#I# ctu-chain.cpp.ast
 c:@N@chns@F@chf2#I# ctu-chain.cpp.ast
 c:@F@fun_using_anon_struct#I# ctu-other.cpp.ast
+c:@extInt ctu-other.cpp.ast
+c:@N@intns@extInt ctu-other.cpp.ast
+c:@extS ctu-other.cpp.ast
Index: test/Analysis/Inputs/ctu-other.cpp
===
--- test/Analysis/Inputs/ctu-other.cpp
+++ test/Analysis/Inputs/ctu-other.cpp
@@ -68,3 +68,12 @@
 
 typedef struct { int n; } Anonymous;
 int fun_using_anon_struct(int n) { Anonymous anon; anon.n = n; return anon.n; }
+
+extern const int extInt = 2;
+namespace intns {
+extern const int extInt = 3;
+}
+struct S {
+  int a;
+};
+extern const S extS = {.a = 4};
Index: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -346,6 +346,27 @@
 return true;
   }
 
+  bool VisitVarDecl(VarDecl *VD) {
+if (!Opts->naiveCTUEnabled())
+  return true;
+
+if (!VD->hasExternalStorage() ||
+VD->getAnyInitializer() != nullptr)
+  return true;
+
+llvm::Expected CTUDeclOrError =
+  CTU.getCrossTUDefinition(VD, Opts->getCTUDir(), Opts->getCTUIndexName());
+
+if (!CTUDeclOrError) {
+  handleAllErrors(CTUDeclOrError.takeError(),
+  [&](const cross_tu::IndexError ) {
+CTU.emitCrossTUDiagnostics(IE);
+  });
+}
+
+return true;
+  }
+
   bool VisitFunctionDecl(FunctionDecl *FD) {
 IdentifierInfo *II = FD->getIdentifier();
 if (II && II->getName().startswith("__inline"))
Index: lib/CrossTU/CrossTranslationUnit.cpp
===
--- lib/CrossTU/CrossTranslationUnit.cpp
+++ lib/CrossTU/CrossTranslationUnit.cpp
@@ -87,14 +87,14 @@
 const size_t Pos = Line.find(" ");
 if (Pos > 0 && Pos != std::string::npos) {
   StringRef LineRef{Line};
-  StringRef FunctionLookupName = LineRef.substr(0, Pos);
-  if (Result.count(FunctionLookupName))
+  StringRef LookupName = LineRef.substr(0, Pos);
+  if (Result.count(LookupName))
 return llvm::make_error(
 index_error_code::multiple_definitions, IndexPath.str(), LineNo);
   StringRef FileName = LineRef.substr(Pos + 1);
   SmallString<256> FilePath = CrossTUDir;
   llvm::sys::path::append(FilePath, FileName);
-  Result[FunctionLookupName] = FilePath.str().str();
+  Result[LookupName] = FilePath.str().str();
 } else
   return llvm::make_error(
   index_error_code::invalid_index_format, IndexPath.str(), LineNo);
@@ -111,52 +111,65 @@
   return Result.str();
 }
 
+bool HasDefinition(const FunctionDecl *D, const FunctionDecl *) {
+  return D->hasBody(DefD);
+}
+bool HasDefinition(const VarDecl *D, const VarDecl *) {
+  return D->getAnyInitializer(DefD) !=

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2018-05-04 Thread Aleksei Sidorin via Phabricator via cfe-commits 
a.sidorin added a comment.

Hi Rafael! I like the change.




Comment at: lib/CrossTU/CrossTranslationUnit.cpp:276
+  ASTImporter  = getOrCreateASTImporter(D->getASTContext());
+  auto *ToDecl = cast(Importer.Import(const_cast(D)));
+  assert(HasDefinition(ToDecl));

Are we always sure that Import() returns non-null value of appropriate type? If 
no, we have to check the result after applying dyn_cast_or_null.


Repository:
  rC Clang

https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2018-05-04 Thread Rafael Stahl via Phabricator via cfe-commits 
r.stahl added a comment.

Tests and doc fixes pending. First I'm interested in your thoughts to this 
change.

It allows to bind more symbolic values to constants if they have been defined 
and initialized in another TU:

use.c:

  extern int * const p;
  extern struct S * const s;

def.c:

  int * const p = 0;
  struct S * const s = { /* complex init */ };


Repository:
  rC Clang

https://reviews.llvm.org/D46421



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

[PATCH] D46421: [analyzer][CrossTU] Extend CTU to VarDecls with initializer

2018-05-04 Thread Rafael Stahl via Phabricator via cfe-commits 
r.stahl created this revision.
r.stahl added reviewers: NoQ, dcoughlin, xazax.hun, george.karpenkov.
Herald added subscribers: cfe-commits, a.sidorin, rnkovacs, szepet.

The existing CTU mechanism imports FunctionDecls where the definition is 
available in another TU. This patch extends that to VarDecls, to bind more 
constants.

- Add VarDecl importing functionality to CrossTranslationUnitContext
- Import Decls while traversing them in AnalysisConsumer
- Add VarDecls to CTU external mappings generator


Repository:
  rC Clang

https://reviews.llvm.org/D46421

Files:
  include/clang/CrossTU/CrossTranslationUnit.h
  lib/CrossTU/CrossTranslationUnit.cpp
  lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
  tools/clang-func-mapping/ClangFnMapGen.cpp

Index: tools/clang-func-mapping/ClangFnMapGen.cpp
===
--- tools/clang-func-mapping/ClangFnMapGen.cpp
+++ tools/clang-func-mapping/ClangFnMapGen.cpp
@@ -54,6 +54,7 @@
 
 private:
   void handleDecl(const Decl *D);
+  void addIfInMain(const DeclaratorDecl *DD, SourceLocation defStart);
 
   ASTContext 
   llvm::StringMap Index;
@@ -65,35 +66,42 @@
 return;
 
   if (const auto *FD = dyn_cast(D)) {
-if (FD->isThisDeclarationADefinition()) {
-  if (const Stmt *Body = FD->getBody()) {
-std::string LookupName = CrossTranslationUnitContext::getLookupName(FD);
-const SourceManager  = Ctx.getSourceManager();
-if (CurrentFileName.empty()) {
-  CurrentFileName =
-  SM.getFileEntryForID(SM.getMainFileID())->tryGetRealPathName();
-  if (CurrentFileName.empty())
-CurrentFileName = "invalid_file";
-}
-
-switch (FD->getLinkageInternal()) {
-case ExternalLinkage:
-case VisibleNoLinkage:
-case UniqueExternalLinkage:
-  if (SM.isInMainFile(Body->getLocStart()))
-Index[LookupName] = CurrentFileName;
-default:
-  break;
-}
-  }
-}
+if (FD->isThisDeclarationADefinition())
+  if (const Stmt *Body = FD->getBody())
+addIfInMain(FD, Body->getLocStart());
+  } else if (const auto *VD = dyn_cast(D)) {
+if (VD->hasInit())
+  if (const Expr *Init = VD->getInit())
+addIfInMain(VD, Init->getLocStart());
   }
 
   if (const auto *DC = dyn_cast(D))
 for (const Decl *D : DC->decls())
   handleDecl(D);
 }
 
+void MapFunctionNamesConsumer::addIfInMain(const DeclaratorDecl *DD,
+   SourceLocation defStart) {
+  std::string LookupName = CrossTranslationUnitContext::getLookupName(DD);
+  const SourceManager  = Ctx.getSourceManager();
+  if (CurrentFileName.empty()) {
+CurrentFileName =
+SM.getFileEntryForID(SM.getMainFileID())->tryGetRealPathName();
+if (CurrentFileName.empty())
+  CurrentFileName = "invalid_file";
+  }
+
+  switch (DD->getLinkageInternal()) {
+  case ExternalLinkage:
+  case VisibleNoLinkage:
+  case UniqueExternalLinkage:
+if (SM.isInMainFile(defStart))
+  Index[LookupName] = CurrentFileName;
+  default:
+break;
+  }
+}
+
 class MapFunctionNamesAction : public ASTFrontendAction {
 protected:
   std::unique_ptr CreateASTConsumer(CompilerInstance ,
Index: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
===
--- lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
+++ lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp
@@ -346,6 +346,27 @@
 return true;
   }
 
+  bool VisitVarDecl(VarDecl *VD) {
+if (!Opts->naiveCTUEnabled())
+  return true;
+
+if (!VD->hasExternalStorage() ||
+VD->getAnyInitializer() != nullptr)
+  return true;
+
+llvm::Expected CTUDeclOrError =
+  CTU.getCrossTUDefinition(VD, Opts->getCTUDir(), Opts->getCTUIndexName());
+
+if (!CTUDeclOrError) {
+  handleAllErrors(CTUDeclOrError.takeError(),
+  [&](const cross_tu::IndexError ) {
+CTU.emitCrossTUDiagnostics(IE);
+  });
+}
+
+return true;
+  }
+
   bool VisitFunctionDecl(FunctionDecl *FD) {
 IdentifierInfo *II = FD->getIdentifier();
 if (II && II->getName().startswith("__inline"))
Index: lib/CrossTU/CrossTranslationUnit.cpp
===
--- lib/CrossTU/CrossTranslationUnit.cpp
+++ lib/CrossTU/CrossTranslationUnit.cpp
@@ -111,52 +111,65 @@
   return Result.str();
 }
 
+bool HasDefinition(const FunctionDecl *D, const FunctionDecl *) {
+  return D->hasBody(DefD);
+}
+bool HasDefinition(const VarDecl *D, const VarDecl *) {
+  return D->getAnyInitializer(DefD) != nullptr;
+}
+template  bool HasDefinition(const T *D) {
+  const T *Unused;
+  return HasDefinition(D, Unused);
+}
+
 CrossTranslationUnitContext::CrossTranslationUnitContext(CompilerInstance )
 : CI(CI), Context(CI.getASTContext()) {}