[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In https://reviews.llvm.org/D34512#890537, @r.stahl wrote:

> In https://reviews.llvm.org/D34512#877643, @xazax.hun wrote:
>
> > - Unittests now creates temporary files at the correct location
> > - Temporary files are also cleaned up when the process is terminated
> > - Absolute paths are handled correctly by the library
>
>
> I think there is an issue with this change.
>
> First, the function map generation writes absolute paths to the map file. Now 
> when the `parseCrossTUIndex` function parses it, it will no longer prepend 
> the paths with the CTUDir and therefore expect the precompiled AST files at 
> the exact path of the source file. This seems like a bad requirement, because 
> the user would have to overwrite his source files.
>
> If I understand your intention correctly, a fix would be to replace the 
> `is_absolute` check with a check for `CTUDir == ""` in the 
> `parseCrossTUIndex` function.


Good catch, that is right!


Repository:
  rL LLVM

https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Rafael Stahl via Phabricator via cfe-commits]

r.stahl added a comment.

In https://reviews.llvm.org/D34512#877643, @xazax.hun wrote:

> - Unittests now creates temporary files at the correct location
> - Temporary files are also cleaned up when the process is terminated
> - Absolute paths are handled correctly by the library


I think there is an issue with this change.

First, the function map generation writes absolute paths to the map file. Now 
when the `parseCrossTUIndex` function parses it, it will no longer prepend the 
paths with the CTUDir and therefore expect the precompiled AST files at the 
exact path of the source file. This seems like a bad requirement, because the 
user would have to overwrite his source files.

If I understand your intention correctly, a fix would be to replace the 
`is_absolute` check with a check for `CTUDir == ""` in the `parseCrossTUIndex` 
function.


Repository:
  rL LLVM

https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL313975: Add Cross Translation Unit support library (authored 
by xazax).

Changed prior to commit:
  https://reviews.llvm.org/D34512?vs=116195=116327#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D34512

Files:
  cfe/trunk/include/clang/Basic/AllDiagnostics.h
  cfe/trunk/include/clang/Basic/CMakeLists.txt
  cfe/trunk/include/clang/Basic/Diagnostic.td
  cfe/trunk/include/clang/Basic/DiagnosticCrossTUKinds.td
  cfe/trunk/include/clang/Basic/DiagnosticIDs.h
  cfe/trunk/include/clang/CrossTU/CrossTUDiagnostic.h
  cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h
  cfe/trunk/lib/AST/ASTImporter.cpp
  cfe/trunk/lib/Basic/DiagnosticIDs.cpp
  cfe/trunk/lib/CMakeLists.txt
  cfe/trunk/lib/CrossTU/CMakeLists.txt
  cfe/trunk/lib/CrossTU/CrossTranslationUnit.cpp
  cfe/trunk/test/Analysis/func-mapping-test.cpp
  cfe/trunk/test/CMakeLists.txt
  cfe/trunk/test/lit.cfg.py
  cfe/trunk/tools/CMakeLists.txt
  cfe/trunk/tools/clang-func-mapping/CMakeLists.txt
  cfe/trunk/tools/clang-func-mapping/ClangFnMapGen.cpp
  cfe/trunk/tools/diagtool/DiagnosticNames.cpp
  cfe/trunk/unittests/CMakeLists.txt
  cfe/trunk/unittests/CrossTU/CMakeLists.txt
  cfe/trunk/unittests/CrossTU/CrossTranslationUnitTest.cpp

Index: cfe/trunk/include/clang/Basic/CMakeLists.txt
===
--- cfe/trunk/include/clang/Basic/CMakeLists.txt
+++ cfe/trunk/include/clang/Basic/CMakeLists.txt
@@ -9,6 +9,7 @@
 clang_diag_gen(AST)
 clang_diag_gen(Comment)
 clang_diag_gen(Common)
+clang_diag_gen(CrossTU)
 clang_diag_gen(Driver)
 clang_diag_gen(Frontend)
 clang_diag_gen(Lex)
Index: cfe/trunk/include/clang/Basic/DiagnosticIDs.h
===
--- cfe/trunk/include/clang/Basic/DiagnosticIDs.h
+++ cfe/trunk/include/clang/Basic/DiagnosticIDs.h
@@ -36,6 +36,7 @@
   DIAG_SIZE_PARSE =  500,
   DIAG_SIZE_AST   =  110,
   DIAG_SIZE_COMMENT   =  100,
+  DIAG_SIZE_CROSSTU   =  100,
   DIAG_SIZE_SEMA  = 3500,
   DIAG_SIZE_ANALYSIS  =  100
 };
@@ -49,7 +50,8 @@
   DIAG_START_PARSE = DIAG_START_LEX   + DIAG_SIZE_LEX,
   DIAG_START_AST   = DIAG_START_PARSE + DIAG_SIZE_PARSE,
   DIAG_START_COMMENT   = DIAG_START_AST   + DIAG_SIZE_AST,
-  DIAG_START_SEMA  = DIAG_START_COMMENT   + DIAG_SIZE_COMMENT,
+  DIAG_START_CROSSTU   = DIAG_START_COMMENT   + DIAG_SIZE_CROSSTU,
+  DIAG_START_SEMA  = DIAG_START_CROSSTU   + DIAG_SIZE_COMMENT,
   DIAG_START_ANALYSIS  = DIAG_START_SEMA  + DIAG_SIZE_SEMA,
   DIAG_UPPER_LIMIT = DIAG_START_ANALYSIS  + DIAG_SIZE_ANALYSIS
 };
Index: cfe/trunk/include/clang/Basic/DiagnosticCrossTUKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticCrossTUKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticCrossTUKinds.td
@@ -0,0 +1,18 @@
+//==--- DiagnosticCrossTUKinds.td - Cross Translation Unit diagnostics ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+let Component = "CrossTU" in {
+
+def err_fnmap_parsing : Error<
+  "error parsing index file: '%0' line: %1 'UniqueID filename' format "
+  "expected">;
+
+def err_multiple_def_index : Error<
+  "multiple definitions are found for the same key in index ">;
+}
Index: cfe/trunk/include/clang/Basic/Diagnostic.td
===
--- cfe/trunk/include/clang/Basic/Diagnostic.td
+++ cfe/trunk/include/clang/Basic/Diagnostic.td
@@ -133,6 +133,7 @@
 include "DiagnosticAnalysisKinds.td"
 include "DiagnosticCommentKinds.td"
 include "DiagnosticCommonKinds.td"
+include "DiagnosticCrossTUKinds.td"
 include "DiagnosticDriverKinds.td"
 include "DiagnosticFrontendKinds.td"
 include "DiagnosticLexKinds.td"
Index: cfe/trunk/include/clang/Basic/AllDiagnostics.h
===
--- cfe/trunk/include/clang/Basic/AllDiagnostics.h
+++ cfe/trunk/include/clang/Basic/AllDiagnostics.h
@@ -18,6 +18,7 @@
 #include "clang/AST/ASTDiagnostic.h"
 #include "clang/AST/CommentDiagnostic.h"
 #include "clang/Analysis/AnalysisDiagnostic.h"
+#include "clang/CrossTU/CrossTUDiagnostic.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
 #include "clang/Lex/LexDiagnostic.h"
Index: cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h
===
--- cfe/trunk/include/clang/CrossTU/CrossTranslationUnit.h
+++ 

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Devin Coughlin via Phabricator via cfe-commits]

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

> But I also think it wouldn't be good to block this until ClanD indexing 
> reaching a mature state.

I agree 100%.

> All in all, we are willing to reuse as much of ClangD indexing as we can in 
> the future, but I think it is also more aligned with the LLVM Developer 
> Policy to have something working committed and do the development in the repo 
> rather than working separately on a fork.

This sounds good to me. I just wanted to make sure that you were onboard with 
changing this index to use the common indexing infrastructure in the future. 
(Assuming it makes sense to do so, of course).


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 116195.
xazax.hun added a comment.

- Unittests now creates temporary files at the correct location
- Temporary files are also cleaned up when the process is terminated
- Absolute paths are handled correctly by the library


https://reviews.llvm.org/D34512

Files:
  include/clang/Basic/AllDiagnostics.h
  include/clang/Basic/CMakeLists.txt
  include/clang/Basic/Diagnostic.td
  include/clang/Basic/DiagnosticCrossTUKinds.td
  include/clang/Basic/DiagnosticIDs.h
  include/clang/CrossTU/CrossTUDiagnostic.h
  include/clang/CrossTU/CrossTranslationUnit.h
  lib/AST/ASTImporter.cpp
  lib/Basic/DiagnosticIDs.cpp
  lib/CMakeLists.txt
  lib/CrossTU/CMakeLists.txt
  lib/CrossTU/CrossTranslationUnit.cpp
  test/Analysis/func-mapping-test.cpp
  test/CMakeLists.txt
  test/lit.cfg
  tools/CMakeLists.txt
  tools/clang-func-mapping/CMakeLists.txt
  tools/clang-func-mapping/ClangFnMapGen.cpp
  tools/diagtool/DiagnosticNames.cpp
  unittests/CMakeLists.txt
  unittests/CrossTU/CMakeLists.txt
  unittests/CrossTU/CrossTranslationUnitTest.cpp

Index: unittests/CrossTU/CrossTranslationUnitTest.cpp
===
--- /dev/null
+++ unittests/CrossTU/CrossTranslationUnitTest.cpp
@@ -0,0 +1,138 @@
+//===- unittest/Tooling/CrossTranslationUnitTest.cpp - Tooling unit tests -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/CrossTU/CrossTranslationUnit.h"
+#include "clang/AST/ASTConsumer.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Config/llvm-config.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/ToolOutputFile.h"
+#include "gtest/gtest.h"
+#include 
+
+namespace clang {
+namespace cross_tu {
+
+namespace {
+
+class CTUASTConsumer : public clang::ASTConsumer {
+public:
+  explicit CTUASTConsumer(clang::CompilerInstance , bool *Success)
+  : CTU(CI), Success(Success) {}
+
+  void HandleTranslationUnit(ASTContext ) {
+const TranslationUnitDecl *TU = Ctx.getTranslationUnitDecl();
+const FunctionDecl *FD = nullptr;
+for (const Decl *D : TU->decls()) {
+  FD = dyn_cast(D);
+  if (FD && FD->getName() == "f")
+break;
+}
+assert(FD && FD->getName() == "f");
+bool OrigFDHasBody = FD->hasBody();
+
+// Prepare the index file and the AST file.
+int ASTFD;
+llvm::SmallString<256> ASTFileName;
+ASSERT_FALSE(
+llvm::sys::fs::createTemporaryFile("f_ast", "ast", ASTFD, ASTFileName));
+llvm::tool_output_file ASTFile(ASTFileName, ASTFD);
+
+int IndexFD;
+llvm::SmallString<256> IndexFileName;
+ASSERT_FALSE(llvm::sys::fs::createTemporaryFile("index", "txt", IndexFD,
+IndexFileName));
+llvm::tool_output_file IndexFile(IndexFileName, IndexFD);
+IndexFile.os() << "c:@F@f#I# " << ASTFileName << "\n";
+IndexFile.os().flush();
+EXPECT_TRUE(llvm::sys::fs::exists(IndexFileName));
+
+StringRef SourceText = "int f(int) { return 0; }\n";
+// This file must exist since the saved ASTFile will reference it.
+int SourceFD;
+llvm::SmallString<256> SourceFileName;
+ASSERT_FALSE(llvm::sys::fs::createTemporaryFile("input", "cpp", SourceFD,
+SourceFileName));
+llvm::tool_output_file SourceFile(SourceFileName, SourceFD);
+SourceFile.os() << SourceText;
+SourceFile.os().flush();
+EXPECT_TRUE(llvm::sys::fs::exists(SourceFileName));
+
+std::unique_ptr ASTWithDefinition =
+tooling::buildASTFromCode(SourceText, SourceFileName);
+ASTWithDefinition->Save(ASTFileName.str());
+EXPECT_TRUE(llvm::sys::fs::exists(ASTFileName));
+
+// Load the definition from the AST file.
+llvm::Expected NewFDorError =
+CTU.getCrossTUDefinition(FD, "", IndexFileName);
+EXPECT_TRUE((bool)NewFDorError);
+const FunctionDecl *NewFD = *NewFDorError;
+
+*Success = NewFD && NewFD->hasBody() && !OrigFDHasBody;
+  }
+
+private:
+  CrossTranslationUnitContext CTU;
+  bool *Success;
+};
+
+class CTUAction : public clang::ASTFrontendAction {
+public:
+  CTUAction(bool *Success) : Success(Success) {}
+
+protected:
+  std::unique_ptr
+  CreateASTConsumer(clang::CompilerInstance , StringRef) override {
+return llvm::make_unique(CI, Success);
+  }
+
+private:
+  bool *Success;
+};
+
+} // end namespace
+
+TEST(CrossTranslationUnit, CanLoadFunctionDefinition) {
+  bool Success = false;
+  EXPECT_TRUE(tooling::runToolOnCode(new CTUAction(), "int f(int);"));
+  EXPECT_TRUE(Success);
+}
+
+TEST(CrossTranslationUnit, IndexFormatCanBeParsed) {
+  llvm::StringMap Index;
+  Index["a"] = "b";
+  Index["c"] = "d";
+  

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In https://reviews.llvm.org/D34512#877032, @dcoughlin wrote:

> I'm OK with this going into the repo a is (although it is light on tests!), 
> as long as we have an agreement that you'll be OK with iteration on both the 
> interface and the implementation to handle real-world projects.


I agree that it could have more tests, but while the main features are already 
tested, more tests are also coming with the first usage of the library (the 
Static Analyzer). We are also willing to add new tests as the interface/library 
evolves.

> More specifically, for this to work well on large/complicated projects we'll 
> need to:
> 
> - Move conflict resolution from index generation where it is now to query 
> time so that clients can pick the best implementation of a function when 
> multiple are found. This is important for projects that have multiple 
> functions with the same name or that build the same file in multiple ways.

I agree. Right now the way we use this internally, the collisions are handled 
after index generation by an external tool (CodeChecker).

> - Change function lookup from needing to traverse over the entire AST.
> - Move away from a linear, flat-file index to something that can handle 
> larger projects more efficiently.
> 
>   For this last point, I suspect it will be good to adopt whatever Clangd 
> ends up using to support indexing. (There has been some discussion of this 
> relatively recently on the cfe-dev list.)

The index right now is a minimal implementation to make this functionality 
work. This part was never the bottleneck according to our measurements so far.  
It would be great to reuse the index format from ClangD in case it supports to 
generate lightweight indexes that only contain the data we need for analysis. 
(We only need an index of function definitions for the analyzer and nothing 
else. I suspect other clients also would not need other information.)

I think it would be a waste of effort to change the lookup strategy or move the 
collision handling mechanisms to index generation until we do not know how the 
index will work. But I also think it wouldn't be good to block this until ClanD 
indexing reaching a mature state.

All in all, we are willing to reuse as much of ClangD indexing as we can in the 
future, but I think it is also more aligned with the LLVM Developer Policy to 
have something working committed and do the development in the repo rather than 
working separately on a fork.


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added a comment.

I'm OK with this going into the repo a is (although it is light on tests!), as 
long as we have an agreement that you'll be OK with iteration on both the 
interface and the implementation to handle real-world projects.

More specifically, for this to work well on large/complicated projects we'll 
need to:

- Move conflict resolution from index generation where it is now to query time 
so that clients can pick the best implementation of a function when multiple 
are found. This is important for projects that have multiple functions with the 
same name or that build the same file in multiple ways.
- Change function lookup from needing to traverse over the entire AST.
- Move away from a linear, flat-file index to something that can handle larger 
projects more efficiently.

For this last point, I suspect it will be good to adopt whatever Clangd ends up 
using to support indexing. (There has been some discussion of this relatively 
recently on the cfe-dev list.)


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Any further reviews?
What are the criteria to accept this patch? Who or how many people should 
accept this?


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 114133.
xazax.hun marked 8 inline comments as done.
xazax.hun added a comment.

- Address review comments.


https://reviews.llvm.org/D34512

Files:
  include/clang/Basic/AllDiagnostics.h
  include/clang/Basic/CMakeLists.txt
  include/clang/Basic/Diagnostic.td
  include/clang/Basic/DiagnosticCrossTUKinds.td
  include/clang/Basic/DiagnosticIDs.h
  include/clang/CrossTU/CrossTUDiagnostic.h
  include/clang/CrossTU/CrossTranslationUnit.h
  lib/AST/ASTImporter.cpp
  lib/Basic/DiagnosticIDs.cpp
  lib/CMakeLists.txt
  lib/CrossTU/CMakeLists.txt
  lib/CrossTU/CrossTranslationUnit.cpp
  test/Analysis/func-mapping-test.cpp
  test/CMakeLists.txt
  test/lit.cfg
  tools/CMakeLists.txt
  tools/clang-func-mapping/CMakeLists.txt
  tools/clang-func-mapping/ClangFnMapGen.cpp
  tools/diagtool/DiagnosticNames.cpp
  unittests/CMakeLists.txt
  unittests/CrossTU/CMakeLists.txt
  unittests/CrossTU/CrossTranslationUnitTest.cpp

Index: unittests/CrossTU/CrossTranslationUnitTest.cpp
===
--- /dev/null
+++ unittests/CrossTU/CrossTranslationUnitTest.cpp
@@ -0,0 +1,124 @@
+//===- unittest/Tooling/CrossTranslationUnitTest.cpp - Tooling unit tests -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/CrossTU/CrossTranslationUnit.h"
+#include "clang/AST/ASTConsumer.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+
+namespace clang {
+namespace cross_tu {
+
+namespace {
+StringRef IndexFileName = "index.txt";
+StringRef ASTFileName = "f.ast";
+StringRef DefinitionFileName = "input.cc";
+
+class CTUASTConsumer : public clang::ASTConsumer {
+public:
+  explicit CTUASTConsumer(clang::CompilerInstance , bool *Success)
+  : CTU(CI), Success(Success) {}
+
+  void HandleTranslationUnit(ASTContext ) {
+const TranslationUnitDecl *TU = Ctx.getTranslationUnitDecl();
+const FunctionDecl *FD = nullptr;
+for (const Decl *D : TU->decls()) {
+  FD = dyn_cast(D);
+  if (FD && FD->getName() == "f")
+break;
+}
+assert(FD && FD->getName() == "f");
+bool OrigFDHasBody = FD->hasBody();
+
+// Prepare the index file and the AST file.
+std::error_code EC;
+llvm::raw_fd_ostream OS(IndexFileName, EC, llvm::sys::fs::F_Text);
+OS << "c:@F@f#I# " << ASTFileName << "\n";
+OS.flush();
+StringRef SourceText = "int f(int) { return 0; }\n";
+// This file must exist since the saved ASTFile will reference it.
+llvm::raw_fd_ostream OS2(DefinitionFileName, EC, llvm::sys::fs::F_Text);
+OS2 << SourceText;
+OS2.flush();
+std::unique_ptr ASTWithDefinition =
+tooling::buildASTFromCode(SourceText);
+ASTWithDefinition->Save(ASTFileName);
+
+// Load the definition from the AST file.
+llvm::Expected NewFDorError =
+CTU.getCrossTUDefinition(FD, ".", IndexFileName);
+assert(NewFDorError);
+const FunctionDecl *NewFD = *NewFDorError;
+
+*Success = NewFD && NewFD->hasBody() && !OrigFDHasBody;
+  }
+
+private:
+  CrossTranslationUnitContext CTU;
+  bool *Success;
+};
+
+class CTUAction : public clang::ASTFrontendAction {
+public:
+  CTUAction(bool *Success) : Success(Success) {}
+
+protected:
+  std::unique_ptr
+  CreateASTConsumer(clang::CompilerInstance , StringRef) override {
+return llvm::make_unique(CI, Success);
+  }
+
+private:
+  bool *Success;
+};
+
+} // end namespace
+
+TEST(CrossTranslationUnit, CanLoadFunctionDefinition) {
+  bool Success = false;
+  EXPECT_TRUE(tooling::runToolOnCode(new CTUAction(), "int f(int);"));
+  EXPECT_TRUE(Success);
+  EXPECT_TRUE(llvm::sys::fs::exists(IndexFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(IndexFileName));
+  EXPECT_TRUE(llvm::sys::fs::exists(ASTFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(ASTFileName));
+  EXPECT_TRUE(llvm::sys::fs::exists(DefinitionFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(DefinitionFileName));
+}
+
+TEST(CrossTranslationUnit, IndexFormatCanBeParsed) {
+  llvm::StringMap Index;
+  Index["a"] = "b";
+  Index["c"] = "d";
+  Index["e"] = "f";
+  std::string IndexText = createCrossTUIndexString(Index);
+  std::error_code EC;
+  llvm::raw_fd_ostream OS(IndexFileName, EC, llvm::sys::fs::F_Text);
+  OS << IndexText;
+  OS.flush();
+  EXPECT_TRUE(llvm::sys::fs::exists(IndexFileName));
+  llvm::Expected IndexOrErr =
+  parseCrossTUIndex(IndexFileName, "");
+  EXPECT_TRUE((bool)IndexOrErr);
+  llvm::StringMap ParsedIndex = IndexOrErr.get();
+  for (const auto  : Index) {
+EXPECT_TRUE(ParsedIndex.count(E.getKey()));
+EXPECT_EQ(ParsedIndex[E.getKey()], 

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:35
+namespace {
+// FIXME: This class is will be removed after the transition to llvm::Error.
+class IndexErrorCategory : public std::error_category {

dcoughlin wrote:
> Is this FIXME still relevant? What will need to be transitioned to 
> llvm::Error for it to be removed? Can you make the comment a bit more 
> specific so that future maintainers will know when/how to address the FIXME?
Unfortunately, it is. IndexError needs to implement `convertToErrorCode` 
because it is pure virtual in the base class, and that requires the existence 
of this class. So this can only be removed, once `convertToErrorCode` is pruned 
globally from the codebase.  I think all of such classes are marked with 
similar FIXMEs. 



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:194
+handleAllErrors(IndexOrErr.takeError(), [&](const IndexError ) {
+  (bool)PropagatedErr;
+  PropagatedErr =

dcoughlin wrote:
> What is this cast for? Is it to suppress an analyzer dead store false 
> positive? I thought we got all those!
The point was to avoid an assertion fail where an error was not checked. The 
reason, there is no really good way to propagate errors right now. But with 
this diagnostic emission moved to the client this is no longer required. 


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added a comment.

Thanks Gabor! Some additional comments in line.




Comment at: include/clang/CrossTU/CrossTranslationUnit.h:118
+  ///
+  /// \return Returns a map with the loaded AST Units and the declarations
+  /// with the definitions.

Is this comment correct? Does it return a map?



Comment at: include/clang/CrossTU/CrossTranslationUnit.h:128
+  /// \brief This function merges a definition from a separate AST Unit into
+  ///the current one.
+  ///

I found this comment confusing. Is 'Unit' the separate ASTUnit or it the 
current one?



Comment at: include/clang/CrossTU/CrossTranslationUnit.h:134
+
+  std::string getLookupName(const NamedDecl *ND);
+

If this is public API, it deserves a comment. If not, can we make it private?



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:35
+namespace {
+// FIXME: This class is will be removed after the transition to llvm::Error.
+class IndexErrorCategory : public std::error_category {

Is this FIXME still relevant? What will need to be transitioned to llvm::Error 
for it to be removed? Can you make the comment a bit more specific so that 
future maintainers will know when/how to address the FIXME?



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:194
+handleAllErrors(IndexOrErr.takeError(), [&](const IndexError ) {
+  (bool)PropagatedErr;
+  PropagatedErr =

What is this cast for? Is it to suppress an analyzer dead store false positive? 
I thought we got all those!



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:199
+  case index_error_code::missing_index_file:
+Context.getDiagnostics().Report(diag::err_fe_error_opening)
+<< ExternalFunctionMap

If I understand correctly, there is no way to call loadExternalAST() without 
the potential for a diagnostic showing up to the user. Can this diagnostic 
emission be moved to the client?



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:249
+CrossTranslationUnitContext::importDefinition(const FunctionDecl *FD,
+  ASTUnit *Unit) {
+  ASTImporter  = getOrCreateASTImporter(Unit->getASTContext());

Does this method need to take the unit? Can it just get the ASTContext from 
'FD'? If not, can we add an assertion that  FD has the required relationship to 
Unit? (And document it in the header doc.)



Comment at: tools/clang-func-mapping/ClangFnMapGen.cpp:56
+private:
+  std::string getLookupName(const FunctionDecl *FD);
+  void handleDecl(const Decl *D);

Is getLookupName() here used?



Comment at: tools/clang-func-mapping/ClangFnMapGen.cpp:72
+SmallString<128> LookupName;
+bool Res = index::generateUSRForDecl(D, LookupName);
+assert(!Res);

Should this instead reuse the logic for generating a lookup name from 
CrossTranslationUnitContext::getLookupName()? That way if we change how the 
lookup name is generated we only have to do it in one place.


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:82
+size_t Pos = Line.find(" ");
+StringRef LineRef{Line};
+if (Pos > 0 && Pos != std::string::npos) {

danielmarjamaki wrote:
> LineRef can be const
StringRef is an immutable reference to a string, I think it is not idiomatic in 
LLVM codebase to make it const. 



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:85
+  FunctionName = LineRef.substr(0, Pos);
+  if (Result.count(FunctionName))
+return llvm::make_error(

danielmarjamaki wrote:
> I would like to see some FunctionName validation. For instance "123" should 
> not be a valid function name.
This is not a real function name but an USR. I updated the name of the variable 
to reflect that this name is only used for lookup. 



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:89
+  FileName = LineRef.substr(Pos + 1);
+  SmallString<256> FilePath = CrossTUDir;
+  llvm::sys::path::append(FilePath, FileName);

danielmarjamaki wrote:
> Stupid question .. how will this work if the path is longer than 256 bytes?
If the path is shorter than 256 bytes it will be stored on stack, otherwise 
`SmallString` will allocate on the heap.



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

danielmarjamaki wrote:
> LookupFnName could be const right?
In LLVM we usually do not mark StringRefs as consts, they represent a const 
reference to a string.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:223
+assert(ToDecl->hasBody());
+assert(FD->hasBody() && "Functions already imported should have body.");
+return ToDecl;

danielmarjamaki wrote:
> sorry I am probably missing something here.. you first assert !FD->hasBody() 
> and here you assert FD->hasBody(). Is it changed in this function somewhere?
Yes, after the importer imports the new declaration with a body we should be 
able to find the body through the original declaration. The importer modifies 
the AST and the supporting data structures. 


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 113740.
xazax.hun marked 19 inline comments as done.
xazax.hun added a comment.

- Make the API capable of using custom lookup strategies for CTU
- Fix review comments


https://reviews.llvm.org/D34512

Files:
  include/clang/Basic/AllDiagnostics.h
  include/clang/Basic/CMakeLists.txt
  include/clang/Basic/Diagnostic.td
  include/clang/Basic/DiagnosticCrossTUKinds.td
  include/clang/Basic/DiagnosticIDs.h
  include/clang/CrossTU/CrossTUDiagnostic.h
  include/clang/CrossTU/CrossTranslationUnit.h
  lib/AST/ASTImporter.cpp
  lib/Basic/DiagnosticIDs.cpp
  lib/CMakeLists.txt
  lib/CrossTU/CMakeLists.txt
  lib/CrossTU/CrossTranslationUnit.cpp
  test/Analysis/func-mapping-test.cpp
  test/CMakeLists.txt
  test/lit.cfg
  tools/CMakeLists.txt
  tools/clang-func-mapping/CMakeLists.txt
  tools/clang-func-mapping/ClangFnMapGen.cpp
  tools/diagtool/DiagnosticNames.cpp
  unittests/CMakeLists.txt
  unittests/CrossTU/CMakeLists.txt
  unittests/CrossTU/CrossTranslationUnitTest.cpp

Index: unittests/CrossTU/CrossTranslationUnitTest.cpp
===
--- /dev/null
+++ unittests/CrossTU/CrossTranslationUnitTest.cpp
@@ -0,0 +1,124 @@
+//===- unittest/Tooling/CrossTranslationUnitTest.cpp - Tooling unit tests -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/CrossTU/CrossTranslationUnit.h"
+#include "clang/AST/ASTConsumer.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+
+namespace clang {
+namespace cross_tu {
+
+namespace {
+StringRef IndexFileName = "index.txt";
+StringRef ASTFileName = "f.ast";
+StringRef DefinitionFileName = "input.cc";
+
+class CTUASTConsumer : public clang::ASTConsumer {
+public:
+  explicit CTUASTConsumer(clang::CompilerInstance , bool *Success)
+  : CTU(CI), Success(Success) {}
+
+  void HandleTranslationUnit(ASTContext ) {
+const TranslationUnitDecl *TU = Ctx.getTranslationUnitDecl();
+const FunctionDecl *FD = nullptr;
+for (const Decl *D : TU->decls()) {
+  FD = dyn_cast(D);
+  if (FD && FD->getName() == "f")
+break;
+}
+assert(FD && FD->getName() == "f");
+bool OrigFDHasBody = FD->hasBody();
+
+// Prepare the index file and the AST file.
+std::error_code EC;
+llvm::raw_fd_ostream OS(IndexFileName, EC, llvm::sys::fs::F_Text);
+OS << "c:@F@f#I# " << ASTFileName << "\n";
+OS.flush();
+StringRef SourceText = "int f(int) { return 0; }\n";
+// This file must exist since the saved ASTFile will reference it.
+llvm::raw_fd_ostream OS2(DefinitionFileName, EC, llvm::sys::fs::F_Text);
+OS2 << SourceText;
+OS2.flush();
+std::unique_ptr ASTWithDefinition =
+tooling::buildASTFromCode(SourceText);
+ASTWithDefinition->Save(ASTFileName);
+
+// Load the definition from the AST file.
+llvm::Expected NewFDorError =
+CTU.getCrossTUDefinition(FD, ".", IndexFileName);
+assert(NewFDorError);
+const FunctionDecl *NewFD = *NewFDorError;
+
+*Success = NewFD && NewFD->hasBody() && !OrigFDHasBody;
+  }
+
+private:
+  CrossTranslationUnitContext CTU;
+  bool *Success;
+};
+
+class CTUAction : public clang::ASTFrontendAction {
+public:
+  CTUAction(bool *Success) : Success(Success) {}
+
+protected:
+  std::unique_ptr
+  CreateASTConsumer(clang::CompilerInstance , StringRef) override {
+return llvm::make_unique(CI, Success);
+  }
+
+private:
+  bool *Success;
+};
+
+} // end namespace
+
+TEST(CrossTranslationUnit, CanLoadFunctionDefinition) {
+  bool Success = false;
+  EXPECT_TRUE(tooling::runToolOnCode(new CTUAction(), "int f(int);"));
+  EXPECT_TRUE(Success);
+  EXPECT_TRUE(llvm::sys::fs::exists(IndexFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(IndexFileName));
+  EXPECT_TRUE(llvm::sys::fs::exists(ASTFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(ASTFileName));
+  EXPECT_TRUE(llvm::sys::fs::exists(DefinitionFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(DefinitionFileName));
+}
+
+TEST(CrossTranslationUnit, IndexFormatCanBeParsed) {
+  llvm::StringMap Index;
+  Index["a"] = "b";
+  Index["c"] = "d";
+  Index["e"] = "f";
+  std::string IndexText = createCrossTUIndexString(Index);
+  std::error_code EC;
+  llvm::raw_fd_ostream OS(IndexFileName, EC, llvm::sys::fs::F_Text);
+  OS << IndexText;
+  OS.flush();
+  EXPECT_TRUE(llvm::sys::fs::exists(IndexFileName));
+  llvm::Expected IndexOrErr =
+  parseCrossTUIndex(IndexFileName, "");
+  EXPECT_TRUE((bool)IndexOrErr);
+  llvm::StringMap ParsedIndex = IndexOrErr.get();
+  for (const auto  : Index) {
+

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

small nits




Comment at: include/clang/CrossTU/CrossTranslationUnit.h:58
+
+/// \brief This function can parse an index file that determines which
+///translation unit contains which definition.

I suggest that "can" is removed.



Comment at: include/clang/CrossTU/CrossTranslationUnit.h:62
+/// The index file format is the following:
+/// each line consists of an USR separated by a filepath.
+llvm::Expected

there is no \return or \returns here.



Comment at: include/clang/CrossTU/CrossTranslationUnit.h:62
+/// The index file format is the following:
+/// each line consists of an USR separated by a filepath.
+llvm::Expected

danielmarjamaki wrote:
> there is no \return or \returns here.
maybe: each line consists of an USR and a filepath separated by a space



Comment at: include/clang/CrossTU/CrossTranslationUnit.h:68
+
+/// \brief This class can be used for tools that requires cross translation
+///unit capability.

Maybe /can be/is/ unless you envision that tools that require cross translation 
unit capability might use some other classes.



Comment at: include/clang/CrossTU/CrossTranslationUnit.h:92
+  /// definition of the function will be merged into the original AST using
+  /// the AST Importer. The declaration with the definition will be returned.
+  /// If no suitable definition is found in the index file, null will be

you should split out some of this information to a \return or \returns



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:60
+
+char IndexError::ID = 0;
+

redundant assignment



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:79
+  std::string Line;
+  unsigned LineNo = 0;
+  while (std::getline(ExternalFnMapFile, Line)) {

I suggest that LineNo is 1 on the first line.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:81
+  while (std::getline(ExternalFnMapFile, Line)) {
+size_t Pos = Line.find(" ");
+StringRef LineRef{Line};

Pos can be const



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:82
+size_t Pos = Line.find(" ");
+StringRef LineRef{Line};
+if (Pos > 0 && Pos != std::string::npos) {

LineRef can be const



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:84
+if (Pos > 0 && Pos != std::string::npos) {
+  FunctionName = LineRef.substr(0, Pos);
+  if (Result.count(FunctionName))

FunctionName and FileName can be moved here and it is possible to make these 
variables const.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:85
+  FunctionName = LineRef.substr(0, Pos);
+  if (Result.count(FunctionName))
+return llvm::make_error(

I would like to see some FunctionName validation. For instance "123" should not 
be a valid function name.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:89
+  FileName = LineRef.substr(Pos + 1);
+  SmallString<256> FilePath = CrossTUDir;
+  llvm::sys::path::append(FilePath, FileName);

Stupid question .. how will this work if the path is longer than 256 bytes?



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:102
+createCrossTUIndexString(const llvm::StringMap ) {
+  std::stringstream Result;
+  for (const auto  : Index) {

how about std::ostringstream , imho that is cleaner



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:121
+
+/// Recursively visits the funtion decls of a DeclContext, and looks up a
+/// function based on USRs.

/funtion/function/



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

LookupFnName could be const right?



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:148
+
+  std::string LookupFnName = getLookupName(FD);
+  if (LookupFnName.empty())

I believe LookupFnName can be const



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:189
+  return nullptr; // No definition found even in some other build unit.
+ASTFileName = It->second;
+auto ASTCacheEntry = FileASTUnitMap.find(ASTFileName);

It is possible to make ASTFileName const



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:223
+assert(ToDecl->hasBody());
+assert(FD->hasBody() && "Functions already imported should have body.");
+return ToDecl;

sorry I am probably missing something here.. you 

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In https://reviews.llvm.org/D34512#856821, @benlangmuir wrote:

> In https://reviews.llvm.org/D34512#856301, @xazax.hun wrote:
>
> > In https://reviews.llvm.org/D34512#856184, @dcoughlin wrote:
> >
> > > In either case, the important scenario I think we should support is 
> > > choosing at a call site to a C function the most likely definition of the 
> > > called function, based on number and type of parameters, from multiple 
> > > possible definitions in other translation units. If the API is rich 
> > > enough to support this then I think that is a good indication it will be 
> > > useful for other scenarios as well.
> >
> >
> > Note that the lookup is already based on USR which is similar to mangled 
> > names in a sense that it contains information about the function 
> > parameters. So the only way to get multiple candidates from the lookup is 
> > having multiple function definitions with the same signature.
>
>
> I just want to clarify that C function USRs do not contain type information, 
> although C++ USRs do.


Just double checked and indeed: 
https://github.com/llvm-mirror/clang/blob/master/lib/Index/USRGeneration.cpp#L236
Thank you for the clarification, it looks like I did not notice this detail. In 
this case, it might make sense to let the client disambiguate.


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

In https://reviews.llvm.org/D34512#856301, @xazax.hun wrote:

> In https://reviews.llvm.org/D34512#856184, @dcoughlin wrote:
>
> > In either case, the important scenario I think we should support is 
> > choosing at a call site to a C function the most likely definition of the 
> > called function, based on number and type of parameters, from multiple 
> > possible definitions in other translation units. If the API is rich enough 
> > to support this then I think that is a good indication it will be useful 
> > for other scenarios as well.
>
>
> Note that the lookup is already based on USR which is similar to mangled 
> names in a sense that it contains information about the function parameters. 
> So the only way to get multiple candidates from the lookup is having multiple 
> function definitions with the same signature.


I just want to clarify that C function USRs do not contain type information, 
although C++ USRs do.


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 113206.
xazax.hun added a comment.

- Added unit test to ensure the produced index format can be parsed.
- Added further diagnostics.


https://reviews.llvm.org/D34512

Files:
  include/clang/Basic/AllDiagnostics.h
  include/clang/Basic/CMakeLists.txt
  include/clang/Basic/Diagnostic.td
  include/clang/Basic/DiagnosticCrossTUKinds.td
  include/clang/Basic/DiagnosticIDs.h
  include/clang/CrossTU/CrossTUDiagnostic.h
  include/clang/CrossTU/CrossTranslationUnit.h
  lib/AST/ASTImporter.cpp
  lib/Basic/DiagnosticIDs.cpp
  lib/CMakeLists.txt
  lib/CrossTU/CMakeLists.txt
  lib/CrossTU/CrossTranslationUnit.cpp
  test/Analysis/func-mapping-test.cpp
  test/CMakeLists.txt
  test/lit.cfg
  tools/CMakeLists.txt
  tools/clang-func-mapping/CMakeLists.txt
  tools/clang-func-mapping/ClangFnMapGen.cpp
  tools/diagtool/DiagnosticNames.cpp
  unittests/CMakeLists.txt
  unittests/CrossTU/CMakeLists.txt
  unittests/CrossTU/CrossTranslationUnitTest.cpp

Index: unittests/CrossTU/CrossTranslationUnitTest.cpp
===
--- /dev/null
+++ unittests/CrossTU/CrossTranslationUnitTest.cpp
@@ -0,0 +1,122 @@
+//===- unittest/Tooling/CrossTranslationUnitTest.cpp - Tooling unit tests -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/CrossTU/CrossTranslationUnit.h"
+#include "clang/AST/ASTConsumer.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+
+namespace clang {
+namespace cross_tu {
+
+namespace {
+StringRef IndexFileName = "index.txt";
+StringRef ASTFileName = "f.ast";
+StringRef DefinitionFileName = "input.cc";
+
+class CTUASTConsumer : public clang::ASTConsumer {
+public:
+  explicit CTUASTConsumer(clang::CompilerInstance , bool *Success)
+  : CTU(CI), Success(Success) {}
+
+  void HandleTranslationUnit(ASTContext ) {
+const TranslationUnitDecl *TU = Ctx.getTranslationUnitDecl();
+const FunctionDecl *FD = nullptr;
+for (const Decl *D : TU->decls()) {
+  FD = dyn_cast(D);
+  if (FD && FD->getName() == "f")
+break;
+}
+assert(FD);
+bool OrigFDHasBody = FD->hasBody();
+
+// Prepare the index file and the AST file.
+std::error_code EC;
+llvm::raw_fd_ostream OS(IndexFileName, EC, llvm::sys::fs::F_Text);
+OS << "c:@F@f#I# " << ASTFileName << "\n";
+OS.flush();
+StringRef SourceText = "int f(int) { return 0; }\n";
+// This file must exist since the saved ASTFile will reference it.
+llvm::raw_fd_ostream OS2(DefinitionFileName, EC, llvm::sys::fs::F_Text);
+OS2 << SourceText;
+OS2.flush();
+std::unique_ptr ASTWithDefinition =
+tooling::buildASTFromCode(SourceText);
+ASTWithDefinition->Save(ASTFileName);
+
+// Load the definition from the AST file.
+const FunctionDecl *NewFD =
+CTU.getCrossTUDefinition(FD, ".", IndexFileName);
+
+*Success = NewFD && NewFD->hasBody() && !OrigFDHasBody;
+  }
+
+private:
+  CrossTranslationUnitContext CTU;
+  bool *Success;
+};
+
+class CTUAction : public clang::ASTFrontendAction {
+public:
+  CTUAction(bool *Success) : Success(Success) {}
+
+protected:
+  std::unique_ptr
+  CreateASTConsumer(clang::CompilerInstance , StringRef) override {
+return llvm::make_unique(CI, Success);
+  }
+
+private:
+  bool *Success;
+};
+
+} // end namespace
+
+TEST(CrossTranslationUnit, CanLoadFunctionDefinition) {
+  bool Success = false;
+  EXPECT_TRUE(tooling::runToolOnCode(new CTUAction(), "int f(int);"));
+  EXPECT_TRUE(Success);
+  EXPECT_TRUE(llvm::sys::fs::exists(IndexFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(IndexFileName));
+  EXPECT_TRUE(llvm::sys::fs::exists(ASTFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(ASTFileName));
+  EXPECT_TRUE(llvm::sys::fs::exists(DefinitionFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(DefinitionFileName));
+}
+
+TEST(CrossTranslationUnit, IndexFormatCanBeParsed) {
+  llvm::StringMap Index;
+  Index["a"] = "b";
+  Index["c"] = "d";
+  Index["e"] = "f";
+  std::string IndexText = createCrossTUIndexString(Index);
+  std::error_code EC;
+  llvm::raw_fd_ostream OS(IndexFileName, EC, llvm::sys::fs::F_Text);
+  OS << IndexText;
+  OS.flush();
+  EXPECT_TRUE(llvm::sys::fs::exists(IndexFileName));
+  llvm::Expected IndexOrErr =
+  parseCrossTUIndex(IndexFileName, "");
+  EXPECT_TRUE((bool)IndexOrErr);
+  llvm::StringMap ParsedIndex = IndexOrErr.get();
+  for (const auto  : Index) {
+EXPECT_TRUE(ParsedIndex.count(E.getKey()));
+EXPECT_EQ(ParsedIndex[E.getKey()], E.getValue());
+  }
+  for (const auto  : ParsedIndex)
+

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In https://reviews.llvm.org/D34512#856184, @dcoughlin wrote:

> In either case, the important scenario I think we should support is choosing 
> at a call site to a C function the most likely definition of the called 
> function, based on number and type of parameters, from multiple possible 
> definitions in other translation units. If the API is rich enough to support 
> this then I think that is a good indication it will be useful for other 
> scenarios as well.


Note that the lookup is already based on USR which is similar to mangled names 
in a sense that it contains information about the function parameters. So the 
only way to get multiple candidates from the lookup is having multiple function 
definitions with the same signature. It is only possible to disambiguate with 
knowledge about the linking graph. I think that information is better to be 
stored in the index in the future and do the disambiguation within the library 
instead of making the client do that.

> 
> 
>> The reason we introduced the diagnostic is that we assumed that the client 
>> can not recover from a configuration error and will end up reporting the 
>> problem to the user.
> 
> For the static analyzer client, at least, one possible recovery is performing 
> the existing invalidation that we do when calling a function defined in 
> another translation unit. (I'm not really sure this a good idea; I just 
> wanted to point out that the decision probably belongs with the client). I 
> think it is reasonable to report an error as a diagnostic -- but I think this 
> should be up to the client and I don't think it should show up in the index 
> file itself. In an ideal world the user wouldn't even know that file existed. 
> Further, for large projects/incremental rebuilds a text file is unlikely to 
> be a suitable representation for the index. In the long term I doubt the file 
> will be end-user editable/readable.

I agree that we do not consider this format to be final for the index. In the 
future, it would be great to include linking information as well and also have 
utilities to merge partial indexes. This is a first version to make the 
machinery work for the most common cases which are already pretty usable for 
the open source projects we tested.


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added a comment.

> The importing alters the ASTContext, so the only way to choose between the 
> definitions would be via a callback that is triggered before the import is 
> done. What do you think?

I think that could work. Another possibility would be to have a two step 
process. The first step would return a representation of possible definitions 
to import; then the client would chose which one to important and call another 
API to perform the actual import.

In either case, the important scenario I think we should support is choosing at 
a call site to a C function the most likely definition of the called function, 
based on number and type of parameters, from multiple possible definitions in 
other translation units. If the API is rich enough to support this then I think 
that is a good indication it will be useful for other scenarios as well.

> The reason we introduced the diagnostic is that we assumed that the client 
> can not recover from a configuration error and will end up reporting the 
> problem to the user.

For the static analyzer client, at least, one possible recovery is performing 
the existing invalidation that we do when calling a function defined in another 
translation unit. (I'm not really sure this a good idea; I just wanted to point 
out that the decision probably belongs with the client). I think it is 
reasonable to report an error as a diagnostic -- but I think this should be up 
to the client and I don't think it should show up in the index file itself. In 
an ideal world the user wouldn't even know that file existed. Further, for 
large projects/incremental rebuilds a text file is unlikely to be a suitable 
representation for the index. In the long term I doubt the file will be 
end-user editable/readable.


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 113088.
xazax.hun marked 2 inline comments as done.
xazax.hun added a comment.

- Updates according to review comments.


https://reviews.llvm.org/D34512

Files:
  include/clang/Basic/AllDiagnostics.h
  include/clang/Basic/CMakeLists.txt
  include/clang/Basic/Diagnostic.td
  include/clang/Basic/DiagnosticCrossTUKinds.td
  include/clang/Basic/DiagnosticIDs.h
  include/clang/CrossTU/CrossTUDiagnostic.h
  include/clang/CrossTU/CrossTranslationUnit.h
  lib/AST/ASTImporter.cpp
  lib/Basic/DiagnosticIDs.cpp
  lib/CMakeLists.txt
  lib/CrossTU/CMakeLists.txt
  lib/CrossTU/CrossTranslationUnit.cpp
  test/Analysis/func-mapping-test.cpp
  test/CMakeLists.txt
  test/lit.cfg
  tools/CMakeLists.txt
  tools/clang-func-mapping/CMakeLists.txt
  tools/clang-func-mapping/ClangFnMapGen.cpp
  tools/diagtool/DiagnosticNames.cpp
  unittests/CMakeLists.txt
  unittests/CrossTU/CMakeLists.txt
  unittests/CrossTU/CrossTranslationUnitTest.cpp

Index: unittests/CrossTU/CrossTranslationUnitTest.cpp
===
--- /dev/null
+++ unittests/CrossTU/CrossTranslationUnitTest.cpp
@@ -0,0 +1,98 @@
+//===- unittest/Tooling/CrossTranslationUnitTest.cpp - Tooling unit tests -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/AST/ASTConsumer.h"
+#include "clang/CrossTU/CrossTranslationUnit.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+
+namespace clang {
+namespace cross_tu {
+
+namespace {
+StringRef IndexFileName = "index.txt";
+StringRef ASTFileName = "f.ast";
+StringRef DefinitionFileName = "input.cc";
+
+class CTUASTConsumer : public clang::ASTConsumer {
+public:
+  explicit CTUASTConsumer(clang::CompilerInstance , bool *Success)
+  : CTU(CI), Success(Success) {}
+
+  void HandleTranslationUnit(ASTContext ) {
+const TranslationUnitDecl *TU = Ctx.getTranslationUnitDecl();
+const FunctionDecl *FD = nullptr;
+for (const Decl *D : TU->decls()) {
+  FD = dyn_cast(D);
+  if (FD && FD->getName() == "f")
+break;
+}
+assert(FD);
+bool OrigFDHasBody = FD->hasBody();
+
+// Prepare the index file and the AST file.
+std::error_code EC;
+llvm::raw_fd_ostream OS(IndexFileName, EC, llvm::sys::fs::F_Text);
+OS << "c:@F@f#I# " << ASTFileName << "\n";
+OS.flush();
+StringRef SourceText = "int f(int) { return 0; }\n";
+// This file must exist since the saved ASTFile will reference it.
+llvm::raw_fd_ostream OS2(DefinitionFileName, EC, llvm::sys::fs::F_Text);
+OS2 << SourceText;
+OS2.flush();
+std::unique_ptr ASTWithDefinition =
+tooling::buildASTFromCode(SourceText);
+ASTWithDefinition->Save(ASTFileName);
+
+// Load the definition from the AST file.
+const FunctionDecl *NewFD =
+CTU.getCrossTUDefinition(FD, ".", IndexFileName);
+
+*Success = NewFD && NewFD->hasBody() && !OrigFDHasBody;
+  }
+
+private:
+  CrossTranslationUnitContext CTU;
+  bool *Success;
+};
+
+class CTUAction : public clang::ASTFrontendAction {
+public:
+  CTUAction(bool *Success) : Success(Success) {}
+
+protected:
+  std::unique_ptr
+  CreateASTConsumer(clang::CompilerInstance , StringRef) override {
+return llvm::make_unique(CI, Success);
+  }
+
+private:
+  bool *Success;
+};
+
+} // end namespace
+
+TEST(CrossTranslationUnit, CanLoadFunctionDefinition) {
+  bool Success = false;
+  EXPECT_TRUE(tooling::runToolOnCode(new CTUAction(), "int f(int);"));
+  EXPECT_TRUE(Success);
+  EXPECT_TRUE(llvm::sys::fs::exists(IndexFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(IndexFileName));
+  EXPECT_TRUE(llvm::sys::fs::exists(ASTFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(ASTFileName));
+  EXPECT_TRUE(llvm::sys::fs::exists(DefinitionFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(DefinitionFileName));
+}
+
+} // end namespace cross_tu
+} // end namespace clang
Index: unittests/CrossTU/CMakeLists.txt
===
--- /dev/null
+++ unittests/CrossTU/CMakeLists.txt
@@ -0,0 +1,16 @@
+set(LLVM_LINK_COMPONENTS
+  ${LLVM_TARGETS_TO_BUILD}
+  Support
+  )
+
+add_clang_unittest(CrossTUTests
+  CrossTranslationUnitTest.cpp
+  )
+
+target_link_libraries(CrossTUTests
+  clangAST
+  clangBasic
+  clangCrossTU
+  clangFrontend
+  clangTooling
+  )
Index: unittests/CMakeLists.txt
===
--- unittests/CMakeLists.txt
+++ unittests/CMakeLists.txt
@@ -19,6 +19,7 @@
 endif()
 add_subdirectory(ASTMatchers)
 add_subdirectory(AST)
+add_subdirectory(CrossTU)
 add_subdirectory(Tooling)
 

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In https://reviews.llvm.org/D34512#854729, @dcoughlin wrote:

> Except for some drive-by nits, this is a high-level review.
>
> I have some high-level questions about the design of the library:
>
> 1. **How do you intend to handle the case when there are multiple function 
> definitions with the same USR?** Whose responsibility it is to deal with 
> this: the client (for example, the static analyzer) or the index/database 
> (libCrossTU)? This seems to me to be a fundamental part of the design for 
> this feature that is missing. If you expect the client to handle this 
> scenario (for example, to pick a definition) I would expect the library API 
> to return more than a single potential result for a query. If you expect the 
> the library to pick a definition then at a minimum you should document how 
> this will be done. If the library will treat this as an error then you should 
> communicate this to the client so it can attempt to recover from it.


Right now we rely on the script that is using the analyzer such as 
scan-build-py or codechecker to remove the duplicate USRs from the index before 
starting the analysis. Having an `ErrorOr` return type so the user can handle 
the case when multiple definitions or none of them is found could be useful. It 
is not feasible to return multiple definitions, however. The importing alters 
the ASTContext, so the only way to choose between the definitions would be via 
a callback that is triggered before the import is done. What do you think?

> 
> 
> 2. **Whose responsibility is it to handle errors that the library runs 
> into?** Right now several errors appear to reported to the end user as 
> diagnostics. It seems to me that the decision of how (or whether) to report 
> these errors to the end user should be up to a particular client. Are these 
> errors even actionable on the part of end users? My sense it that with the 
> envisioned workflow users would not know/care about the format of the 
> database file.

The intention was that to report to the user that the index format is invalid, 
since it is a configuration issue that she might be able to handle. But we 
could also return an error code and leave the reporting to the client. The 
reason we introduced the diagnostic is that we assumed that the client can not 
recover from a configuration error and will end up reporting the problem to the 
user.

> 
> 
> 3. **Where should the code that understands the database file format live?** 
> Right now the logic for the parsing of the index format is in libCrossTU and 
> the emission of the index is in the command-line tool. I think it would be 
> easier to maintain if there were a single place that understand the file 
> format. This way consumers and emitters of the index could be easily updated 
> when the format/representation changes. Additionally, I think it is important 
> to document this format.

This is a very good point! I will update the library.




Comment at: include/clang/Basic/DiagnosticCrossTUKinds.td:13
+def err_fnmap_parsing : Error<
+  "error parsing CrossTU index file: '%0' line: %1 'USR filename' format "
+  "expected">;

dcoughlin wrote:
> Is this a user-facing diagnostic? Do we expect users to know what 'CrossTU' 
> means? Or what 'USR' means?
It is. Do you have any alternative suggestion? What about `error parsing index 
file` without mentioning any of the terms you mentioned?



Comment at: include/clang/CrossTU/CrossTranslationUnit.h:60
+  ///
+  /// Note that the AST files should also be in the \p CrossTUDir.
+  const FunctionDecl *getCrossTUDefinition(const FunctionDecl *FD,

dcoughlin wrote:
> Can this document what happens when the function declaration cannot be found? 
> And what happens when multiple function declarations are found?
Sure. Good point.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:48
+   StringRef LookupFnName) 
{
+  if (!DC)
+return nullptr;

dcoughlin wrote:
> Should this assert that DC is not null? What is the reason to accept null 
> here?
Indeed, an assert should be better. 



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:81
+  if (!ExternalFnMapFile) {
+Context.getDiagnostics().Report(diag::err_fe_error_opening)
+<< ExternalFunctionMap << "required by the CrossTU functionality";

dcoughlin wrote:
> What is the rationale behind emitting this as a diagnostic? In general for a 
> library I would expect that errors would be communicated to the client, which 
> then would have responsibility for handling them, reporting them to the user, 
> or aborting.
Since this is likely to be a configuration error we were thinking that only the 
user can handle it. But I could transform this to an error code if you would 
prefer that solution. 



Comment at: 

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added a comment.

Except for some drive-by nits, this is a high-level review.

I have some high-level questions about the design of the library:

1. **How do you intend to handle the case when there are multiple function 
definitions with the same USR?** Whose responsibility it is to deal with this: 
the client (for example, the static analyzer) or the index/database 
(libCrossTU)? This seems to me to be a fundamental part of the design for this 
feature that is missing. If you expect the client to handle this scenario (for 
example, to pick a definition) I would expect the library API to return more 
than a single potential result for a query. If you expect the the library to 
pick a definition then at a minimum you should document how this will be done. 
If the library will treat this as an error then you should communicate this to 
the client so it can attempt to recover from it.

2. **Whose responsibility is it to handle errors that the library runs into?** 
Right now several errors appear to reported to the end user as diagnostics. It 
seems to me that the decision of how (or whether) to report these errors to the 
end user should be up to a particular client. Are these errors even actionable 
on the part of end users? My sense it that with the envisioned workflow users 
would not know/care about the format of the database file.

3. **Where should the code that understands the database file format live?** 
Right now the logic for the parsing of the index format is in libCrossTU and 
the emission of the index is in the command-line tool. I think it would be 
easier to maintain if there were a single place that understand the file 
format. This way consumers and emitters of the index could be easily updated 
when the format/representation changes. Additionally, I think it is important 
to document this format.




Comment at: include/clang/Basic/DiagnosticCrossTUKinds.td:13
+def err_fnmap_parsing : Error<
+  "error parsing CrossTU index file: '%0' line: %1 'USR filename' format "
+  "expected">;

Is this a user-facing diagnostic? Do we expect users to know what 'CrossTU' 
means? Or what 'USR' means?



Comment at: include/clang/CrossTU/CrossTranslationUnit.h:60
+  ///
+  /// Note that the AST files should also be in the \p CrossTUDir.
+  const FunctionDecl *getCrossTUDefinition(const FunctionDecl *FD,

Can this document what happens when the function declaration cannot be found? 
And what happens when multiple function declarations are found?



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:48
+   StringRef LookupFnName) 
{
+  if (!DC)
+return nullptr;

Should this assert that DC is not null? What is the reason to accept null here?



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:81
+  if (!ExternalFnMapFile) {
+Context.getDiagnostics().Report(diag::err_fe_error_opening)
+<< ExternalFunctionMap << "required by the CrossTU functionality";

What is the rationale behind emitting this as a diagnostic? In general for a 
library I would expect that errors would be communicated to the client, which 
then would have responsibility for handling them, reporting them to the user, 
or aborting.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:92
+StringRef LineRef{Line};
+if (Pos > 0 && Pos != std::string::npos) {
+  FunctionName = LineRef.substr(0, Pos);

It looks like this is parsing. Is the file format documented anywhere? Also, it 
would be good to keep the parsing and generation code  in the same place so 
that it is easy to figure out what to update when the file format changes.



Comment at: tools/clang-func-mapping/ClangFnMapGen.cpp:86
+  if (SM.isInMainFile(Body->getLocStart()))
+DefinedFuncsStr << LookupName.str().str() << " " << CurrentFileName
+<< "\n";

It seems like the functionality that writes an entry and the functionality that 
reads an entry should ideally be in the same place. This way when the format is 
updated it is obvious what other places need to be updated. Can the code that 
writes a mapping be moved to libCrossTU and can we have this tool link against 
it?


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Daniel Krupp via Phabricator via cfe-commits]

dkrupp added a subscriber: zaks.anna.
dkrupp added a comment.

The creation of this library (libCrossTU) is approved for importing function 
definitions.  @zaks.anna, @NoQ , @klimek could you please help us reviewing the 
code itself?

Then, when this is approved, we could progress with the review of the dependent 
"Support for naive cross translational unit analysis" 
(https://reviews.llvm.org/D30691) feature. The two patch is now in sync. 
Thanks a lot in advance.


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 110559.
xazax.hun marked 4 inline comments as done.
xazax.hun added a comment.

- Address review comments


https://reviews.llvm.org/D34512

Files:
  include/clang/Basic/AllDiagnostics.h
  include/clang/Basic/CMakeLists.txt
  include/clang/Basic/Diagnostic.td
  include/clang/Basic/DiagnosticCrossTUKinds.td
  include/clang/Basic/DiagnosticIDs.h
  include/clang/CrossTU/CrossTUDiagnostic.h
  include/clang/CrossTU/CrossTranslationUnit.h
  lib/AST/ASTImporter.cpp
  lib/Basic/DiagnosticIDs.cpp
  lib/CrossTU/CMakeLists.txt
  lib/CrossTU/CrossTranslationUnit.cpp
  test/Analysis/func-mapping-test.cpp
  test/CMakeLists.txt
  test/lit.cfg
  tools/CMakeLists.txt
  tools/clang-func-mapping/CMakeLists.txt
  tools/clang-func-mapping/ClangFnMapGen.cpp
  tools/diagtool/DiagnosticNames.cpp
  unittests/CrossTU/CMakeLists.txt
  unittests/CrossTU/CrossTranslationUnitTest.cpp

Index: unittests/CrossTU/CrossTranslationUnitTest.cpp
===
--- /dev/null
+++ unittests/CrossTU/CrossTranslationUnitTest.cpp
@@ -0,0 +1,98 @@
+//===- unittest/Tooling/CrossTranslationUnitTest.cpp - Tooling unit tests -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/AST/ASTConsumer.h"
+#include "clang/CrossTU/CrossTranslationUnit.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+
+namespace clang {
+namespace cross_tu {
+
+namespace {
+StringRef IndexFileName = "index.txt";
+StringRef ASTFileName = "f.ast";
+StringRef DefinitionFileName = "input.cc";
+
+class CTUASTConsumer : public clang::ASTConsumer {
+public:
+  explicit CTUASTConsumer(clang::CompilerInstance , bool *Success)
+  : CTU(CI), Success(Success) {}
+
+  void HandleTranslationUnit(ASTContext ) {
+const TranslationUnitDecl *TU = Ctx.getTranslationUnitDecl();
+const FunctionDecl *FD = nullptr;
+for (const Decl *D : TU->decls()) {
+  FD = dyn_cast(D);
+  if (FD && FD->getName() == "f")
+break;
+}
+assert(FD);
+bool OrigFDHasBody = FD->hasBody();
+
+// Prepare the index file and the AST file.
+std::error_code EC;
+llvm::raw_fd_ostream OS(IndexFileName, EC, llvm::sys::fs::F_Text);
+OS << "c:@F@f#I# " << ASTFileName << "\n";
+OS.flush();
+StringRef SourceText = "int f(int) { return 0; }\n";
+// This file must exist since the saved ASTFile will reference it.
+llvm::raw_fd_ostream OS2(DefinitionFileName, EC, llvm::sys::fs::F_Text);
+OS2 << SourceText;
+OS2.flush();
+std::unique_ptr ASTWithDefinition =
+tooling::buildASTFromCode(SourceText);
+ASTWithDefinition->Save(ASTFileName);
+
+// Load the definition from the AST file.
+const FunctionDecl *NewFD =
+CTU.getCrossTUDefinition(FD, ".", IndexFileName);
+
+*Success = NewFD && NewFD->hasBody() && !OrigFDHasBody;
+  }
+
+private:
+  CrossTranslationUnitContext CTU;
+  bool *Success;
+};
+
+class CTUAction : public clang::ASTFrontendAction {
+public:
+  CTUAction(bool *Success) : Success(Success) {}
+
+protected:
+  std::unique_ptr
+  CreateASTConsumer(clang::CompilerInstance , StringRef) override {
+return llvm::make_unique(CI, Success);
+  }
+
+private:
+  bool *Success;
+};
+
+} // end namespace
+
+TEST(CrossTranslationUnit, CanLoadFunctionDefinition) {
+  bool Success = false;
+  EXPECT_TRUE(tooling::runToolOnCode(new CTUAction(), "int f(int);"));
+  EXPECT_TRUE(Success);
+  EXPECT_TRUE(llvm::sys::fs::exists(IndexFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(IndexFileName));
+  EXPECT_TRUE(llvm::sys::fs::exists(ASTFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(ASTFileName));
+  EXPECT_TRUE(llvm::sys::fs::exists(DefinitionFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(DefinitionFileName));
+}
+
+} // end namespace cross_tu
+} // end namespace clang
Index: unittests/CrossTU/CMakeLists.txt
===
--- /dev/null
+++ unittests/CrossTU/CMakeLists.txt
@@ -0,0 +1,16 @@
+set(LLVM_LINK_COMPONENTS
+  ${LLVM_TARGETS_TO_BUILD}
+  Support
+  )
+
+add_clang_unittest(CrossTUTests
+  CrossTranslationUnitTest.cpp
+  )
+
+target_link_libraries(CrossTUTests
+  clangAST
+  clangBasic
+  clangCrossTU
+  clangFrontend
+  clangTooling
+  )
Index: tools/diagtool/DiagnosticNames.cpp
===
--- tools/diagtool/DiagnosticNames.cpp
+++ tools/diagtool/DiagnosticNames.cpp
@@ -32,6 +32,7 @@
  SFINAE,NOWERROR,SHOWINSYSHEADER,CATEGORY)\
   { #ENUM, diag::ENUM, STR_SIZE(#ENUM, uint8_t) },
 #include 

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: include/clang/CrossTU/CrossTranslationUnit.h:42
+/// Note that this class also implements caching.
+class CrossTranslationUnit {
+public:

xazax.hun wrote:
> whisperity wrote:
> > Does the name of this class make sense? If I say
> > 
> > 
> > ```
> > CrossTranslationUnit *ctuptr = new CrossTranslationUnit(...);
> > ```
> > 
> > did I construct a new //CrossTranslationUnit//? What even **is** a 
> > //CrossTranslationUnit//? Other class names, such as `ASTImporter`, 
> > `DiagOpts`, and `FrontendAction`, etc. somehow feel better at 
> > ~~transmitting~~ reflecting upon their meaning in their name.
> > 
> > 
> What would you think about `CrossTranslationUnitContext`?
> 
> The functionality of this class is have all the relevant data required for 
> doing Cross TU lookups and also provide the interface for that. 
WFM. I'm not sure whose authority is the class names, but I like it a lot 
better.


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In https://reviews.llvm.org/D34512#836831, @whisperity wrote:

> Apart from those in the in-line comments, I have a question: how safe is this 
> library to `Release` builds? I know this is only a submodule dependency for 
> the "real deal" in https://reviews.llvm.org/D30691, but I have seen some 
> asserts that "imported function should already have a body" and such.
>
> Will the static analyzer handle these errors gracefully and fall back to 
> "function is unknown, let's throw my presumptions out of the window" or will 
> it bail away? I'm explicitly thinking of the assert-lacking `Release` build.


The basic idea is that, if a function already have a body in the current 
translation unit and you still want to import it from somewhere else, it might 
be a programmer error, so we do not handle this gracefully.




Comment at: include/clang/CrossTU/CrossTranslationUnit.h:42
+/// Note that this class also implements caching.
+class CrossTranslationUnit {
+public:

whisperity wrote:
> Does the name of this class make sense? If I say
> 
> 
> ```
> CrossTranslationUnit *ctuptr = new CrossTranslationUnit(...);
> ```
> 
> did I construct a new //CrossTranslationUnit//? What even **is** a 
> //CrossTranslationUnit//? Other class names, such as `ASTImporter`, 
> `DiagOpts`, and `FrontendAction`, etc. somehow feel better at 
> ~~transmitting~~ reflecting upon their meaning in their name.
> 
> 
What would you think about `CrossTranslationUnitContext`?

The functionality of this class is have all the relevant data required for 
doing Cross TU lookups and also provide the interface for that. 


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Apart from those in the in-line comments, I have a question: how safe is this 
library to `Release` builds? I know this is only a submodule dependency for the 
"real deal" in https://reviews.llvm.org/D30691, but I have seen some asserts 
that "imported function should already have a body" and such.

Will the static analyzer handle these errors gracefully and fall back to 
"function is unknown, let's throw my presumptions out of the window" or will it 
bail away? I'm explicitly thinking of the assert-lacking `Release` build.




Comment at: include/clang/Basic/AllDiagnostics.h:20
 #include "clang/AST/CommentDiagnostic.h"
+#include "clang/CrossTU/CrossTUDiagnostic.h"
 #include "clang/Analysis/AnalysisDiagnostic.h"

Import file order?



Comment at: include/clang/CrossTU/CrossTranslationUnit.h:42
+/// Note that this class also implements caching.
+class CrossTranslationUnit {
+public:

Does the name of this class make sense? If I say


```
CrossTranslationUnit *ctuptr = new CrossTranslationUnit(...);
```

did I construct a new //CrossTranslationUnit//? What even **is** a 
//CrossTranslationUnit//? Other class names, such as `ASTImporter`, `DiagOpts`, 
and `FrontendAction`, etc. somehow feel better at ~~transmitting~~ reflecting 
upon their meaning in their name.





Comment at: lib/CrossTU/CrossTranslationUnit.cpp:45
+
+/// Recursively visit the funtion decls of a DeclContext, and looks up a
+/// function based on mangled name.

"visit**s** (...) and looks up" or "visit (...) and look up"



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:46
+/// Recursively visit the funtion decls of a DeclContext, and looks up a
+/// function based on mangled name.
+const FunctionDecl *

If we are using //USR// for lookup, then why does this look up "based on 
mangled name"?



Comment at: tools/CMakeLists.txt:25
   add_clang_subdirectory(scan-view)
+  add_clang_subdirectory(clang-func-mapping)
 endif()

The other "blocks" in this file look //somewhat// in alphabetic order, perhaps 
this should be added a few lines above?


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 109559.
xazax.hun marked 2 inline comments as done.
xazax.hun added a comment.

- Address further review comments.


https://reviews.llvm.org/D34512

Files:
  include/clang/Basic/AllDiagnostics.h
  include/clang/Basic/CMakeLists.txt
  include/clang/Basic/Diagnostic.td
  include/clang/Basic/DiagnosticCrossTUKinds.td
  include/clang/Basic/DiagnosticIDs.h
  include/clang/CrossTU/CrossTUDiagnostic.h
  include/clang/CrossTU/CrossTranslationUnit.h
  lib/AST/ASTImporter.cpp
  lib/Basic/DiagnosticIDs.cpp
  lib/CrossTU/CMakeLists.txt
  lib/CrossTU/CrossTranslationUnit.cpp
  test/Analysis/func-mapping-test.cpp
  test/CMakeLists.txt
  test/lit.cfg
  tools/CMakeLists.txt
  tools/clang-func-mapping/CMakeLists.txt
  tools/clang-func-mapping/ClangFnMapGen.cpp
  tools/diagtool/DiagnosticNames.cpp
  unittests/CrossTU/CMakeLists.txt
  unittests/CrossTU/CrossTranslationUnitTest.cpp

Index: unittests/CrossTU/CrossTranslationUnitTest.cpp
===
--- /dev/null
+++ unittests/CrossTU/CrossTranslationUnitTest.cpp
@@ -0,0 +1,98 @@
+//===- unittest/Tooling/CrossTranslationUnitTest.cpp - Tooling unit tests -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/AST/ASTConsumer.h"
+#include "clang/CrossTU/CrossTranslationUnit.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+
+namespace clang {
+namespace cross_tu {
+
+namespace {
+StringRef IndexFileName = "index.txt";
+StringRef ASTFileName = "f.ast";
+StringRef DefinitionFileName = "input.cc";
+
+class CTUASTConsumer : public clang::ASTConsumer {
+public:
+  explicit CTUASTConsumer(clang::CompilerInstance , bool *Success)
+  : CTU(CI), Success(Success) {}
+
+  void HandleTranslationUnit(ASTContext ) {
+const TranslationUnitDecl *TU = Ctx.getTranslationUnitDecl();
+const FunctionDecl *FD = nullptr;
+for (const Decl *D : TU->decls()) {
+  FD = dyn_cast(D);
+  if (FD && FD->getName() == "f")
+break;
+}
+assert(FD);
+bool OrigFDHasBody = FD->hasBody();
+
+// Prepare the index file and the AST file.
+std::error_code EC;
+llvm::raw_fd_ostream OS(IndexFileName, EC, llvm::sys::fs::F_Text);
+OS << "c:@F@f#I# " << ASTFileName << "\n";
+OS.flush();
+StringRef SourceText = "int f(int) { return 0; }\n";
+// This file must exist since the saved ASTFile will reference it.
+llvm::raw_fd_ostream OS2(DefinitionFileName, EC, llvm::sys::fs::F_Text);
+OS2 << SourceText;
+OS2.flush();
+std::unique_ptr ASTWithDefinition =
+tooling::buildASTFromCode(SourceText);
+ASTWithDefinition->Save(ASTFileName);
+
+// Load the definition from the AST file.
+const FunctionDecl *NewFD =
+CTU.getCrossTUDefinition(FD, ".", IndexFileName);
+
+*Success = NewFD && NewFD->hasBody() && !OrigFDHasBody;
+  }
+
+private:
+  CrossTranslationUnit CTU;
+  bool *Success;
+};
+
+class CTUAction : public clang::ASTFrontendAction {
+public:
+  CTUAction(bool *Success) : Success(Success) {}
+
+protected:
+  std::unique_ptr
+  CreateASTConsumer(clang::CompilerInstance , StringRef) override {
+return llvm::make_unique(CI, Success);
+  }
+
+private:
+  bool *Success;
+};
+
+} // end namespace
+
+TEST(CrossTranslationUnit, CanLoadFunctionDefinition) {
+  bool Success = false;
+  EXPECT_TRUE(tooling::runToolOnCode(new CTUAction(), "int f(int);"));
+  EXPECT_TRUE(Success);
+  EXPECT_TRUE(llvm::sys::fs::exists(IndexFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(IndexFileName));
+  EXPECT_TRUE(llvm::sys::fs::exists(ASTFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(ASTFileName));
+  EXPECT_TRUE(llvm::sys::fs::exists(DefinitionFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(DefinitionFileName));
+}
+
+} // end namespace cross_tu
+} // end namespace clang
Index: unittests/CrossTU/CMakeLists.txt
===
--- /dev/null
+++ unittests/CrossTU/CMakeLists.txt
@@ -0,0 +1,16 @@
+set(LLVM_LINK_COMPONENTS
+  ${LLVM_TARGETS_TO_BUILD}
+  Support
+  )
+
+add_clang_unittest(CrossTUTests
+  CrossTranslationUnitTest.cpp
+  )
+
+target_link_libraries(CrossTUTests
+  clangAST
+  clangBasic
+  clangCrossTU
+  clangFrontend
+  clangTooling
+  )
Index: tools/diagtool/DiagnosticNames.cpp
===
--- tools/diagtool/DiagnosticNames.cpp
+++ tools/diagtool/DiagnosticNames.cpp
@@ -32,6 +32,7 @@
  SFINAE,NOWERROR,SHOWINSYSHEADER,CATEGORY)\
   { #ENUM, diag::ENUM, STR_SIZE(#ENUM, uint8_t) },
 #include 

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added inline comments.



Comment at: include/clang/CrossTU/CrossTUDiagnostic.h:16
+namespace clang {
+  namespace diag {
+enum {

LLVM Style uses no indent for namespaces. Reformat with `clang-format`.



Comment at: include/clang/CrossTU/CrossTranslationUnit.h:70
+  llvm::StringMap FileASTUnitMap;
+  llvm::StringMap FunctionAstUnitMap;
+  llvm::StringMap FunctionFileMap;

Maybe rename to `FunctionASTUnitMap` for consistency with the previous line?


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 109552.
xazax.hun marked 3 inline comments as done.
xazax.hun added a comment.

- Addressed review comments.


https://reviews.llvm.org/D34512

Files:
  include/clang/Basic/AllDiagnostics.h
  include/clang/Basic/CMakeLists.txt
  include/clang/Basic/Diagnostic.td
  include/clang/Basic/DiagnosticCrossTUKinds.td
  include/clang/Basic/DiagnosticIDs.h
  include/clang/CrossTU/CrossTUDiagnostic.h
  include/clang/CrossTU/CrossTranslationUnit.h
  lib/AST/ASTImporter.cpp
  lib/Basic/DiagnosticIDs.cpp
  lib/CrossTU/CMakeLists.txt
  lib/CrossTU/CrossTranslationUnit.cpp
  test/Analysis/func-mapping-test.cpp
  test/CMakeLists.txt
  test/lit.cfg
  tools/CMakeLists.txt
  tools/clang-func-mapping/CMakeLists.txt
  tools/clang-func-mapping/ClangFnMapGen.cpp
  tools/diagtool/DiagnosticNames.cpp
  unittests/CrossTU/CMakeLists.txt
  unittests/CrossTU/CrossTranslationUnitTest.cpp

Index: unittests/CrossTU/CrossTranslationUnitTest.cpp
===
--- /dev/null
+++ unittests/CrossTU/CrossTranslationUnitTest.cpp
@@ -0,0 +1,98 @@
+//===- unittest/Tooling/CrossTranslationUnitTest.cpp - Tooling unit tests -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/AST/ASTConsumer.h"
+#include "clang/CrossTU/CrossTranslationUnit.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+
+namespace clang {
+namespace cross_tu {
+
+namespace {
+StringRef IndexFileName = "index.txt";
+StringRef ASTFileName = "f.ast";
+StringRef DefinitionFileName = "input.cc";
+
+class CTUASTConsumer : public clang::ASTConsumer {
+public:
+  explicit CTUASTConsumer(clang::CompilerInstance , bool *Success)
+  : CTU(CI), Success(Success) {}
+
+  void HandleTranslationUnit(ASTContext ) {
+const TranslationUnitDecl *TU = Ctx.getTranslationUnitDecl();
+const FunctionDecl *FD = nullptr;
+for (const Decl *D : TU->decls()) {
+  FD = dyn_cast(D);
+  if (FD && FD->getName() == "f")
+break;
+}
+assert(FD);
+bool OrigFDHasBody = FD->hasBody();
+
+// Prepare the index file and the AST file.
+std::error_code EC;
+llvm::raw_fd_ostream OS(IndexFileName, EC, llvm::sys::fs::F_Text);
+OS << "c:@F@f#I# " << ASTFileName << "\n";
+OS.flush();
+StringRef SourceText = "int f(int) { return 0; }\n";
+// This file must exist since the saved ASTFile will reference it.
+llvm::raw_fd_ostream OS2(DefinitionFileName, EC, llvm::sys::fs::F_Text);
+OS2 << SourceText;
+OS2.flush();
+std::unique_ptr ASTWithDefinition =
+tooling::buildASTFromCode(SourceText);
+ASTWithDefinition->Save(ASTFileName);
+
+// Load the definition from the AST file.
+const FunctionDecl *NewFD =
+CTU.getCrossTUDefinition(FD, ".", IndexFileName);
+
+*Success = NewFD && NewFD->hasBody() && !OrigFDHasBody;
+  }
+
+private:
+  CrossTranslationUnit CTU;
+  bool *Success;
+};
+
+class CTUAction : public clang::ASTFrontendAction {
+public:
+  CTUAction(bool *Success) : Success(Success) {}
+
+protected:
+  std::unique_ptr
+  CreateASTConsumer(clang::CompilerInstance , StringRef) override {
+return llvm::make_unique(CI, Success);
+  }
+
+private:
+  bool *Success;
+};
+
+} // end namespace
+
+TEST(CrossTranslationUnit, CanLoadFunctionDefinition) {
+  bool Success = false;
+  EXPECT_TRUE(tooling::runToolOnCode(new CTUAction(), "int f(int);"));
+  EXPECT_TRUE(Success);
+  EXPECT_TRUE(llvm::sys::fs::exists(IndexFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(IndexFileName));
+  EXPECT_TRUE(llvm::sys::fs::exists(ASTFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(ASTFileName));
+  EXPECT_TRUE(llvm::sys::fs::exists(DefinitionFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(DefinitionFileName));
+}
+
+} // end namespace cross_tu
+} // end namespace clang
Index: unittests/CrossTU/CMakeLists.txt
===
--- /dev/null
+++ unittests/CrossTU/CMakeLists.txt
@@ -0,0 +1,16 @@
+set(LLVM_LINK_COMPONENTS
+  ${LLVM_TARGETS_TO_BUILD}
+  Support
+  )
+
+add_clang_unittest(CrossTUTests
+  CrossTranslationUnitTest.cpp
+  )
+
+target_link_libraries(CrossTUTests
+  clangAST
+  clangBasic
+  clangCrossTU
+  clangFrontend
+  clangTooling
+  )
Index: tools/diagtool/DiagnosticNames.cpp
===
--- tools/diagtool/DiagnosticNames.cpp
+++ tools/diagtool/DiagnosticNames.cpp
@@ -32,6 +32,7 @@
  SFINAE,NOWERROR,SHOWINSYSHEADER,CATEGORY)\
   { #ENUM, diag::ENUM, STR_SIZE(#ENUM, uint8_t) },
 #include 

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In https://reviews.llvm.org/D34512#829712, @rsmith wrote:

> Organizationally, this seems fine. Carry on :)


Great news! Thank you!


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Organizationally, this seems fine. Carry on :)




Comment at: include/clang/Basic/DiagnosticFrontendKinds.td:229-231
+def err_fnmap_parsing : Error<
+  "error parsing CrossTU index file: '%0' line: %1 'USR filename' format "
+  "expected">;

Please add a new `DiagnosticCrossTUKinds.td` file for the new subdirectory.



Comment at: include/clang/CrossTU/CrossTranslationUnit.h:13-14
+//===--===//
+#ifndef LLVM_CLANG_TOOLING_CROSSTRANSLATIONUNIT_H
+#define LLVM_CLANG_TOOLING_CROSSTRANSLATIONUNIT_H
+

This should be updated to match the new directory.



Comment at: include/clang/CrossTU/CrossTranslationUnit.h:30
+
+namespace crossTU {
+

This doesn't follow either of our normal conventions for namespace names within 
Clang and LLVM (`CamelCase` and `snake_case`, with the latter being more 
common). Maybe `cross_tu`?


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In https://reviews.llvm.org/D34512#806505, @klimek wrote:

> Yep, I want Richard's approval for the name. I think he already expressed a 
> pro-pulling-out stance.


Great! Ping for the name approval.


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

Yep, I want Richard's approval for the name. I think he already expressed a 
pro-pulling-out stance.


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In https://reviews.llvm.org/D34512#803724, @klimek wrote:

> Specifically, ping Richard for new top-level lib in clang.


Richard proposed pulling this out into a separate library in the first place. 
Do we need his approval for the name? Or we want him to consider if this patch 
justifies the existence of a separate library?


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

Specifically, ping Richard for new top-level lib in clang.


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

gentle ping


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In https://reviews.llvm.org/D34512#800626, @whisperity wrote:

> In https://reviews.llvm.org/D34512#800502, @xazax.hun wrote:
>
> > In https://reviews.llvm.org/D34512#800499, @klimek wrote:
> >
> > > In https://reviews.llvm.org/D34512#800490, @xazax.hun wrote:
> > >
> > > > It looks like Richard approved libTooling as a dependency for clang on 
> > > > the mailing list 
> > > > (http://lists.llvm.org/pipermail/cfe-dev/2017-July/054536.html).
> > > >  If it is ok to have this code in libTooling (for now), I think we 
> > > > could start/continue the review of this patch.
> > >
> > >
> > > I read that somewhat differently? It seems like Richard basically 
> > > proposes adding a new library for things that control how we run clang in 
> > > a multi-TU scenario. I'd call it libIndex, but that already exists :)
> >
> >
> > No, but since Richard said he did not see any justifications to include 
> > this in libTooling, I had the impression this is not his final word, and in 
> > case you see it justified, this remains the suggested direction. 
> >  But in this case I will rewrite this patch to create a new library. Are 
> > you still interested in reviewing it? Do you have any other name in mind? 
> > What about libCrossTU?
>
>
>
>
> In https://reviews.llvm.org/D34512#800625, @xazax.hun wrote:
>
> > In https://reviews.llvm.org/D34512#800618, @klimek wrote:
> >
> > > +Richard as top-level code owner for new libs.
> > >
> > > For bikeshedding the name: I'd have liked libIndex, but that's already 
> > > taken. CrossTU doesn't seem too bad to me, too, though.
> >
> >
> > Some brainstorming for the bikeshedding: libProjet, libProjectIndex, 
> > libImport
> >  The reason I am not sure about the Index in the name because this code 
> > does not contain (yet) an interface to create/handle indexes. 
> >  The import might be a good once, since this library is basically wrapping 
> > the ASTImporter to import code from external sources, but it might be 
> > misleading, since ASTImporter is in the AST module.
>
>
> Cross-TU is now used (in the lifeline of these features introduced by the 
> team) in multiple contexts: CTU analysis, and now CTU lookup.
>  Maybe the name of this lib should reflect on the fact that the lib is used 
> to support **cross-TU definition lookup**.
>
> Hmm... `libCrossTULocator`?


Well, a better name for locator is index :)


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 105412.
xazax.hun marked an inline comment as done.
xazax.hun added a comment.

- Fix a copy and paste error and removed an unintended change.


https://reviews.llvm.org/D34512

Files:
  include/clang/Basic/DiagnosticFrontendKinds.td
  include/clang/CrossTU/CrossTranslationUnit.h
  lib/AST/ASTImporter.cpp
  lib/CrossTU/CMakeLists.txt
  lib/CrossTU/CrossTranslationUnit.cpp
  test/Analysis/func-mapping-test.cpp
  test/CMakeLists.txt
  test/lit.cfg
  tools/CMakeLists.txt
  tools/clang-func-mapping/CMakeLists.txt
  tools/clang-func-mapping/ClangFnMapGen.cpp
  unittests/CrossTU/CMakeLists.txt
  unittests/CrossTU/CrossTranslationUnitTest.cpp

Index: unittests/CrossTU/CrossTranslationUnitTest.cpp
===
--- /dev/null
+++ unittests/CrossTU/CrossTranslationUnitTest.cpp
@@ -0,0 +1,98 @@
+//===- unittest/Tooling/CrossTranslationUnitTest.cpp - Tooling unit tests -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/AST/ASTConsumer.h"
+#include "clang/CrossTU/CrossTranslationUnit.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+
+namespace clang {
+namespace crossTU {
+
+namespace {
+StringRef IndexFileName = "index.txt";
+StringRef ASTFileName = "f.ast";
+StringRef DefinitionFileName = "input.cc";
+
+class CTUASTConsumer : public clang::ASTConsumer {
+public:
+  explicit CTUASTConsumer(clang::CompilerInstance , bool *Success)
+  : CTU(CI), Success(Success) {}
+
+  void HandleTranslationUnit(ASTContext ) {
+const TranslationUnitDecl *TU = Ctx.getTranslationUnitDecl();
+const FunctionDecl *FD = nullptr;
+for (const Decl *D : TU->decls()) {
+  FD = dyn_cast(D);
+  if (FD && FD->getName() == "f")
+break;
+}
+assert(FD);
+bool OrigFDHasBody = FD->hasBody();
+
+// Prepare the index file and the AST file.
+std::error_code EC;
+llvm::raw_fd_ostream OS(IndexFileName, EC, llvm::sys::fs::F_Text);
+OS << "c:@F@f#I# " << ASTFileName << "\n";
+OS.flush();
+StringRef SourceText = "int f(int) { return 0; }\n";
+// This file must exist since the saved ASTFile will reference it.
+llvm::raw_fd_ostream OS2(DefinitionFileName, EC, llvm::sys::fs::F_Text);
+OS2 << SourceText;
+OS2.flush();
+std::unique_ptr ASTWithDefinition =
+tooling::buildASTFromCode(SourceText);
+ASTWithDefinition->Save(ASTFileName);
+
+// Load the definition from the AST file.
+const FunctionDecl *NewFD =
+CTU.getCrossTUDefinition(FD, ".", IndexFileName);
+
+*Success = NewFD && NewFD->hasBody() && !OrigFDHasBody;
+  }
+
+private:
+  CrossTranslationUnit CTU;
+  bool *Success;
+};
+
+class CTUAction : public clang::ASTFrontendAction {
+public:
+  CTUAction(bool *Success) : Success(Success) {}
+
+protected:
+  std::unique_ptr
+  CreateASTConsumer(clang::CompilerInstance , StringRef) override {
+return llvm::make_unique(CI, Success);
+  }
+
+private:
+  bool *Success;
+};
+
+} // end namespace
+
+TEST(CrossTranslationUnit, CanLoadFunctionDefinition) {
+  bool Success = false;
+  EXPECT_TRUE(tooling::runToolOnCode(new CTUAction(), "int f(int);"));
+  EXPECT_TRUE(Success);
+  EXPECT_TRUE(llvm::sys::fs::exists(IndexFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(IndexFileName));
+  EXPECT_TRUE(llvm::sys::fs::exists(ASTFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(ASTFileName));
+  EXPECT_TRUE(llvm::sys::fs::exists(DefinitionFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(DefinitionFileName));
+}
+
+} // end namespace tooling
+} // end namespace clang
Index: unittests/CrossTU/CMakeLists.txt
===
--- /dev/null
+++ unittests/CrossTU/CMakeLists.txt
@@ -0,0 +1,16 @@
+set(LLVM_LINK_COMPONENTS
+  ${LLVM_TARGETS_TO_BUILD}
+  Support
+  )
+
+add_clang_unittest(CrossTUTests
+  CrossTranslationUnitTest.cpp
+  )
+
+target_link_libraries(CrossTUTests
+  clangAST
+  clangBasic
+  clangCrossTU
+  clangFrontend
+  clangTooling
+  )
Index: tools/clang-func-mapping/ClangFnMapGen.cpp
===
--- /dev/null
+++ tools/clang-func-mapping/ClangFnMapGen.cpp
@@ -0,0 +1,127 @@
+//===- ClangFnMapGen.cpp ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//======//
+//
+// Clang tool which creates a list of 

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In https://reviews.llvm.org/D34512#800502, @xazax.hun wrote:

> In https://reviews.llvm.org/D34512#800499, @klimek wrote:
>
> > In https://reviews.llvm.org/D34512#800490, @xazax.hun wrote:
> >
> > > It looks like Richard approved libTooling as a dependency for clang on 
> > > the mailing list 
> > > (http://lists.llvm.org/pipermail/cfe-dev/2017-July/054536.html).
> > >  If it is ok to have this code in libTooling (for now), I think we could 
> > > start/continue the review of this patch.
> >
> >
> > I read that somewhat differently? It seems like Richard basically proposes 
> > adding a new library for things that control how we run clang in a multi-TU 
> > scenario. I'd call it libIndex, but that already exists :)
>
>
> No, but since Richard said he did not see any justifications to include this 
> in libTooling, I had the impression this is not his final word, and in case 
> you see it justified, this remains the suggested direction. 
>  But in this case I will rewrite this patch to create a new library. Are you 
> still interested in reviewing it? Do you have any other name in mind? What 
> about libCrossTU?




In https://reviews.llvm.org/D34512#800625, @xazax.hun wrote:

> In https://reviews.llvm.org/D34512#800618, @klimek wrote:
>
> > +Richard as top-level code owner for new libs.
> >
> > For bikeshedding the name: I'd have liked libIndex, but that's already 
> > taken. CrossTU doesn't seem too bad to me, too, though.
>
>
> Some brainstorming for the bikeshedding: libProjet, libProjectIndex, libImport
>  The reason I am not sure about the Index in the name because this code does 
> not contain (yet) an interface to create/handle indexes. 
>  The import might be a good once, since this library is basically wrapping 
> the ASTImporter to import code from external sources, but it might be 
> misleading, since ASTImporter is in the AST module.


Cross-TU is now used (in the lifeline of these features introduced by the team) 
in multiple contexts: CTU analysis, and now CTU lookup.
Maybe the name of this lib should reflect on the fact that the lib is used to 
support **cross-TU definition lookup**.

Hmm... `libCrossTULocator`?


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In https://reviews.llvm.org/D34512#800618, @klimek wrote:

> +Richard as top-level code owner for new libs.
>
> For bikeshedding the name: I'd have liked libIndex, but that's already taken. 
> CrossTU doesn't seem too bad to me, too, though.


Some brainstorming for the bikeshedding: libProjet, libProjectIndex, libImport
The reason I am not sure about the Index in the name because this code does not 
contain (yet) an interface to create/handle indexes. 
The import might be a good once, since this library is basically wrapping the 
ASTImporter to import code from external sources, but it might be misleading, 
since ASTImporter is in the AST module.


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.



Comment at: unittests/CrossTU/CMakeLists.txt:10
+
+target_link_libraries(ToolingTests
+  clangAST

`CrossTUTests`?


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a reviewer: rsmith.
klimek added a comment.

+Richard as top-level code owner for new libs.

For bikeshedding the name: I'd have liked libIndex, but that's already taken. 
CrossTU doesn't seem too bad to me, too, though.


https://reviews.llvm.org/D34512



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 105409.
xazax.hun retitled this revision from "[libTooling] Add preliminary Cross 
Translation Unit support for libTooling" to "Add preliminary Cross Translation 
Unit support library".
xazax.hun added a comment.

- Move CrossTU functionality into its own library


https://reviews.llvm.org/D34512

Files:
  include/clang/Basic/DiagnosticFrontendKinds.td
  include/clang/CrossTU/CrossTranslationUnit.h
  lib/AST/ASTImporter.cpp
  lib/CrossTU/CMakeLists.txt
  lib/CrossTU/CrossTranslationUnit.cpp
  lib/Tooling/CMakeLists.txt
  test/Analysis/func-mapping-test.cpp
  test/CMakeLists.txt
  test/lit.cfg
  tools/CMakeLists.txt
  tools/clang-func-mapping/CMakeLists.txt
  tools/clang-func-mapping/ClangFnMapGen.cpp
  unittests/CrossTU/CMakeLists.txt
  unittests/CrossTU/CrossTranslationUnitTest.cpp

Index: unittests/CrossTU/CrossTranslationUnitTest.cpp
===
--- /dev/null
+++ unittests/CrossTU/CrossTranslationUnitTest.cpp
@@ -0,0 +1,98 @@
+//===- unittest/Tooling/CrossTranslationUnitTest.cpp - Tooling unit tests -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/AST/ASTConsumer.h"
+#include "clang/CrossTU/CrossTranslationUnit.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+#include 
+
+namespace clang {
+namespace crossTU {
+
+namespace {
+StringRef IndexFileName = "index.txt";
+StringRef ASTFileName = "f.ast";
+StringRef DefinitionFileName = "input.cc";
+
+class CTUASTConsumer : public clang::ASTConsumer {
+public:
+  explicit CTUASTConsumer(clang::CompilerInstance , bool *Success)
+  : CTU(CI), Success(Success) {}
+
+  void HandleTranslationUnit(ASTContext ) {
+const TranslationUnitDecl *TU = Ctx.getTranslationUnitDecl();
+const FunctionDecl *FD = nullptr;
+for (const Decl *D : TU->decls()) {
+  FD = dyn_cast(D);
+  if (FD && FD->getName() == "f")
+break;
+}
+assert(FD);
+bool OrigFDHasBody = FD->hasBody();
+
+// Prepare the index file and the AST file.
+std::error_code EC;
+llvm::raw_fd_ostream OS(IndexFileName, EC, llvm::sys::fs::F_Text);
+OS << "c:@F@f#I# " << ASTFileName << "\n";
+OS.flush();
+StringRef SourceText = "int f(int) { return 0; }\n";
+// This file must exist since the saved ASTFile will reference it.
+llvm::raw_fd_ostream OS2(DefinitionFileName, EC, llvm::sys::fs::F_Text);
+OS2 << SourceText;
+OS2.flush();
+std::unique_ptr ASTWithDefinition =
+tooling::buildASTFromCode(SourceText);
+ASTWithDefinition->Save(ASTFileName);
+
+// Load the definition from the AST file.
+const FunctionDecl *NewFD =
+CTU.getCrossTUDefinition(FD, ".", IndexFileName);
+
+*Success = NewFD && NewFD->hasBody() && !OrigFDHasBody;
+  }
+
+private:
+  CrossTranslationUnit CTU;
+  bool *Success;
+};
+
+class CTUAction : public clang::ASTFrontendAction {
+public:
+  CTUAction(bool *Success) : Success(Success) {}
+
+protected:
+  std::unique_ptr
+  CreateASTConsumer(clang::CompilerInstance , StringRef) override {
+return llvm::make_unique(CI, Success);
+  }
+
+private:
+  bool *Success;
+};
+
+} // end namespace
+
+TEST(CrossTranslationUnit, CanLoadFunctionDefinition) {
+  bool Success = false;
+  EXPECT_TRUE(tooling::runToolOnCode(new CTUAction(), "int f(int);"));
+  EXPECT_TRUE(Success);
+  EXPECT_TRUE(llvm::sys::fs::exists(IndexFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(IndexFileName));
+  EXPECT_TRUE(llvm::sys::fs::exists(ASTFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(ASTFileName));
+  EXPECT_TRUE(llvm::sys::fs::exists(DefinitionFileName));
+  EXPECT_FALSE((bool)llvm::sys::fs::remove(DefinitionFileName));
+}
+
+} // end namespace tooling
+} // end namespace clang
Index: unittests/CrossTU/CMakeLists.txt
===
--- /dev/null
+++ unittests/CrossTU/CMakeLists.txt
@@ -0,0 +1,16 @@
+set(LLVM_LINK_COMPONENTS
+  ${LLVM_TARGETS_TO_BUILD}
+  Support
+  )
+
+add_clang_unittest(CrossTUTests
+  CrossTranslationUnitTest.cpp
+  )
+
+target_link_libraries(ToolingTests
+  clangAST
+  clangBasic
+  clangCrossTU
+  clangFrontend
+  clangTooling
+  )
Index: tools/clang-func-mapping/ClangFnMapGen.cpp
===
--- /dev/null
+++ tools/clang-func-mapping/ClangFnMapGen.cpp
@@ -0,0 +1,127 @@
+//===- ClangFnMapGen.cpp ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See